linux-kernel - Re: [PATCH v2 06/10] objtool: Implement jump_assert for _static_cpu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20180116230256.7uhpc3bzfj7vivmr@treble>
Date:   Tue, 16 Jan 2018 17:02:56 -0600
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     David Woodhouse <dwmw2@...radead.org>,
        linux-kernel@...r.kernel.org, Dave Hansen <dave.hansen@...el.com>,
        Ashok Raj <ashok.raj@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Tim Chen <tim.c.chen@...ux.intel.com>,
        Andy Lutomirski <luto@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Arjan Van De Ven <arjan.van.de.ven@...el.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Jun Nakajima <jun.nakajima@...el.com>,
        Asit Mallick <asit.k.mallick@...el.com>,
        Borislav Petkov <bp@...en8.de>
Subject: Re: [PATCH v2 06/10] objtool: Implement jump_assert for
 _static_cpu_has()

On Tue, Jan 16, 2018 at 03:28:31PM +0100, Peter Zijlstra wrote:
> Unlike the jump_label bits, static_cpu_has is implemented with
> alternatives. We use the new type field to distinguish them from any
> other alternatives
> 
> Like jump_labels, make static_cpu_has set static_jump_dest on the
> instructions after the static branch such that we can assert on it.
> 
> Cc: Borislav Petkov <bp@...en8.de>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Josh Poimboeuf <jpoimboe@...hat.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> ---
>  tools/objtool/check.c   |   21 +++++++++++++++++++++
>  tools/objtool/special.c |   11 +++++++++++
>  tools/objtool/special.h |    1 +
>  3 files changed, 33 insertions(+)
> 
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -636,6 +636,12 @@ static int handle_group_alt(struct objto
>  	fake_jump->ignore = true;
>  
>  	if (!special_alt->new_len) {
> +		/*
> +		 * The NOP case for _static_cpu_has()
> +		 */
> +		if (special_alt->static_feat)

s/static_feat/static_cpu_has/ ?

> +			fake_jump->jump_dest->static_jump_dest = true;
> +
>  		*new_insn = fake_jump;
>  		return 0;
>  	}
> @@ -664,6 +670,21 @@ static int handle_group_alt(struct objto
>  				  insn->sec, insn->offset);
>  			return -1;
>  		}
> +
> +		if (special_alt->static_feat) {
> +			if (insn->type != INSN_JUMP_UNCONDITIONAL) {
> +				WARN_FUNC("not an unconditional jump in _static_cpu_has()",
> +					  insn->sec, insn->offset);
> +			}

So I think this is trying to assert the fact that you're only expecting
a single instruction which is an unconditional jump.  We already weeded
out non-jumps earlier in the loop, so would it make sense to do this
check before the INSN_JUMP_CONDITIONAL/INSN_JUMP_UNCONDITIONAL check a
little higher up?

> +			if (insn->jump_dest == fake_jump) {
> +				WARN_FUNC("jump inside alternative for _static_cpu_has()",
> +					  insn->sec, insn->offset);
> +			}

The error message doesn't seem to match the condition, so I'm not sure
which one you're trying to check, or why.

IIRC, 'insn->jump_dest == fake_jump' means we reached the end of the
alternative code block without hitting a jump.

But based on the loop exit condition, I don't think it's ever possible
for insn->jump_dest to ever point to the fake_jump at the end.

> +			/*
> +			 * The JMP+disp case for _static_cpu_has()
> +			 */
> +			insn->jump_dest->static_jump_dest = true;
> +		}
>  	}
>  
>  	if (!last_new_insn) {
> --- a/tools/objtool/special.c
> +++ b/tools/objtool/special.c
> @@ -40,6 +40,11 @@
>  #define ALT_FEATURE_OFFSET	8
>  #define ALT_ORIG_LEN_OFFSET	10
>  #define ALT_NEW_LEN_OFFSET	11
> +#define ALT_PADDING_OFFSET	12
> +#define ALT_TYPE_OFFSET		13

We don't need the ALT_PADDING_OFFSET define (notice we have gaps
already, there are only defines for the ones we use).

> +
> +#define ALT_TYPE_DEFAULT	0
> +#define ALT_TYPE_STATIC_CPU_HAS	1
>  
>  #define X86_FEATURE_POPCNT (4*32+23)
>  
> @@ -99,10 +104,13 @@ static int get_alt_entry(struct elf *elf
>  
>  	if (entry->feature) {

Since this block now uses more than entry->feature, and is now just a
general alternatives thing, can you change it to

	if (entry->feature == ALT_FEATURE_OFFSET)

so it's more clear and slightly more future proof?

>  		unsigned short feature;
> +		unsigned char type;
>  
>  		feature = *(unsigned short *)(sec->data->d_buf + offset +
>  					      entry->feature);
>  
> +		type = *(unsigned char *)(sec->data->d_buf + offset + ALT_TYPE_OFFSET);
> +
>  		/*
>  		 * It has been requested that we don't validate the !POPCNT
>  		 * feature path which is a "very very small percentage of
> @@ -110,6 +118,9 @@ static int get_alt_entry(struct elf *elf
>  		 */
>  		if (feature == X86_FEATURE_POPCNT)
>  			alt->skip_orig = true;
> +
> +		if (type == ALT_TYPE_STATIC_CPU_HAS)
> +			alt->static_feat = true;
>  	}
>  
>  	orig_rela = find_rela_by_dest(sec, offset + entry->orig);
> --- a/tools/objtool/special.h
> +++ b/tools/objtool/special.h
> @@ -27,6 +27,7 @@ struct special_alt {
>  	bool group;
>  	bool skip_orig;
>  	bool jump_or_nop;
> +	bool static_feat;
>  
>  	struct section *orig_sec;
>  	unsigned long orig_off;
> 
> 

-- 
Josh