Re: [PATCH 3/3] efi/x86: simplify mixed mode call wrapper

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Fri, 27 Dec 2019 at 03:56, Andy Lutomirski <luto@kernel.org> wrote:
>
> On Thu, Dec 26, 2019 at 7:16 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > Calling 32-bit EFI runtime services from a 64-bit OS involves
> > switching back to the flat mapping with a stack carved out of
> > memory that is 32-bit addressable.
> >
> > There is no need to actually execute the 64-bit part of this
> > routine from the flat mapping as well, as long as the entry
> > and return address fit in 32 bits. There is also no need to
> > preserve part of the calling context in global variables: we
> > can simply preserve the old stack pointer in %r11 across the
> > call into 32-bit firmware, and use either stack to preserve
> > other values.
>
> The %r11 trick makes me a little bit nervous.  I can imagine a 32-bit
> firmware implementation clobbering r11 by one of a few means: SMM bugs
> (unlikely -- this would probably kill the system even outside of an
> EFI call) or, more likely, if some code module is actualy 64-bit.
> Maybe we shouldn't be worried about this.  More comments below.
>

I'll put it on the stack instead, just to be safe.

> > diff --git a/arch/x86/platform/efi/efi_thunk_64.S b/arch/x86/platform/efi/efi_thunk_64.S
> > index 3189f1394701..7357808d3ae8 100644
> > --- a/arch/x86/platform/efi/efi_thunk_64.S
> > +++ b/arch/x86/platform/efi/efi_thunk_64.S
> > @@ -28,11 +28,17 @@
> >  SYM_FUNC_START(efi64_thunk)
> >         push    %rbp
> >         push    %rbx
> > +       movl    %ds, %ebx
> > +       push    %rbx
> > +       movl    %es, %ebx
> > +       push    %rbx
> > +       movl    %ss, %ebx
> > +       push    %rbx
>
> I realize that you haven't actually changed any of the below, but this
> code has issues.
>
> You don't actually need to save %ss.  Loading KERNEL_DS is fine.  0
> would almost be fine, except that AMD CPUs have some oddities and the
> fallout would be subtle and annoying to debug.
>
> The kernel does not strictly guarantee that the selectors in DS and ES
> are always valid.  They're fairly likely to be valid when running
> syscalls, but code like this should not bet on it.  And the EFI thunk
> is missing exception handlers when it reloads them.  So the right
> thing to do is probably to get rid of all the segment handling in the
> asm for everything except CS and to move it into C, like:
>
> unsigned short ds, es;
>
> /* DS and ES contain user values.  We need to save them. */
> savesegment(ds, ds);
> savesegment(es, es);
>
> /* The 32-bit EFI code needs a valid DS, ES, and SS.  There's no need
> to save the old SS: __KERNEL_DS is always acceptable.  */
> loadsegment(ss, __KERNEL_DS);
> loadsegment(ds, __KERNEL_DS);
> loadsegment(es, __KERNEL_DS);
>
> __s = efi64_thunk(...);
>
> loadsegment(ds, ds);
> loadsegment(es, es);
>
> Want to make that change?

Sure.