Re: [PATCH v3] efi: Ignore unrealistically large option roms

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux