linux-kernel - Re: x86 entry perf unwinding failure (missing IRET

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: <20200428154909.4cjwetyyb2zhnq5i@treble>
Date:   Tue, 28 Apr 2020 10:49:09 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Jann Horn <jannh@...gle.com>, Andy Lutomirski <luto@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        the arch/x86 maintainers <x86@...nel.org>,
        kernel list <linux-kernel@...r.kernel.org>,
        alexandre.chartre@...cle.com
Subject: Re: x86 entry perf unwinding failure (missing IRET_REGS annotation
 on stack switch?)

On Tue, Apr 28, 2020 at 05:25:52PM +0200, Peter Zijlstra wrote:
> On Tue, Apr 28, 2020 at 09:31:57AM -0500, Josh Poimboeuf wrote:
> > That's quite the monstrosity, and I still don't see the point.  I
> > thought we decided to just disallow CFI changes in alternatives anyway?
> > That can be done much simpler.
> 
> Something like so then ?
> 
> ---
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 8443ec690051..d14d83e6edb0 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -940,6 +940,7 @@ static int handle_group_alt(struct objtool_file *file,
>  
>  		last_new_insn = insn;
>  
> +		insn->alt_group = true;
>  		insn->ignore = orig_insn->ignore_alts;
>  		insn->func = orig_insn->func;
>  
> @@ -2242,6 +2243,11 @@ static int handle_insn_ops(struct instruction *insn, struct insn_state *state)
>  	list_for_each_entry(op, &insn->stack_ops, list) {
>  		int res;
>  
> +		if (insn->alt_group) {
> +			WARN_FUNC("alternative has CFI", insn->sec, insn->offset);
> +			return 1;
> +		}
> +

ACK (separate patch)

>  		res = update_cfi_state(insn, &state->cfi, op);
>  		if (res)
>  			return res;
> @@ -2439,12 +2445,6 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>  
>  	sec = insn->sec;
>  
> -	if (insn->alt_group && list_empty(&insn->alts)) {
> -		WARN_FUNC("don't know how to handle branch to middle of alternative instruction group",
> -			  sec, insn->offset);
> -		return 1;
> -	}
> -

ACK (separate patch)

>  	while (1) {
>  		next_insn = next_insn_same_sec(file, insn);
>  
> @@ -2494,8 +2494,16 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>  				}
>  			}
>  
> -			if (skip_orig)
> +			if (skip_orig) {
> +				struct instruction *prev_insn = insn;
> +				sec_for_each_insn_continue(file, insn) {
> +					if (!insn->alt_group)
> +						break;
> +					if (!insn->visited)
> +						insn->cfi = prev_insn->cfi;
> +				}
>  				return 0;
> +			}

NACK :-)

What happens if you have two alternatives adjacent to each other (which
can definitely happen in this scenario)?

I still like my patch, at least the hack is done before the validate
code, so validate_branch() itself is simpler.

-- 
Josh