linux-kernel - Re: [PATCH v5 02/17] objtool: Better handle 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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200417182339.GJ20730@hirez.programming.kicks-ass.net>
Date:   Fri, 17 Apr 2020 20:23:39 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Alexandre Chartre <alexandre.chartre@...cle.com>
Cc:     tglx@...utronix.de, jpoimboe@...hat.com,
        linux-kernel@...r.kernel.org, x86@...nel.org, mhiramat@...nel.org,
        mbenes@...e.cz, jthierry@...hat.com
Subject: Re: [PATCH v5 02/17] objtool: Better handle IRET

On Fri, Apr 17, 2020 at 07:37:32PM +0200, Alexandre Chartre wrote:
> > @@ -2243,6 +2232,20 @@ static int validate_branch(struct objtoo
> >   			break;
> > +		case INSN_EXCEPTION_RETURN:
> > +			if (handle_insn_ops(insn, &state))
> > +				return 1;
> 
> Do we need to update the stack state for normal IRET? This wasn't done before.
> So shouldn't this better be:
> 
>                case INSN_EXCEPTION_RETURN:
>                         if (!func)
>                                 return 0;
> 
>                         if (handle_insn_ops(insn, &state))
>                                 return 1;
> 
>                         break;

Well, I was going to do the unconditional handle_insn_ops(), like
mentioned, but then that intra_function_call thing spoiled it.

It doesn't matter though; STT_NOTYPE doesn't care.

> > +
> > +			/*
> > +			 * This handles x86's sync_core() case, where we use an
> > +			 * IRET to self. All 'normal' IRET instructions are in
> > +			 * STT_NOTYPE entry symbols.
> > +			 */
> > +			if (func)
> > +				break;
> 
> Is it worth checking that func->name is effectively "sync_core"?

It's an inline..