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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Fri, 17 Apr 2020 16:53:31 -0700
From:   Andy Lutomirski <luto@...nel.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Alexandre Chartre <alexandre.chartre@...cle.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>, X86 ML <x86@...nel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Miroslav Benes <mbenes@...e.cz>, jthierry@...hat.com
Subject: Re: [PATCH v5 02/17] objtool: Better handle IRET

On Fri, Apr 17, 2020 at 11:23 AM Peter Zijlstra <peterz@...radead.org> wrote:
>
> 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..

I'm wondering if this would be easier if we just moved the guts of
sync_core() into asm.

In the near future, I think we want to rework it a tiny bit.  In
particular, I think we're going to want to make sync_core() do:

if (static_cpu_has(X86_FEATURE_SERIALIZE))
  asm volatile ("serialize");
else
  iret_to_self();

where iret_to_self() is the meat of the IRET hack.  And then we're
going to add a new thingy unmask_nmi() that does iret_to_self() on
everything except SEV-ES.  The near-term motivation is that I think we
have some genuine bugs in a couple of corner cases:

1. On AMD chips, if NMI hits user code with invalid CS or SS, we will
enter on the NMI stack, switch to the normal stack, and return with
IRET, and the IRET will fail.  And then we end up in a nasty state in
which NMIs are masked but the code path we run doesn't expect that.
So we should unmask_nmi() in fixup_bad_iret() or similar.  Intel CPUs
are unaffected because Intel is differently quirky.

2.  do_nmi() does this:

    if (user_mode(regs))
                mds_user_clear_cpu_buffers();

because it can't safely call prepare_exit_to_usermode().  This is a
gross wart and I'd like to fix it.  Fixing it involves teaching the
relevant code paths to unmask_nmis() if they're going to so IRQs-on
exit work.

None of this is really relevant to the current patch, but it wouldn't
be unreasonable to turn the IRET thing from an inline asm into a real
asm function if it makes objtool's life easier.