| Message ID | 20180623211903.4794-1-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | 2e6eb40ca5eb53836d18f3b9ac61ff2e0b417038 |
| Headers | show |
| Series | x86/efi: Fix incorrect invocation of PciIo->Attributes() | expand |
Hi Ard, On 23-06-18 23:19, Ard Biesheuvel wrote: > Commit 2c3625cb9fa2 > > efi/x86: Fold __setup_efi_pci32() and __setup_efi_pci64() into one function > > merged the two versions of __setup_efi_pciXX(), without taking into > account that the 32-bit version used a rather dodgy trick to pass an > immediate 0 constant as argument for a uint64_t parameter. > > The issue is caused by the fact that on x86, UEFI protocol method calls > are redirected via struct efi_config::call(), which is a variadic function, > and so the compiler has to infer the types of the parameters from the > arguments rather than from the prototype. As the 32-bit x86 calling > convention passes arguments via the stack, passing the unqualified > constant 0 twice is the same as passing 0ULL, which is why the 32-bit > code in __setup_efi_pci32() contained the following call: > > status = efi_early->call(pci->attributes, pci, > EfiPciIoAttributeOperationGet, 0, 0, > &attributes); > > to invoke this UEFI protocol method: > > typedef > EFI_STATUS > (EFIAPI *EFI_PCI_IO_PROTOCOL_ATTRIBUTES) ( > IN EFI_PCI_IO_PROTOCOL *This, > IN EFI_PCI_IO_PROTOCOL_ATTRIBUTE_OPERATION Operation, > IN UINT64 Attributes, > OUT UINT64 *Result OPTIONAL > ); > > After the merge, we inadvertently ended up with this version for both > 32-bit and 64-bit builds, breaking the latter. > > So replace the two zeroes with the explicitly typed constant 0ULL, > which works as expected on both 32-bit and 64-bit builds. > > Reported-by: Wilfried Klaebe <linux-kernel@lebenslange-mailadresse.de> > Tested-by: Wilfried Klaebe <linux-kernel@lebenslange-mailadresse.de> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > > Wilfried tested the 64-bit build, and I checked the generated assembly > of a 32-bit build with and without this patch, and they are identical. Ard, thank you for Cc-ing me on this. I've tested a 64 bit kernel build on a 32 bit UEFI firmware (so mixed mode) and this patch causes a reboot loop there. I do get grub (no surprise there as grub is unchanged), but as soon as the kernel loads the device resets. So I think we need some special casing there to deal with a 64 bit kernel calling into 32 bit UEFI. Regards, Hans > > Ingo, mind applying this directly? Thanks. > > arch/x86/boot/compressed/eboot.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c > index a8a8642d2b0b..e57665b4ba1c 100644 > --- a/arch/x86/boot/compressed/eboot.c > +++ b/arch/x86/boot/compressed/eboot.c > @@ -118,7 +118,7 @@ __setup_efi_pci(efi_pci_io_protocol_t *pci, struct pci_setup_rom **__rom) > void *romimage; > > status = efi_call_proto(efi_pci_io_protocol, attributes, pci, > - EfiPciIoAttributeOperationGet, 0, 0, > + EfiPciIoAttributeOperationGet, 0ULL, > &attributes); > if (status != EFI_SUCCESS) > return status; > -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 24 June 2018 at 15:16, Hans de Goede <hdegoede@redhat.com> wrote: > Hi Ard, > > > On 23-06-18 23:19, Ard Biesheuvel wrote: >> >> Commit 2c3625cb9fa2 >> >> efi/x86: Fold __setup_efi_pci32() and __setup_efi_pci64() into one >> function >> >> merged the two versions of __setup_efi_pciXX(), without taking into >> account that the 32-bit version used a rather dodgy trick to pass an >> immediate 0 constant as argument for a uint64_t parameter. >> >> The issue is caused by the fact that on x86, UEFI protocol method calls >> are redirected via struct efi_config::call(), which is a variadic >> function, >> and so the compiler has to infer the types of the parameters from the >> arguments rather than from the prototype. As the 32-bit x86 calling >> convention passes arguments via the stack, passing the unqualified >> constant 0 twice is the same as passing 0ULL, which is why the 32-bit >> code in __setup_efi_pci32() contained the following call: >> >> status = efi_early->call(pci->attributes, pci, >> EfiPciIoAttributeOperationGet, 0, 0, >> &attributes); >> >> to invoke this UEFI protocol method: >> >> typedef >> EFI_STATUS >> (EFIAPI *EFI_PCI_IO_PROTOCOL_ATTRIBUTES) ( >> IN EFI_PCI_IO_PROTOCOL *This, >> IN EFI_PCI_IO_PROTOCOL_ATTRIBUTE_OPERATION Operation, >> IN UINT64 Attributes, >> OUT UINT64 *Result OPTIONAL >> ); >> >> After the merge, we inadvertently ended up with this version for both >> 32-bit and 64-bit builds, breaking the latter. >> >> So replace the two zeroes with the explicitly typed constant 0ULL, >> which works as expected on both 32-bit and 64-bit builds. >> >> Reported-by: Wilfried Klaebe <linux-kernel@lebenslange-mailadresse.de> >> Tested-by: Wilfried Klaebe <linux-kernel@lebenslange-mailadresse.de> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> >> Wilfried tested the 64-bit build, and I checked the generated assembly >> of a 32-bit build with and without this patch, and they are identical. > > > Ard, thank you for Cc-ing me on this. > > I've tested a 64 bit kernel build on a 32 bit UEFI firmware (so mixed mode) > and this patch causes a reboot loop there. I do get grub (no surprise there > as grub is unchanged), but as soon as the kernel loads the device resets. > > So I think we need some special casing there to deal with a 64 bit kernel > calling into 32 bit UEFI. > OK, so mixed mode rears its ugly hand again :-( Considering we had other weird issues involving uint64_t types with the TPM code just this week, I wonder if this isn't a fundamental problem with the mixed mode thunking, and so I need some help from the x86 gurus (Ingo?) If the thunking code simply maps each 64-bit argument onto a 32-bit stack slot, it is obvious that passing uint64_t type arguments is impossible. So would it be possible to have a efi_config::call() variant that is annotated as expecting the i386 calling convention, and let the compiler handle this? All we'd need to do in the mixed mode thunking code is pushing an additional word [as we do know] for the function pointer. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 24 Jun 2018, Ard Biesheuvel wrote: > On 24 June 2018 at 15:16, Hans de Goede <hdegoede@redhat.com> wrote: > > Ard, thank you for Cc-ing me on this. > > > > I've tested a 64 bit kernel build on a 32 bit UEFI firmware (so mixed mode) > > and this patch causes a reboot loop there. I do get grub (no surprise there > > as grub is unchanged), but as soon as the kernel loads the device resets. > > > > So I think we need some special casing there to deal with a 64 bit kernel > > calling into 32 bit UEFI. > > > > OK, so mixed mode rears its ugly hand again :-( > > Considering we had other weird issues involving uint64_t types with > the TPM code just this week, I wonder if this isn't a fundamental > problem with the mixed mode thunking, and so I need some help from the > x86 gurus (Ingo?) > > If the thunking code simply maps each 64-bit argument onto a 32-bit > stack slot, it is obvious that passing uint64_t type arguments is > impossible. > > So would it be possible to have a efi_config::call() variant that is > annotated as expecting the i386 calling convention, and let the > compiler handle this? All we'd need to do in the mixed mode thunking > code is pushing an additional word [as we do know] for the function > pointer. Grumbl. This thing just went into rc2 a few minutes ago. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 24 June 2018 at 15:43, Thomas Gleixner <tglx@linutronix.de> wrote: > On Sun, 24 Jun 2018, Ard Biesheuvel wrote: >> On 24 June 2018 at 15:16, Hans de Goede <hdegoede@redhat.com> wrote: >> > Ard, thank you for Cc-ing me on this. >> > >> > I've tested a 64 bit kernel build on a 32 bit UEFI firmware (so mixed mode) >> > and this patch causes a reboot loop there. I do get grub (no surprise there >> > as grub is unchanged), but as soon as the kernel loads the device resets. >> > >> > So I think we need some special casing there to deal with a 64 bit kernel >> > calling into 32 bit UEFI. >> > >> >> OK, so mixed mode rears its ugly hand again :-( >> >> Considering we had other weird issues involving uint64_t types with >> the TPM code just this week, I wonder if this isn't a fundamental >> problem with the mixed mode thunking, and so I need some help from the >> x86 gurus (Ingo?) >> >> If the thunking code simply maps each 64-bit argument onto a 32-bit >> stack slot, it is obvious that passing uint64_t type arguments is >> impossible. >> >> So would it be possible to have a efi_config::call() variant that is >> annotated as expecting the i386 calling convention, and let the >> compiler handle this? All we'd need to do in the mixed mode thunking >> code is pushing an additional word [as we do know] for the function >> pointer. > > Grumbl. This thing just went into rc2 a few minutes ago. > Good. Without that patch, 64-bit Linux on 64-bit EFI is broken, which is much worse. It seems mixed mode is fundamentally broken anyway, so we can take a bit of time to fix it properly -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 24 Jun 2018, Ard Biesheuvel wrote: > On 24 June 2018 at 15:43, Thomas Gleixner <tglx@linutronix.de> wrote: > > On Sun, 24 Jun 2018, Ard Biesheuvel wrote: > >> On 24 June 2018 at 15:16, Hans de Goede <hdegoede@redhat.com> wrote: > >> > Ard, thank you for Cc-ing me on this. > >> > > >> > I've tested a 64 bit kernel build on a 32 bit UEFI firmware (so mixed mode) > >> > and this patch causes a reboot loop there. I do get grub (no surprise there > >> > as grub is unchanged), but as soon as the kernel loads the device resets. > >> > > >> > So I think we need some special casing there to deal with a 64 bit kernel > >> > calling into 32 bit UEFI. > >> > > >> > >> OK, so mixed mode rears its ugly hand again :-( > >> > >> Considering we had other weird issues involving uint64_t types with > >> the TPM code just this week, I wonder if this isn't a fundamental > >> problem with the mixed mode thunking, and so I need some help from the > >> x86 gurus (Ingo?) > >> > >> If the thunking code simply maps each 64-bit argument onto a 32-bit > >> stack slot, it is obvious that passing uint64_t type arguments is > >> impossible. > >> > >> So would it be possible to have a efi_config::call() variant that is > >> annotated as expecting the i386 calling convention, and let the > >> compiler handle this? All we'd need to do in the mixed mode thunking > >> code is pushing an additional word [as we do know] for the function > >> pointer. > > > > Grumbl. This thing just went into rc2 a few minutes ago. > > > > Good. Without that patch, 64-bit Linux on 64-bit EFI is broken, which > is much worse. > > It seems mixed mode is fundamentally broken anyway, so we can take a > bit of time to fix it properly Fair enough. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jun 24, 2018 at 03:42:19PM +0200, Ard Biesheuvel wrote: > Considering we had other weird issues involving uint64_t types with > the TPM code just this week, I wonder if this isn't a fundamental > problem with the mixed mode thunking, and so I need some help from the > x86 gurus (Ingo?) Looks like that's exactly what it does: /* * Convert x86-64 ABI params to i386 ABI */ subq $32, %rsp movl %esi, 0x0(%rsp) movl %edx, 0x4(%rsp) movl %ecx, 0x8(%rsp) movq %r8, %rsi movl %esi, 0xc(%rsp) movq %r9, %rsi movl %esi, 0x10(%rsp) I don't think there's a way to find out in thunk_64.S if the register was populated with a 64-bit or 32-bit value, but it *might* be possible to do it with a macro that accepts a __VA_ARGS__ argument, iterates over the parameters, checks whether a parameter is 64-bit and we're in mixed mode, and if so, passes in the high and low dword separately. Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 24 June 2018 at 16:53, Lukas Wunner <lukas@wunner.de> wrote: > On Sun, Jun 24, 2018 at 03:42:19PM +0200, Ard Biesheuvel wrote: >> Considering we had other weird issues involving uint64_t types with >> the TPM code just this week, Actually, that issue involved pointers to uint64_t types, so they seem unrelated after all, but that still means the mixed mode code is fragile in the presence of protocols taking uint64_t type arguments by value. >> I wonder if this isn't a fundamental >> problem with the mixed mode thunking, and so I need some help from the >> x86 gurus (Ingo?) > > Looks like that's exactly what it does: > > /* > * Convert x86-64 ABI params to i386 ABI > */ > subq $32, %rsp > movl %esi, 0x0(%rsp) > movl %edx, 0x4(%rsp) > movl %ecx, 0x8(%rsp) > movq %r8, %rsi > movl %esi, 0xc(%rsp) > movq %r9, %rsi > movl %esi, 0x10(%rsp) > > I don't think there's a way to find out in thunk_64.S if the register > was populated with a 64-bit or 32-bit value, but it *might* be possible > to do it with a macro that accepts a __VA_ARGS__ argument, iterates > over the parameters, checks whether a parameter is 64-bit and we're > in mixed mode, and if so, passes in the high and low dword separately. > That would be nice, but I don't see how the preprocessor can infer the size of a type. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 24 June 2018 at 16:57, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 24 June 2018 at 16:53, Lukas Wunner <lukas@wunner.de> wrote: >> On Sun, Jun 24, 2018 at 03:42:19PM +0200, Ard Biesheuvel wrote: >>> Considering we had other weird issues involving uint64_t types with >>> the TPM code just this week, > > Actually, that issue involved pointers to uint64_t types, so they seem > unrelated after all, but that still means the mixed mode code is > fragile in the presence of protocols taking uint64_t type arguments by > value. > >>> I wonder if this isn't a fundamental >>> problem with the mixed mode thunking, and so I need some help from the >>> x86 gurus (Ingo?) >> >> Looks like that's exactly what it does: >> >> /* >> * Convert x86-64 ABI params to i386 ABI >> */ >> subq $32, %rsp >> movl %esi, 0x0(%rsp) >> movl %edx, 0x4(%rsp) >> movl %ecx, 0x8(%rsp) >> movq %r8, %rsi >> movl %esi, 0xc(%rsp) >> movq %r9, %rsi >> movl %esi, 0x10(%rsp) >> >> I don't think there's a way to find out in thunk_64.S if the register >> was populated with a 64-bit or 32-bit value, but it *might* be possible >> to do it with a macro that accepts a __VA_ARGS__ argument, iterates >> over the parameters, checks whether a parameter is 64-bit and we're >> in mixed mode, and if so, passes in the high and low dword separately. >> > > That would be nice, but I don't see how the preprocessor can infer the > size of a type. As far as I can tell, only PciIo->Attributes () and efi_file_handle::open() [in efi_file_size()] are affected. I couldn't find any other occurrences of uint64_t arguments passed by value. For the latter case, the UEFI spec documents that the last u64 argument is ignored unless the one before it equals EFI_FILE_MODE_CREATE, which it doesn't. And I suppose we /could/ work around this occurrence by simply passing another couple of zero values just to be sure (and efi_file_size() is only called by the x86 code to handle initrd=, which I don't think anyone uses anyway?) [/me makes mental note to check whether we can deprecate initrd= handling in the stub for all architectures] So PciIo->Attributes () is really the only occurrence of this issue where we need to do something special. So I am leaning towards taking the easy way out here. and proposing some variant of Wilfried's original patch and simply pass two zeroes instead of one [almost as before] Thoughts? -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index a8a8642d2b0b..e57665b4ba1c 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -118,7 +118,7 @@ __setup_efi_pci(efi_pci_io_protocol_t *pci, struct pci_setup_rom **__rom) void *romimage; status = efi_call_proto(efi_pci_io_protocol, attributes, pci, - EfiPciIoAttributeOperationGet, 0, 0, + EfiPciIoAttributeOperationGet, 0ULL, &attributes); if (status != EFI_SUCCESS) return status;
Commit 2c3625cb9fa2 efi/x86: Fold __setup_efi_pci32() and __setup_efi_pci64() into one function merged the two versions of __setup_efi_pciXX(), without taking into account that the 32-bit version used a rather dodgy trick to pass an immediate 0 constant as argument for a uint64_t parameter. The issue is caused by the fact that on x86, UEFI protocol method calls are redirected via struct efi_config::call(), which is a variadic function, and so the compiler has to infer the types of the parameters from the arguments rather than from the prototype. As the 32-bit x86 calling convention passes arguments via the stack, passing the unqualified constant 0 twice is the same as passing 0ULL, which is why the 32-bit code in __setup_efi_pci32() contained the following call: status = efi_early->call(pci->attributes, pci, EfiPciIoAttributeOperationGet, 0, 0, &attributes); to invoke this UEFI protocol method: typedef EFI_STATUS (EFIAPI *EFI_PCI_IO_PROTOCOL_ATTRIBUTES) ( IN EFI_PCI_IO_PROTOCOL *This, IN EFI_PCI_IO_PROTOCOL_ATTRIBUTE_OPERATION Operation, IN UINT64 Attributes, OUT UINT64 *Result OPTIONAL ); After the merge, we inadvertently ended up with this version for both 32-bit and 64-bit builds, breaking the latter. So replace the two zeroes with the explicitly typed constant 0ULL, which works as expected on both 32-bit and 64-bit builds. Reported-by: Wilfried Klaebe <linux-kernel@lebenslange-mailadresse.de> Tested-by: Wilfried Klaebe <linux-kernel@lebenslange-mailadresse.de> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- Wilfried tested the 64-bit build, and I checked the generated assembly of a 32-bit build with and without this patch, and they are identical. Ingo, mind applying this directly? Thanks. arch/x86/boot/compressed/eboot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html