Date:   Sun, 13 May 2018 14:26:21 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Ard Biesheuvel <ard.biesheuvel@...aro.org>
Cc:     "Luis R . Rodriguez" <mcgrof@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H . Peter Anvin" <hpa@...or.com>, Peter Jones <pjones@...hat.com>,
        Dave Olsthoorn <dave@...aar.me>,
        Will Deacon <will.deacon@....com>,
        Andy Lutomirski <luto@...nel.org>,
        Matt Fleming <matt@...eblueprint.co.uk>,
        David Howells <dhowells@...hat.com>,
        Mimi Zohar <zohar@...ux.vnet.ibm.com>,
        Josh Triplett <josh@...htriplett.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Martin Fuzzey <mfuzzey@...keon.com>,
        Kalle Valo <kvalo@...eaurora.org>,
        Arend Van Spriel <arend.vanspriel@...adcom.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Nicolas Broeking <nbroeking@...com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Torsten Duwe <duwe@...e.de>, Kees Cook <keescook@...omium.org>,
        the arch/x86 maintainers <x86@...nel.org>,
        linux-efi@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

Hi,

On 05/13/2018 12:43 PM, Ard Biesheuvel wrote:
> On 13 May 2018 at 13:03, Hans de Goede <hdegoede@...hat.com> wrote:
>> Hi,
>>
>>
>> On 05/04/2018 06:56 AM, Ard Biesheuvel wrote:
>>>
>>> Hi Hans,
>>>
>>> One comment below, which I missed in review before.
>>>
>>> On 29 April 2018 at 11:35, Hans de Goede <hdegoede@...hat.com> wrote:
>>>>
>>>> Just like with PCI options ROMs, which we save in the setup_efi_pci*
>>>> functions from arch/x86/boot/compressed/eboot.c, the EFI code / ROM
>>>> itself
>>>> sometimes may contain data which is useful/necessary for peripheral
>>>> drivers
>>>> to have access to.
>>>>
>>>> Specifically the EFI code may contain an embedded copy of firmware which
>>>> needs to be (re)loaded into the peripheral. Normally such firmware would
>>>> be
>>>> part of linux-firmware, but in some cases this is not feasible, for 2
>>>> reasons:
>>>>
>>>> 1) The firmware is customized for a specific use-case of the chipset /
>>>> use
>>>> with a specific hardware model, so we cannot have a single firmware file
>>>> for the chipset. E.g. touchscreen controller firmwares are compiled
>>>> specifically for the hardware model they are used with, as they are
>>>> calibrated for a specific model digitizer.
>>>>
>>>> 2) Despite repeated attempts we have failed to get permission to
>>>> redistribute the firmware. This is especially a problem with customized
>>>> firmwares, these get created by the chip vendor for a specific ODM and
>>>> the
>>>> copyright may partially belong with the ODM, so the chip vendor cannot
>>>> give a blanket permission to distribute these.
>>>>
>>>> This commit adds support for finding peripheral firmware embedded in the
>>>> EFI code and making this available to peripheral drivers through the
>>>> standard firmware loading mechanism.
>>>>
>>>> Note we check the EFI_BOOT_SERVICES_CODE for embedded firmware near the
>>>> end
>>>> of start_kernel(), just before calling rest_init(), this is on purpose
>>>> because the typical EFI_BOOT_SERVICES_CODE memory-segment is too large
>>>> for
>>>> early_memremap(), so the check must be done after mm_init(). This relies
>>>> on EFI_BOOT_SERVICES_CODE not being free-ed until
>>>> efi_free_boot_services()
>>>> is called, which means that this will only work on x86 for now.
>>>>
>>>> Reported-by: Dave Olsthoorn <dave@...aar.me>
>>>> Suggested-by: Peter Jones <pjones@...hat.com>
>>>> Acked-by: Ard Biesheuvel <ard.biesheuvel@...aro.org>
>>>> Signed-off-by: Hans de Goede <hdegoede@...hat.com>
>>>> ---
>>>
>>> [...]
>>>>
>>>> diff --git a/drivers/firmware/efi/embedded-firmware.c
>>>> b/drivers/firmware/efi/embedded-firmware.c
>>>> new file mode 100644
>>>> index 000000000000..22a0f598b53d
>>>> --- /dev/null
>>>> +++ b/drivers/firmware/efi/embedded-firmware.c
>>>> @@ -0,0 +1,149 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Support for extracting embedded firmware for peripherals from EFI
>>>> code,
>>>> + *
>>>> + * Copyright (c) 2018 Hans de Goede <hdegoede@...hat.com>
>>>> + */
>>>> +
>>>> +#include <linux/crc32.h>
>>>> +#include <linux/dmi.h>
>>>> +#include <linux/efi.h>
>>>> +#include <linux/efi_embedded_fw.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/types.h>
>>>> +#include <linux/vmalloc.h>
>>>> +
>>>> +struct embedded_fw {
>>>> +       struct list_head list;
>>>> +       const char *name;
>>>> +       void *data;
>>>> +       size_t length;
>>>> +};
>>>> +
>>>> +static LIST_HEAD(found_fw_list);
>>>> +
>>>> +static const struct dmi_system_id * const embedded_fw_table[] = {
>>>> +       NULL
>>>> +};
>>>> +
>>>> +/*
>>>> + * Note the efi_check_for_embedded_firmwares() code currently makes the
>>>> + * following 2 assumptions. This may needs to be revisited if embedded
>>>> firmware
>>>> + * is found where this is not true:
>>>> + * 1) The firmware is only found in EFI_BOOT_SERVICES_CODE memory
>>>> segments
>>>> + * 2) The firmware always starts at an offset which is a multiple of 8
>>>> bytes
>>>> + */
>>>> +static int __init efi_check_md_for_embedded_firmware(
>>>> +       efi_memory_desc_t *md, const struct efi_embedded_fw_desc *desc)
>>>> +{
>>>> +       struct embedded_fw *fw;
>>>> +       u64 i, size;
>>>> +       u32 crc;
>>>> +       u8 *mem;
>>>> +
>>>> +       size = md->num_pages << EFI_PAGE_SHIFT;
>>>> +       mem = memremap(md->phys_addr, size, MEMREMAP_WB);
>>>> +       if (!mem) {
>>>> +               pr_err("Error mapping EFI mem at %#llx\n",
>>>> md->phys_addr);
>>>> +               return -ENOMEM;
>>>> +       }
>>>> +
>>>> +       size -= desc->length;
>>>> +       for (i = 0; i < size; i += 8) {
>>>> +               if (*((u64 *)(mem + i)) != *((u64 *)desc->prefix))
>>>> +                       continue;
>>>> +
>>>
>>>
>>> Please use the proper APIs here to cast u8* to u64*, i.e., either use
>>> get_unaligned64() or use memcmp()
>>
>>
>> But we know the memory addresses are 64 bit aligned, so using
>> get_unaligned64 seems wrong, and I'm not sure if the compiler is
>> smart enough to optimize a memcmp to the single 64 bit integer comparison
>> we want done here.
>>
> 
> Fair enough. The memory regions are indeed guaranteed to be 4k aligned.
> 
> So in that case, please make mem a u64* and cast the other way where needed.

Ok, I've reworked the code to get rid of the compares in the if condition.

Regards,

Hans



> 
>>>
>>>> +               /* Seed with ~0, invert to match crc32 userspace utility
>>>> */
>>>> +               crc = ~crc32(~0, mem + i, desc->length);
>>>> +               if (crc == desc->crc)
>>>> +                       break;
>>>> +       }
>>>> +
>>>> +       memunmap(mem);
>>>> +
>>>> +       if (i >= size)
>>>> +               return -ENOENT;
>>>> +
>>>> +       pr_info("Found EFI embedded fw '%s' crc %08x\n", desc->name,
>>>> desc->crc);
>>>> +
>>>> +       fw = kmalloc(sizeof(*fw), GFP_KERNEL);
>>>> +       if (!fw)
>>>> +               return -ENOMEM;
>>>> +
>>>> +       mem = memremap(md->phys_addr + i, desc->length, MEMREMAP_WB);
>>>> +       if (!mem) {
>>>> +               pr_err("Error mapping embedded firmware\n");
>>>> +               goto error_free_fw;
>>>> +       }
>>>> +       fw->data = kmemdup(mem, desc->length, GFP_KERNEL);
>>>> +       memunmap(mem);
>>>> +       if (!fw->data)
>>>> +               goto error_free_fw;
>>>> +
>>>> +       fw->name = desc->name;
>>>> +       fw->length = desc->length;
>>>> +       list_add(&fw->list, &found_fw_list);
>>>> +
>>>> +       return 0;
>>>> +
>>>> +error_free_fw:
>>>> +       kfree(fw);
>>>> +       return -ENOMEM;
>>>> +}
>>>> +
>>>> +void __init efi_check_for_embedded_firmwares(void)
>>>> +{
>>>> +       const struct efi_embedded_fw_desc *fw_desc;
>>>> +       const struct dmi_system_id *dmi_id;
>>>> +       efi_memory_desc_t *md;
>>>> +       int i, r;
>>>> +
>>>> +       for (i = 0; embedded_fw_table[i]; i++) {
>>>> +               dmi_id = dmi_first_match(embedded_fw_table[i]);
>>>> +               if (!dmi_id)
>>>> +                       continue;
>>>> +
>>>> +               fw_desc = dmi_id->driver_data;
>>>> +               for_each_efi_memory_desc(md) {
>>>> +                       if (md->type != EFI_BOOT_SERVICES_CODE)
>>>> +                               continue;
>>>> +
>>>> +                       r = efi_check_md_for_embedded_firmware(md,
>>>> fw_desc);
>>>> +                       if (r == 0)
>>>> +                               break;
>>>> +               }
>>>> +       }
>>>> +}
>>>> +
>>>> +int efi_get_embedded_fw(const char *name, void **data, size_t *size,
>>>> +                       size_t msize)
>>>> +{
>>>> +       struct embedded_fw *iter, *fw = NULL;
>>>> +       void *buf = *data;
>>>> +
>>>> +       list_for_each_entry(iter, &found_fw_list, list) {
>>>> +               if (strcmp(name, iter->name) == 0) {
>>>> +                       fw = iter;
>>>> +                       break;
>>>> +               }
>>>> +       }
>>>> +
>>>> +       if (!fw)
>>>> +               return -ENOENT;
>>>> +
>>>> +       if (msize && msize < fw->length)
>>>> +               return -EFBIG;
>>>> +
>>>> +       if (!buf) {
>>>> +               buf = vmalloc(fw->length);
>>>> +               if (!buf)
>>>> +                       return -ENOMEM;
>>>> +       }
>>>> +
>>>> +       memcpy(buf, fw->data, fw->length);
>>>> +       *size = fw->length;
>>>> +       *data = buf;
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(efi_get_embedded_fw);
>>>> diff --git a/include/linux/efi.h b/include/linux/efi.h
>>>> index 791088360c1e..23e8a9c26ce2 100644
>>>> --- a/include/linux/efi.h
>>>> +++ b/include/linux/efi.h
>>>> @@ -1575,6 +1575,12 @@ static inline void
>>>>    efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg) {
>>>> }
>>>>    #endif
>>>>
>>>> +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
>>>> +void efi_check_for_embedded_firmwares(void);
>>>> +#else
>>>> +static inline void efi_check_for_embedded_firmwares(void) { }
>>>> +#endif
>>>> +
>>>>    void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table);
>>>>
>>>>    /*
>>>> diff --git a/include/linux/efi_embedded_fw.h
>>>> b/include/linux/efi_embedded_fw.h
>>>> new file mode 100644
>>>> index 000000000000..0f7d4df3f57a
>>>> --- /dev/null
>>>> +++ b/include/linux/efi_embedded_fw.h
>>>> @@ -0,0 +1,25 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +#ifndef _LINUX_EFI_EMBEDDED_FW_H
>>>> +#define _LINUX_EFI_EMBEDDED_FW_H
>>>> +
>>>> +#include <linux/mod_devicetable.h>
>>>> +
>>>> +/**
>>>> + * struct efi_embedded_fw_desc - This struct is used by the EFI
>>>> embedded-fw
>>>> + *                               code to search for embedded firmwares.
>>>> + *
>>>> + * @name:   Name to register the firmware with if found
>>>> + * @prefix: First 8 bytes of the firmware
>>>> + * @length: Length of the firmware in bytes including prefix
>>>> + * @crc:    Inverted little endian Ethernet style CRC32, with 0xffffffff
>>>> seed
>>>> + */
>>>> +struct efi_embedded_fw_desc {
>>>> +       const char *name;
>>>> +       u8 prefix[8];
>>>> +       u32 length;
>>>> +       u32 crc;
>>>> +};
>>>> +
>>>> +int efi_get_embedded_fw(const char *name, void **dat, size_t *sz, size_t
>>>> msize);
>>>> +
>>>> +#endif
>>>> diff --git a/init/main.c b/init/main.c
>>>> index b795aa341a3a..ab29775b35db 100644
>>>> --- a/init/main.c
>>>> +++ b/init/main.c
>>>> @@ -729,6 +729,9 @@ asmlinkage __visible void __init start_kernel(void)
>>>>           arch_post_acpi_subsys_init();
>>>>           sfi_init_late();
>>>>
>>>> +       if (efi_enabled(EFI_PRESERVE_BS_REGIONS))
>>>> +               efi_check_for_embedded_firmwares();
>>>> +
>>>>           if (efi_enabled(EFI_RUNTIME_SERVICES)) {
>>>>                   efi_free_boot_services();
>>>>           }
>>>> --
>>>> 2.17.0
>>>>
>>

Powered by blists - more mailing lists