On 29 April 2018 at 12:00, Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote: > On 29 April 2018 at 11:08, Hans de Goede <hdegoede@xxxxxxxxxx> wrote: >> Hi, >> >> >> On 29-04-18 11:07, Ard Biesheuvel wrote: >>> >>> On 29 April 2018 at 10:40, Hans de Goede <hdegoede@xxxxxxxxxx> wrote: >>>> >>>> Hi, >>>> >>>> >>>> On 29-04-18 09:43, Ingo Molnar wrote: >>>>> >>>>> >>>>> >>>>> * Hans de Goede <hdegoede@xxxxxxxxxx> wrote: >>>>> >>>>>> diff --git a/arch/x86/boot/compressed/eboot.c >>>>>> b/arch/x86/boot/compressed/eboot.c >>>>>> index 47d3efff6805..8650ab268ee7 100644 >>>>>> --- a/arch/x86/boot/compressed/eboot.c >>>>>> +++ b/arch/x86/boot/compressed/eboot.c >>>>>> @@ -122,7 +122,14 @@ __setup_efi_pci32(efi_pci_io_protocol_32 *pci, >>>>>> struct pci_setup_rom **__rom) >>>>>> if (status != EFI_SUCCESS) >>>>>> return status; >>>>>> - if (!pci->romimage || !pci->romsize) >>>>>> + /* >>>>>> + * Some firmwares contain EFI function pointers at the place >>>>>> where the >>>>>> + * romimage and romsize fields are supposed to be. Typically >>>>>> the >>>>>> EFI >>>>>> + * code is mapped at high addresses, translating to an >>>>>> unrealistically >>>>>> + * large romsize. The UEFI spec limits the size of option ROMs >>>>>> to >>>>>> 16 >>>>>> + * MiB so we reject any roms over 16 MiB in size to catch this. >>>>>> + */ >>>>>> + if (!pci->romimage || !pci->romsize || pci->romsize > >>>>>> 0x1000000) >>>>>> return EFI_INVALID_PARAMETER; >>>>>> size = pci->romsize + sizeof(*rom); >>>>>> @@ -230,7 +237,14 @@ __setup_efi_pci64(efi_pci_io_protocol_64 *pci, >>>>>> struct pci_setup_rom **__rom) >>>>>> if (status != EFI_SUCCESS) >>>>>> return status; >>>>>> - if (!pci->romimage || !pci->romsize) >>>>>> + /* >>>>>> + * Some firmwares contain EFI function pointers at the place >>>>>> where the >>>>>> + * romimage and romsize fields are supposed to be. Typically >>>>>> the >>>>>> EFI >>>>>> + * code is mapped at high addresses, translating to an >>>>>> unrealistically >>>>>> + * large romsize. The UEFI spec limits the size of option ROMs >>>>>> to >>>>>> 16 >>>>>> + * MiB so we reject any roms over 16 MiB in size to catch this. >>>>>> + */ >>>>>> + if (!pci->romimage || !pci->romsize || pci->romsize > >>>>>> 0x1000000) >>>>>> return EFI_INVALID_PARAMETER; >>>>> >>>>> >>>>> >>>>> Any reason why this couldn't be factored out into a efi_check_rom(pci) >>>>> kind of >>>>> helper function >>>> >>>> >>>> >>>> The pci pointer is of 2 different types: >>>> >>>> __setup_efi_pci32(efi_pci_io_protocol_32 *pci, ... >>>> __setup_efi_pci64(efi_pci_io_protocol_64 *pci, ... >>>> >>>> I guess I could give the helper a romimage and romsize argument to get >>>> around that. >>>> Ard, do you want me to do a v4 with such a helper ? >>>> >>> >>> I think Lukas has a point, although I shouldn't expect you to implement >>> that. >>> >>> I will look into this myself, and rebase your patch on top of that. >> >> >> Ok, thanks. >> > > Actually, looking into this, I think I may have spotted a bug in this code. > Does the issue you are solving here occur when running a 64-bit kernel > on 32-bit EFI? Never mind. I was looking at romimage being defined as a void* in efi_pci_io_protocol32, which is a bug, but the romsize field is unambiguously defined as a uint64_t, and this is the field holding the bogus value. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
