On Thu, Dec 26, 2019 at 7:16 AM Ard Biesheuvel <ardb@xxxxxxxxxx> 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. > 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?
