Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
Hi, On 05/08/2018 06:12 PM, Luis R. Rodriguez wrote: On Fri, May 04, 2018 at 07:54:28AM +0200, Ard Biesheuvel wrote: On 4 May 2018 at 01:29, Luis R. Rodriguez wrote: On Sun, Apr 29, 2018 at 11:35:55AM +0200, Hans de Goede wrote: [...] diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst index c8bddbdcfd10..560dfed76e38 100644 --- a/Documentation/driver-api/firmware/request_firmware.rst +++ b/Documentation/driver-api/firmware/request_firmware.rst @@ -73,3 +73,69 @@ If something went wrong firmware_request() returns non-zero and fw_entry is set to NULL. Once your driver is done with processing the firmware it can call call firmware_release(fw_entry) to release the firmware image and any related resource. + +EFI embedded firmware support += This is a new fallback mechanism, please see: Documentation/driver-api/firmware/fallback-mechanisms.rst Refer to the section "Types of fallback mechanisms", augument the list there and then move the section "Firmware sysfs loading facility" to a new file, and then add a new file for your own. + +On some devices the system's EFI code / ROM may contain an embedded copy +of firmware for some of the system's integrated peripheral devices and +the peripheral's Linux device-driver needs to access this firmware. You in no way indicate this is a just an invented scheme, a custom solution and nothing standard. I realize Ard criticized that the EFI Firmware Volume Protocol is not part of the UEFI spec -- however it is a bit more widely used right? Why can't Linux support it instead? Most implementations of UEFI are based on PI, That seems to be the UEFI Platform Initialization specification: http://www.uefi.org/sites/default/files/resources/PI_Spec_1_6.pdf and so it is likely that the protocols are available. However, the PI spec does not cover firmware blobs, Indeed, I cannot find anything about it on the PI Spec, but I *can* easily find a few documents referring to the Firmware Volume Protocol: http://wiki.phoenix.com/wiki/index.php/EFI_FIRMWARE_VOLUME_PROTOCOL But this has no references at all... I see stupid patents over some of this and authentication mechanisms for it: https://patents.google.com/patent/US20170098084 and so it is undefined whether such blobs are self contained (i.e., in separate files in the firmware volume), statically linked into the driver or maybe even encrypted or otherwise encapsulated, and the actual loadable image only lives in memory. Got it, thanks this helps! There are two things then: 1) The "EFI Firmware Volume Protocol" ("FV" for short in your descriptions below), and whether to support it or not in the future and recommend it for future use cases. b) Han's EFI scraper to help support 2 drivers, and whether or not to recommend it for future use cases. Hans's case is the second one, i.e., the firmware is at an arbitrary offset in the driver image. Using the FV protocol in this case would result in a mix of both approaches: look up the driver file by GUID [which could change btw between different versions of the system firmware, although this is unlikely] and then still use the prefix/crc based approach to sift through the image itself. Got it. And to be clear its a reversed engineered solution to what two vendors decided to do. But my main objection is simply that from the UEFI forum point of view, there is a clear distinction between the OS visible interfaces in the UEFI spec and the internal interfaces in the PI spec (which for instance are not subject to the same rules when it comes to backward compatibility), and so I think we should not depend on PI at all. Ah I see. This is all the more important considering that we are trying to encourage the creation of other implementations of UEFI that are not based on PI (e.g., uboot for arm64 implements the required UEFI interfaces for booting the kernel via GRUB), and adding dependencies on PI protocols makes that a moving target. Got it! So in my view, we either take a ad-hoc approach which works for the few platforms we expect to support, in which case Hans's approach is sufficient, Modulo it needs some work for ARM as it only works for x86 right now ;) or we architect it properly, in which case we shouldn't depend on PI because it does not belong in a properly architected OS<->firmware exchange. OK, it sounds to me like we have room to then implement our own de-facto standard for letting vendors stuff firmware into EFI as we in the Linux community see fit. We can start out by supporting existing drivers, but also consider customizing this in the future for our own needs, so long as we document it and set expectations well. So we need to support what Hans is implementing for two reasons then: a) The FV Protocol cannot be used to support the two drivers he's trying to provide support for -- I believe Hans tried a
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 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 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 Suggested-by: Peter Jones Acked-by: Ard Biesheuvel Signed-off-by: Hans de Goede --- [...] diff --git a/drivers/firmware/efi/embedded-firmware.c b/drivers/firmware/efi/embedded-firmware.c new file mode 100644 index ..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 + */ + +#include +#include +#include +#include +#include +#include +#include + +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 (!m
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
On 13 May 2018 at 13:03, Hans de Goede 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 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 >>> Suggested-by: Peter Jones >>> Acked-by: Ard Biesheuvel >>> Signed-off-by: Hans de Goede >>> --- >> >> [...] >>> >>> diff --git a/drivers/firmware/efi/embedded-firmware.c >>> b/drivers/firmware/efi/embedded-firmware.c >>> new file mode 100644 >>> index ..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 >>> + */ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +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. >> >>> + /* Seed with ~0, invert to match crc32 userspace utility >>> */ >>> + crc = ~crc32(~0, mem + i, desc->length); >>> + if (crc == desc->crc) >>> + break
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
Hi, On 05/03/2018 11:35 PM, Andy Lutomirski wrote: On Thu, May 3, 2018 at 3:31 PM Luis R. Rodriguez wrote: On Wed, May 02, 2018 at 04:49:53PM +0200, Hans de Goede wrote: Hi, On 05/01/2018 09:29 PM, Andy Lutomirski wrote: On Sun, Apr 29, 2018 at 2:36 AM Hans de Goede wrote: +The EFI embedded-fw code works by scanning all EFI_BOOT_SERVICES_CODE memory +segments for an eight byte sequence matching prefix, if the prefix is found it +then does a crc32 over length bytes and if that matches makes a copy of length +bytes and adds that to its list with found firmwares. + Eww, gross. Is there really no better way to do this? I'm afraid not. Is the issue that the EFI code does not intend to pass the firmware to the OS but that it has a copy for its own purposes and that Linux is just going to hijack EFI's copy? If so, that's brilliant and terrible at the same time. Yes that is exactly the issue / what it happening here. + for (i = 0; i < size; i += 8) { + if (*((u64 *)(mem + i)) != *((u64 *)desc->prefix)) + continue; + + /* Seed with ~0, invert to match crc32 userspace utility */ + crc = ~crc32(~0, mem + i, desc->length); + if (crc == desc->crc) + break; + } I hate to play the security card, but this stinks a bit. The kernel obviously needs to trust the EFI boot services code since the EFI boot services code is free to modify the kernel image. But your patch is not actually getting this firmware blob from the boot services code via any defined interface -- you're literally snarfing up the blob from a range of memory. I fully expect there to be any number of ways for untrustworthy entities to inject malicious blobs into this memory range on quite a few implementations. For example, there are probably unauthenticated EFI variables and even parts of USB sticks and such that get read into boot services memory, and I see no reason at all to expect that nothing in the so-called "boot services code" range is actually just plain old boot services *heap*. Fortunately, given your design, this is very easy to fix. Just replace CRC32 with SHA-256 or similar. If you find the crypto api too ugly for this purpose, I have patches that only need a small amount of dusting off to give an entirely reasonable SHA-256 API in the kernel. My main reason for going with crc32 is that the scanning happens before the kernel is fully up and running (it happens just before the rest_init() call in start_kernel() (from init/main.c) I'm open to using the crypto api, but I was not sure if that is ready for use at that time. Not being sure is different than being certain. As Andy noted, if that does not work please poke Andy about the SHA-256 API he has which would enable its use in kernel. Nah, don't use the cryptoapi for this. You'll probably regret it for any number of reasons. My code is here: https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=crypto/sha256_bpf&id=e9e12f056f2abed50a30b762db9185799f5864e6 and its two parents. It needs a little bit of dusting and it needs checking that all combinations of modular and non-modular builds work. Ard probably has further comments. Looks good, I've cherry picked this into my personal tree and will make the next version of the EFI embedded-firmware patches use SHA256. As Luis already mentioned geting the EFI embedded-firmware patches upstream is not something urgent, so it is probably best to just wait for you to push these upstream I guess? Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
Hi, On 05/03/2018 11:31 PM, Luis R. Rodriguez wrote: On Wed, May 02, 2018 at 04:49:53PM +0200, Hans de Goede wrote: Hi, On 05/01/2018 09:29 PM, Andy Lutomirski wrote: On Sun, Apr 29, 2018 at 2:36 AM Hans de Goede wrote: +The EFI embedded-fw code works by scanning all EFI_BOOT_SERVICES_CODE memory +segments for an eight byte sequence matching prefix, if the prefix is found it +then does a crc32 over length bytes and if that matches makes a copy of length +bytes and adds that to its list with found firmwares. + Eww, gross. Is there really no better way to do this? I'm afraid not. Is the issue that the EFI code does not intend to pass the firmware to the OS but that it has a copy for its own purposes and that Linux is just going to hijack EFI's copy? If so, that's brilliant and terrible at the same time. Yes that is exactly the issue / what it happening here. + for (i = 0; i < size; i += 8) { + if (*((u64 *)(mem + i)) != *((u64 *)desc->prefix)) + continue; + + /* Seed with ~0, invert to match crc32 userspace utility */ + crc = ~crc32(~0, mem + i, desc->length); + if (crc == desc->crc) + break; + } I hate to play the security card, but this stinks a bit. The kernel obviously needs to trust the EFI boot services code since the EFI boot services code is free to modify the kernel image. But your patch is not actually getting this firmware blob from the boot services code via any defined interface -- you're literally snarfing up the blob from a range of memory. I fully expect there to be any number of ways for untrustworthy entities to inject malicious blobs into this memory range on quite a few implementations. For example, there are probably unauthenticated EFI variables and even parts of USB sticks and such that get read into boot services memory, and I see no reason at all to expect that nothing in the so-called "boot services code" range is actually just plain old boot services *heap*. Fortunately, given your design, this is very easy to fix. Just replace CRC32 with SHA-256 or similar. If you find the crypto api too ugly for this purpose, I have patches that only need a small amount of dusting off to give an entirely reasonable SHA-256 API in the kernel. My main reason for going with crc32 is that the scanning happens before the kernel is fully up and running (it happens just before the rest_init() call in start_kernel() (from init/main.c) I'm open to using the crypto api, but I was not sure if that is ready for use at that time. Not being sure is different than being certain. As Andy noted, if that does not work please poke Andy about the SHA-256 API he has which would enable its use in kernel. Right now this is just a crazy hack for *2* drivers. Its a lot of hacks for just that, so no need to rush this in just yet. I agree that there is no rush to get this in. I will rebase this on top of the "[PATCH v7 00/14] firmware_loader changes for v4.18" series you recently send as well as try to address all the remarks made sofar. I'm not entirely sure when I will get around to this. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
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 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 Suggested-by: Peter Jones Acked-by: Ard Biesheuvel Signed-off-by: Hans de Goede --- [...] diff --git a/drivers/firmware/efi/embedded-firmware.c b/drivers/firmware/efi/embedded-firmware.c new file mode 100644 index ..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 + */ + +#include +#include +#include +#include +#include +#include +#include + +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. 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(&
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
On Fri, May 04, 2018 at 07:54:28AM +0200, Ard Biesheuvel wrote: > On 4 May 2018 at 01:29, Luis R. Rodriguez wrote: > > On Sun, Apr 29, 2018 at 11:35:55AM +0200, Hans de Goede wrote: > [...] > >> diff --git a/Documentation/driver-api/firmware/request_firmware.rst > >> b/Documentation/driver-api/firmware/request_firmware.rst > >> index c8bddbdcfd10..560dfed76e38 100644 > >> --- a/Documentation/driver-api/firmware/request_firmware.rst > >> +++ b/Documentation/driver-api/firmware/request_firmware.rst > >> @@ -73,3 +73,69 @@ If something went wrong firmware_request() returns > >> non-zero and fw_entry > >> is set to NULL. Once your driver is done with processing the firmware it > >> can call call firmware_release(fw_entry) to release the firmware image > >> and any related resource. > >> + > >> +EFI embedded firmware support > >> += > > > > This is a new fallback mechanism, please see: > > > > Documentation/driver-api/firmware/fallback-mechanisms.rst > > > > Refer to the section "Types of fallback mechanisms", augument the list there > > and then move the section "Firmware sysfs loading facility" to a new file, > > and > > then add a new file for your own. > > > >> + > >> +On some devices the system's EFI code / ROM may contain an embedded copy > >> +of firmware for some of the system's integrated peripheral devices and > >> +the peripheral's Linux device-driver needs to access this firmware. > > > > You in no way indicate this is a just an invented scheme, a custom solution > > and > > nothing standard. I realize Ard criticized that the EFI Firmware Volume > > Protocol > > is not part of the UEFI spec -- however it is a bit more widely used right? > > Why can't Linux support it instead? > > > > Most implementations of UEFI are based on PI, That seems to be the UEFI Platform Initialization specification: http://www.uefi.org/sites/default/files/resources/PI_Spec_1_6.pdf > and so it is likely that > the protocols are available. However, the PI spec does not cover > firmware blobs, Indeed, I cannot find anything about it on the PI Spec, but I *can* easily find a few documents referring to the Firmware Volume Protocol: http://wiki.phoenix.com/wiki/index.php/EFI_FIRMWARE_VOLUME_PROTOCOL But this has no references at all... I see stupid patents over some of this and authentication mechanisms for it: https://patents.google.com/patent/US20170098084 > and so it is undefined whether such blobs are self > contained (i.e., in separate files in the firmware volume), statically > linked into the driver or maybe even encrypted or otherwise > encapsulated, and the actual loadable image only lives in memory. Got it, thanks this helps! There are two things then: 1) The "EFI Firmware Volume Protocol" ("FV" for short in your descriptions below), and whether to support it or not in the future and recommend it for future use cases. b) Han's EFI scraper to help support 2 drivers, and whether or not to recommend it for future use cases. > Hans's case is the second one, i.e., the firmware is at an arbitrary > offset in the driver image. Using the FV protocol in this case would > result in a mix of both approaches: look up the driver file by GUID > [which could change btw between different versions of the system > firmware, although this is unlikely] and then still use the prefix/crc > based approach to sift through the image itself. Got it. And to be clear its a reversed engineered solution to what two vendors decided to do. > But my main objection is simply that from the UEFI forum point of > view, there is a clear distinction between the OS visible interfaces > in the UEFI spec and the internal interfaces in the PI spec (which for > instance are not subject to the same rules when it comes to backward > compatibility), and so I think we should not depend on PI at all. Ah I see. > This > is all the more important considering that we are trying to encourage > the creation of other implementations of UEFI that are not based on PI > (e.g., uboot for arm64 implements the required UEFI interfaces for > booting the kernel via GRUB), and adding dependencies on PI protocols > makes that a moving target. Got it! > So in my view, we either take a ad-hoc approach which works for the > few platforms we expect to support, in which case Hans's approach is > sufficient, Modulo it needs some work for ARM as it only works for x86 right now ;) > or we architect it properly, in which case we shouldn't > depend on PI because it does not belong in a properly architected > OS<->firmware exchange. OK, it sounds to me like we have room to then implement our own de-facto standard for letting vendors stuff firmware into EFI as we in the Linux community see fit. We can start out by supporting existing drivers, but also consider customizing this in the future for our own needs, so long as we document it and set expectations well. So we need to support what Hans is implementing for two r
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
Hi Hans, One comment below, which I missed in review before. On 29 April 2018 at 11:35, Hans de Goede 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 > Suggested-by: Peter Jones > Acked-by: Ard Biesheuvel > Signed-off-by: Hans de Goede > --- [...] > diff --git a/drivers/firmware/efi/embedded-firmware.c > b/drivers/firmware/efi/embedded-firmware.c > new file mode 100644 > index ..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 > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +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() > + /* 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); > + > +
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
On 4 May 2018 at 01:29, Luis R. Rodriguez wrote: > On Sun, Apr 29, 2018 at 11:35:55AM +0200, Hans de Goede wrote: [...] >> diff --git a/Documentation/driver-api/firmware/request_firmware.rst >> b/Documentation/driver-api/firmware/request_firmware.rst >> index c8bddbdcfd10..560dfed76e38 100644 >> --- a/Documentation/driver-api/firmware/request_firmware.rst >> +++ b/Documentation/driver-api/firmware/request_firmware.rst >> @@ -73,3 +73,69 @@ If something went wrong firmware_request() returns >> non-zero and fw_entry >> is set to NULL. Once your driver is done with processing the firmware it >> can call call firmware_release(fw_entry) to release the firmware image >> and any related resource. >> + >> +EFI embedded firmware support >> += > > This is a new fallback mechanism, please see: > > Documentation/driver-api/firmware/fallback-mechanisms.rst > > Refer to the section "Types of fallback mechanisms", augument the list there > and then move the section "Firmware sysfs loading facility" to a new file, and > then add a new file for your own. > >> + >> +On some devices the system's EFI code / ROM may contain an embedded copy >> +of firmware for some of the system's integrated peripheral devices and >> +the peripheral's Linux device-driver needs to access this firmware. > > You in no way indicate this is a just an invented scheme, a custom solution > and > nothing standard. I realize Ard criticized that the EFI Firmware Volume > Protocol > is not part of the UEFI spec -- however it is a bit more widely used right? > Why can't Linux support it instead? > Most implementations of UEFI are based on PI, and so it is likely that the protocols are available. However, the PI spec does not cover firmware blobs, and so it is undefined whether such blobs are self contained (i.e., in separate files in the firmware volume), statically linked into the driver or maybe even encrypted or otherwise encapsulated, and the actual loadable image only lives in memory. Hans's case is the second one, i.e., the firmware is at an arbitrary offset in the driver image. Using the FV protocol in this case would result in a mix of both approaches: look up the driver file by GUID [which could change btw between different versions of the system firmware, although this is unlikely] and then still use the prefix/crc based approach to sift through the image itself. But my main objection is simply that from the UEFI forum point of view, there is a clear distinction between the OS visible interfaces in the UEFI spec and the internal interfaces in the PI spec (which for instance are not subject to the same rules when it comes to backward compatibility), and so I think we should not depend on PI at all. This is all the more important considering that we are trying to encourage the creation of other implementations of UEFI that are not based on PI (e.g., uboot for arm64 implements the required UEFI interfaces for booting the kernel via GRUB), and adding dependencies on PI protocols makes that a moving target. So in my view, we either take a ad-hoc approach which works for the few platforms we expect to support, in which case Hans's approach is sufficient, or we architect it properly, in which case we shouldn't depend on PI because it does not belong in a properly architected OS<->firmware exchange. -- Ard. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
Please Cc andre...@gmail.com on future patches. On Sun, Apr 29, 2018 at 11:35:55AM +0200, Hans de Goede 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. Interesting. Mimi you may want to look into that for appraisal gap coverage. > 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: This and other reasons have been scattered through mailing lists, and documentation, our Kconfig does not reflect which of the reasons are currently addressed as how. Our goal for your commit log would be to document the precise reason you can't use any of the existing interfaces exposed. Let's see if some of the reasons you state cannot be used with the existing APIs. > 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. In such cases the firmware may be stashed in a separate partition of a custom device and firmwared could be used with a uevent and the fallback mechanism, enabling userspace to do whatever it needs. However in your case the firmware has been found on EFI. > 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. Indeed, we don't have a good way to deal with this now except the above noted firmwared use case, and that still ponies up the redistribution problem up to a system integrator somehow. But also -- your solution is super custom to only two drivers, does not follow a standard of any sort either, and I fear of setting any wrong precedents which will be hard to later not support. Just look at the fallback mechanism and how hairy it got, I've put some love into it recently but it was a pain to even comprehend. Once we add this interface we are going to have to support it for a while so I want to be sure we get the architecture right. Specially if were going to enable *other* vendors to start using such interface. Is your goal to enable or encourage other vendors in similar predicament to use the same strategy? There is nothing wrong in setting de-facto standards for Linux, we do this all the time, but if we are going to do it I want to be sure we document this well and provide proper justifications in the commit log and documentation. > This commit adds support for finding peripheral firmware embedded in the > EFI code Please specify this is through a custom scheme as it reads now it seems to read this is a sort of standard. > diff --git a/Documentation/driver-api/firmware/request_firmware.rst > b/Documentation/driver-api/firmware/request_firmware.rst > index c8bddbdcfd10..560dfed76e38 100644 > --- a/Documentation/driver-api/firmware/request_firmware.rst > +++ b/Documentation/driver-api/firmware/request_firmware.rst > @@ -73,3 +73,69 @@ If something went wrong firmware_request() returns > non-zero and fw_entry > is set to NULL. Once your driver is done with processing the firmware it > can call call firmware_release(fw_entry) to release the firmware image > and any related resource. > + > +EFI embedded firmware support > += This is a new fallback mechanism, please see: Documentation/driver-api/firmware/fallback-mechanisms.rst Refer to the section "Types of fallback mechanisms", augument the list there and then move the section "Firmware sysfs loading facility" to a new file, and then add a new file for your own. > + > +On some devices the system's EFI code / ROM may contain an embedded copy > +of firmware for some of the system's integrated peripheral devices and > +the peripheral's Linux device-driver needs to access this firmware. You in no way indicate this is a just an invented scheme, a custom solution and nothing standard. I realize Ard criticized that the EFI Firmware Volume Protocol is not part of the UEFI spec -- however it is a bit more widely used right? Why can't Linux support it instead? Could your new scheme grow to support the EFI Firmware Volume Protocol rather easily if someone wants to wrap up their sleeves? If so what likely would change? Ensure that the documentation answers the question then, when would folks use the EFI Firmware Volume Protocol? Why can't Linux support it? Why and would u
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
On Thu, 2018-05-03 at 22:23 +, Luis R. Rodriguez wrote: > On Tue, May 01, 2018 at 03:27:27PM -0400, Mimi Zohar wrote: > > On Tue, 2018-05-01 at 21:11 +0200, Hans de Goede wrote: > > > Only the pre hook? I believe the post-hook should still be called too, > > > right? So that we've hashes of all loaded firmwares in the IMA core. > > > > Good catch! Right, if IMA-measurement is enabled, then we would want > > to add the measurement. > > Mimi, just a heads up, we only use the post hook for the syfs fallback > mechanism, ie, we don't even use the post hook for direct fs lookup. > Do we want that there? The direct fs lookup calls kernel_read_file_from_path(), which calls the security_kernel_read_file() and security_kernel_post_read_file() hooks. So there is no need to add a direct call to either of these security calls. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
On Thu, May 3, 2018 at 3:31 PM Luis R. Rodriguez wrote: > On Wed, May 02, 2018 at 04:49:53PM +0200, Hans de Goede wrote: > > Hi, > > > > On 05/01/2018 09:29 PM, Andy Lutomirski wrote: > > > On Sun, Apr 29, 2018 at 2:36 AM Hans de Goede wrote: > > > > +The EFI embedded-fw code works by scanning all EFI_BOOT_SERVICES_CODE > > > memory > > > > +segments for an eight byte sequence matching prefix, if the prefix is > > > found it > > > > +then does a crc32 over length bytes and if that matches makes a copy of > > > length > > > > +bytes and adds that to its list with found firmwares. > > > > + > > > > > > Eww, gross. Is there really no better way to do this? > > > > I'm afraid not. > > > > > Is the issue that > > > the EFI code does not intend to pass the firmware to the OS but that it has > > > a copy for its own purposes and that Linux is just going to hijack EFI's > > > copy? If so, that's brilliant and terrible at the same time. > > > > Yes that is exactly the issue / what it happening here. > > > > > > > > > + for (i = 0; i < size; i += 8) { > > > > + if (*((u64 *)(mem + i)) != *((u64 *)desc->prefix)) > > > > + continue; > > > > + > > > > + /* Seed with ~0, invert to match crc32 userspace utility > > > */ > > > > + crc = ~crc32(~0, mem + i, desc->length); > > > > + if (crc == desc->crc) > > > > + break; > > > > + } > > > > > > I hate to play the security card, but this stinks a bit. The kernel > > > obviously needs to trust the EFI boot services code since the EFI boot > > > services code is free to modify the kernel image. But your patch is not > > > actually getting this firmware blob from the boot services code via any > > > defined interface -- you're literally snarfing up the blob from a range of > > > memory. I fully expect there to be any number of ways for untrustworthy > > > entities to inject malicious blobs into this memory range on quite a few > > > implementations. For example, there are probably unauthenticated EFI > > > variables and even parts of USB sticks and such that get read into boot > > > services memory, and I see no reason at all to expect that nothing in the > > > so-called "boot services code" range is actually just plain old boot > > > services *heap*. > > > > > > Fortunately, given your design, this is very easy to fix. Just replace > > > CRC32 with SHA-256 or similar. If you find the crypto api too ugly for > > > this purpose, I have patches that only need a small amount of dusting off > > > to give an entirely reasonable SHA-256 API in the kernel. > > > > My main reason for going with crc32 is that the scanning happens before > > the kernel is fully up and running (it happens just before the rest_init() > > call in start_kernel() (from init/main.c) I'm open to using the > > crypto api, but I was not sure if that is ready for use at that time. > Not being sure is different than being certain. As Andy noted, if that does > not work please poke Andy about the SHA-256 API he has which would enable > its use in kernel. Nah, don't use the cryptoapi for this. You'll probably regret it for any number of reasons. My code is here: https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=crypto/sha256_bpf&id=e9e12f056f2abed50a30b762db9185799f5864e6 and its two parents. It needs a little bit of dusting and it needs checking that all combinations of modular and non-modular builds work. Ard probably has further comments. > Right now this is just a crazy hack for *2* drivers. Its a lot of hacks for > just that, so no need to rush this in just yet. It seems unclear if we're > all happy with this yet as well. Fair enough. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
On Wed, May 02, 2018 at 04:49:53PM +0200, Hans de Goede wrote: > Hi, > > On 05/01/2018 09:29 PM, Andy Lutomirski wrote: > > On Sun, Apr 29, 2018 at 2:36 AM Hans de Goede wrote: > > > +The EFI embedded-fw code works by scanning all EFI_BOOT_SERVICES_CODE > > memory > > > +segments for an eight byte sequence matching prefix, if the prefix is > > found it > > > +then does a crc32 over length bytes and if that matches makes a copy of > > length > > > +bytes and adds that to its list with found firmwares. > > > + > > > > Eww, gross. Is there really no better way to do this? > > I'm afraid not. > > > Is the issue that > > the EFI code does not intend to pass the firmware to the OS but that it has > > a copy for its own purposes and that Linux is just going to hijack EFI's > > copy? If so, that's brilliant and terrible at the same time. > > Yes that is exactly the issue / what it happening here. > > > > > > + for (i = 0; i < size; i += 8) { > > > + if (*((u64 *)(mem + i)) != *((u64 *)desc->prefix)) > > > + continue; > > > + > > > + /* Seed with ~0, invert to match crc32 userspace utility > > */ > > > + crc = ~crc32(~0, mem + i, desc->length); > > > + if (crc == desc->crc) > > > + break; > > > + } > > > > I hate to play the security card, but this stinks a bit. The kernel > > obviously needs to trust the EFI boot services code since the EFI boot > > services code is free to modify the kernel image. But your patch is not > > actually getting this firmware blob from the boot services code via any > > defined interface -- you're literally snarfing up the blob from a range of > > memory. I fully expect there to be any number of ways for untrustworthy > > entities to inject malicious blobs into this memory range on quite a few > > implementations. For example, there are probably unauthenticated EFI > > variables and even parts of USB sticks and such that get read into boot > > services memory, and I see no reason at all to expect that nothing in the > > so-called "boot services code" range is actually just plain old boot > > services *heap*. > > > > Fortunately, given your design, this is very easy to fix. Just replace > > CRC32 with SHA-256 or similar. If you find the crypto api too ugly for > > this purpose, I have patches that only need a small amount of dusting off > > to give an entirely reasonable SHA-256 API in the kernel. > > My main reason for going with crc32 is that the scanning happens before > the kernel is fully up and running (it happens just before the rest_init() > call in start_kernel() (from init/main.c) I'm open to using the > crypto api, but I was not sure if that is ready for use at that time. Not being sure is different than being certain. As Andy noted, if that does not work please poke Andy about the SHA-256 API he has which would enable its use in kernel. Right now this is just a crazy hack for *2* drivers. Its a lot of hacks for just that, so no need to rush this in just yet. It seems unclear if we're all happy with this yet as well. Luis > > (To be clear, I don't love my own suggestion here. What I'd *really* like > > to see is a better interface and no attempt to match the data to some > > built-in hash at all. In particular, there are plenty of devices for which > > the driver wants access to a genuinely device-specific blob. For example, > > I'm typing this email while connected to a router that is running ath10k > > and is using a calibration blob awkwardly snarfed out of flash somewhere. > > It would be really nice if there was a way to pull a blob out of EFI space > > that is marked, by EFI, as belonging to a particular device. Then the > > firmware could just pass it over without any particular verification. But > > since your code is literally scanning a wide swath of physical memory for > > something that looks like a valid blob, I think you need to use a > > cryptographically strong concept of validity.) > > Yes ideally this would not be needed at all and/or use a well defined > interface, but alas we don't live in an ideal world :) > > Regards, > > Hans > -- Do not panic -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
On Tue, May 01, 2018 at 03:27:27PM -0400, Mimi Zohar wrote: > On Tue, 2018-05-01 at 21:11 +0200, Hans de Goede wrote: > > Only the pre hook? I believe the post-hook should still be called too, > > right? So that we've hashes of all loaded firmwares in the IMA core. > > Good catch! Right, if IMA-measurement is enabled, then we would want > to add the measurement. Mimi, just a heads up, we only use the post hook for the syfs fallback mechanism, ie, we don't even use the post hook for direct fs lookup. Do we want that there? Luis -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
Hi, On 05/01/2018 09:29 PM, Andy Lutomirski wrote: On Sun, Apr 29, 2018 at 2:36 AM Hans de Goede wrote: +The EFI embedded-fw code works by scanning all EFI_BOOT_SERVICES_CODE memory +segments for an eight byte sequence matching prefix, if the prefix is found it +then does a crc32 over length bytes and if that matches makes a copy of length +bytes and adds that to its list with found firmwares. + Eww, gross. Is there really no better way to do this? I'm afraid not. Is the issue that the EFI code does not intend to pass the firmware to the OS but that it has a copy for its own purposes and that Linux is just going to hijack EFI's copy? If so, that's brilliant and terrible at the same time. Yes that is exactly the issue / what it happening here. + for (i = 0; i < size; i += 8) { + if (*((u64 *)(mem + i)) != *((u64 *)desc->prefix)) + continue; + + /* Seed with ~0, invert to match crc32 userspace utility */ + crc = ~crc32(~0, mem + i, desc->length); + if (crc == desc->crc) + break; + } I hate to play the security card, but this stinks a bit. The kernel obviously needs to trust the EFI boot services code since the EFI boot services code is free to modify the kernel image. But your patch is not actually getting this firmware blob from the boot services code via any defined interface -- you're literally snarfing up the blob from a range of memory. I fully expect there to be any number of ways for untrustworthy entities to inject malicious blobs into this memory range on quite a few implementations. For example, there are probably unauthenticated EFI variables and even parts of USB sticks and such that get read into boot services memory, and I see no reason at all to expect that nothing in the so-called "boot services code" range is actually just plain old boot services *heap*. Fortunately, given your design, this is very easy to fix. Just replace CRC32 with SHA-256 or similar. If you find the crypto api too ugly for this purpose, I have patches that only need a small amount of dusting off to give an entirely reasonable SHA-256 API in the kernel. My main reason for going with crc32 is that the scanning happens before the kernel is fully up and running (it happens just before the rest_init() call in start_kernel() (from init/main.c) I'm open to using the crypto api, but I was not sure if that is ready for use at that time. (To be clear, I don't love my own suggestion here. What I'd *really* like to see is a better interface and no attempt to match the data to some built-in hash at all. In particular, there are plenty of devices for which the driver wants access to a genuinely device-specific blob. For example, I'm typing this email while connected to a router that is running ath10k and is using a calibration blob awkwardly snarfed out of flash somewhere. It would be really nice if there was a way to pull a blob out of EFI space that is marked, by EFI, as belonging to a particular device. Then the firmware could just pass it over without any particular verification. But since your code is literally scanning a wide swath of physical memory for something that looks like a valid blob, I think you need to use a cryptographically strong concept of validity.) Yes ideally this would not be needed at all and/or use a well defined interface, but alas we don't live in an ideal world :) Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
On Tue, May 01, 2018 at 07:29:19PM +, Andy Lutomirski wrote: > On Sun, Apr 29, 2018 at 2:36 AM Hans de Goede wrote: > > + for (i = 0; i < size; i += 8) { > > + if (*((u64 *)(mem + i)) != *((u64 *)desc->prefix)) > > + continue; > > + > > + /* Seed with ~0, invert to match crc32 userspace utility > */ > > + crc = ~crc32(~0, mem + i, desc->length); > > + if (crc == desc->crc) > > + break; > > + } > > I hate to play the security card, but this stinks a bit. The kernel > obviously needs to trust the EFI boot services code since the EFI boot > services code is free to modify the kernel image. But your patch is not > actually getting this firmware blob from the boot services code via any > defined interface -- you're literally snarfing up the blob from a range of > memory. I fully expect there to be any number of ways for untrustworthy > entities to inject malicious blobs into this memory range on quite a few > implementations. [snip] > It would be really nice if there was a way to pull a blob out of EFI space > that is marked, by EFI, as belonging to a particular device. Upthread I suggested to read the firmware from the EFI Firmware Volume. >From my point of view that would fulfill your requirements: https://lkml.org/lkml/2018/4/3/568 In the case of Hans' HID device, the firmware is embedded as a binary array in the EFI driver for the device. So the kernel would read the driver from the Firmware Volume, but it would still have to extract the firmware from the driver, either by scanning for the 8 byte magic or by hardcoding the offset and length in the kernel. An attacker would have to modify the Firmware Volume as opposed to just modifying a particular space in memory. My suggestion was dismissed by Hans and Peter Jones on the grounds of causing additional work without perceived benefit, given that a working (albeit admitted to be hackish) solution exists. Moreover Ard criticized that the EFI Firmware Volume Protocol is not part of the UEFI spec. I agree with you completely BTW. Thanks, Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
On Sun, Apr 29, 2018 at 2:36 AM Hans de Goede wrote: > +The EFI embedded-fw code works by scanning all EFI_BOOT_SERVICES_CODE memory > +segments for an eight byte sequence matching prefix, if the prefix is found it > +then does a crc32 over length bytes and if that matches makes a copy of length > +bytes and adds that to its list with found firmwares. > + Eww, gross. Is there really no better way to do this? Is the issue that the EFI code does not intend to pass the firmware to the OS but that it has a copy for its own purposes and that Linux is just going to hijack EFI's copy? If so, that's brilliant and terrible at the same time. > + for (i = 0; i < size; i += 8) { > + if (*((u64 *)(mem + i)) != *((u64 *)desc->prefix)) > + continue; > + > + /* Seed with ~0, invert to match crc32 userspace utility */ > + crc = ~crc32(~0, mem + i, desc->length); > + if (crc == desc->crc) > + break; > + } I hate to play the security card, but this stinks a bit. The kernel obviously needs to trust the EFI boot services code since the EFI boot services code is free to modify the kernel image. But your patch is not actually getting this firmware blob from the boot services code via any defined interface -- you're literally snarfing up the blob from a range of memory. I fully expect there to be any number of ways for untrustworthy entities to inject malicious blobs into this memory range on quite a few implementations. For example, there are probably unauthenticated EFI variables and even parts of USB sticks and such that get read into boot services memory, and I see no reason at all to expect that nothing in the so-called "boot services code" range is actually just plain old boot services *heap*. Fortunately, given your design, this is very easy to fix. Just replace CRC32 with SHA-256 or similar. If you find the crypto api too ugly for this purpose, I have patches that only need a small amount of dusting off to give an entirely reasonable SHA-256 API in the kernel. (To be clear, I don't love my own suggestion here. What I'd *really* like to see is a better interface and no attempt to match the data to some built-in hash at all. In particular, there are plenty of devices for which the driver wants access to a genuinely device-specific blob. For example, I'm typing this email while connected to a router that is running ath10k and is using a calibration blob awkwardly snarfed out of flash somewhere. It would be really nice if there was a way to pull a blob out of EFI space that is marked, by EFI, as belonging to a particular device. Then the firmware could just pass it over without any particular verification. But since your code is literally scanning a wide swath of physical memory for something that looks like a valid blob, I think you need to use a cryptographically strong concept of validity.) -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
On Tue, 2018-05-01 at 21:11 +0200, Hans de Goede wrote: > Hi, > > On 01-05-18 16:36, Mimi Zohar wrote: > > [Cc'ing linux-security] > > > > On Sun, 2018-04-29 at 11:35 +0200, Hans de Goede wrote: > > [...] > >> diff --git a/drivers/base/firmware_loader/fallback_efi.c > >> b/drivers/base/firmware_loader/fallback_efi.c > >> new file mode 100644 > >> index ..82ba82f48a79 > >> --- /dev/null > >> +++ b/drivers/base/firmware_loader/fallback_efi.c > >> @@ -0,0 +1,51 @@ > >> +// SPDX-License-Identifier: GPL-2.0 > >> + > >> +#include > >> +#include > >> +#include > >> +#include > >> + > >> +#include "fallback.h" > >> +#include "firmware.h" > >> + > >> +int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv, > >> + enum fw_opt *opt_flags, int ret) > >> +{ > >> + enum kernel_read_file_id id = READING_FIRMWARE; > > > > Please define a new kernel_read_file_id for this (eg. > > READING_FIRMWARE_EFI_EMBEDDED). > > Are you sure, I wonder how useful it is to add a new > kernel_read_file_id every time a new way to get firmware > comes up? > > I especially wonder about the sense in adding a new id > given that the quite old FIRMWARE_PREALLOC_BUFFER is > still not supported / checked properly by the security code. I posted patches earlier today[1], which address this. Patch 5/6 just makes it equivalent to READING_FIRMWARE. Patch 6/6 questions whether the device has access to the pre-allocated buffer *before* the signature has been verified. [1] kernsec.org/pipermail/linux-security-module-archive/2018-May/006639.html > > Anyways I can add a new id if you want me to, what about > when fw_get_efi_embedded_fw is reading into a driver allocated > buffer, do you want a separate EADING_FIRMWARE_EFI_EMBEDDED_PREALLOC_BUFFER > for that ? Without the kernel being able to verify the firmware's signature, I'm not sure it makes much of a difference. > > > > >> + size_t size, max = INT_MAX; > >> + int rc; > >> + > >> + if (!dev) > >> + return ret; > >> + > >> + if (!device_property_read_bool(dev, "efi-embedded-firmware")) > >> + return ret; > > > > Instead of calling security_kernel_post_read_file(), either in > > device_property_read_bool() or here call security_kernel_read_file(). > > > > The pre read call is for deciding whether to allow this call > > independent of the firmware being loaded, whereas the post security > > call is currently being used by IMA-appraisal for verifying a > > signature. There might be other LSMs using the post hook as well. As > > there is no kernel signature associated with this firmware, use the > > security pre read_file hook. > > Only the pre hook? I believe the post-hook should still be called too, > right? So that we've hashes of all loaded firmwares in the IMA core. Good catch! Right, if IMA-measurement is enabled, then we would want to add the measurement. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
Hi, On 01-05-18 16:36, Mimi Zohar wrote: [Cc'ing linux-security] On Sun, 2018-04-29 at 11:35 +0200, Hans de Goede wrote: [...] diff --git a/drivers/base/firmware_loader/fallback_efi.c b/drivers/base/firmware_loader/fallback_efi.c new file mode 100644 index ..82ba82f48a79 --- /dev/null +++ b/drivers/base/firmware_loader/fallback_efi.c @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include +#include +#include +#include + +#include "fallback.h" +#include "firmware.h" + +int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv, + enum fw_opt *opt_flags, int ret) +{ + enum kernel_read_file_id id = READING_FIRMWARE; Please define a new kernel_read_file_id for this (eg. READING_FIRMWARE_EFI_EMBEDDED). Are you sure, I wonder how useful it is to add a new kernel_read_file_id every time a new way to get firmware comes up? I especially wonder about the sense in adding a new id given that the quite old FIRMWARE_PREALLOC_BUFFER is still not supported / checked properly by the security code. Anyways I can add a new id if you want me to, what about when fw_get_efi_embedded_fw is reading into a driver allocated buffer, do you want a separate EADING_FIRMWARE_EFI_EMBEDDED_PREALLOC_BUFFER for that ? + size_t size, max = INT_MAX; + int rc; + + if (!dev) + return ret; + + if (!device_property_read_bool(dev, "efi-embedded-firmware")) + return ret; Instead of calling security_kernel_post_read_file(), either in device_property_read_bool() or here call security_kernel_read_file(). The pre read call is for deciding whether to allow this call independent of the firmware being loaded, whereas the post security call is currently being used by IMA-appraisal for verifying a signature. There might be other LSMs using the post hook as well. As there is no kernel signature associated with this firmware, use the security pre read_file hook. Only the pre hook? I believe the post-hook should still be called too, right? So that we've hashes of all loaded firmwares in the IMA core. Regards, Hans thanks, Mimi + + *opt_flags |= FW_OPT_NO_WARN | FW_OPT_NOCACHE | FW_OPT_NOFALLBACK; + + /* Already populated data member means we're loading into a buffer */ + if (fw_priv->data) { + id = READING_FIRMWARE_PREALLOC_BUFFER; + max = fw_priv->allocated_size; + } + + rc = efi_get_embedded_fw(fw_priv->fw_name, &fw_priv->data, &size, max); + if (rc) { + dev_warn(dev, "Firmware %s not in EFI\n", fw_priv->fw_name); + return ret; + } + + rc = security_kernel_post_read_file(NULL, fw_priv->data, size, id); + if (rc) { + if (id != READING_FIRMWARE_PREALLOC_BUFFER) { + vfree(fw_priv->data); + fw_priv->data = NULL; + } + return rc; + } + + dev_dbg(dev, "using efi-embedded fw %s\n", fw_priv->fw_name); + fw_priv->size = size; + fw_state_done(fw_priv); + return 0; +} -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
[Cc'ing linux-security] On Sun, 2018-04-29 at 11:35 +0200, Hans de Goede wrote: [...] > diff --git a/drivers/base/firmware_loader/fallback_efi.c > b/drivers/base/firmware_loader/fallback_efi.c > new file mode 100644 > index ..82ba82f48a79 > --- /dev/null > +++ b/drivers/base/firmware_loader/fallback_efi.c > @@ -0,0 +1,51 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +#include > +#include > +#include > +#include > + > +#include "fallback.h" > +#include "firmware.h" > + > +int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv, > +enum fw_opt *opt_flags, int ret) > +{ > + enum kernel_read_file_id id = READING_FIRMWARE; Please define a new kernel_read_file_id for this (eg. READING_FIRMWARE_EFI_EMBEDDED). > + size_t size, max = INT_MAX; > + int rc; > + > + if (!dev) > + return ret; > + > + if (!device_property_read_bool(dev, "efi-embedded-firmware")) > + return ret; Instead of calling security_kernel_post_read_file(), either in device_property_read_bool() or here call security_kernel_read_file(). The pre read call is for deciding whether to allow this call independent of the firmware being loaded, whereas the post security call is currently being used by IMA-appraisal for verifying a signature. There might be other LSMs using the post hook as well. As there is no kernel signature associated with this firmware, use the security pre read_file hook. thanks, Mimi > + > + *opt_flags |= FW_OPT_NO_WARN | FW_OPT_NOCACHE | FW_OPT_NOFALLBACK; > + > + /* Already populated data member means we're loading into a buffer */ > + if (fw_priv->data) { > + id = READING_FIRMWARE_PREALLOC_BUFFER; > + max = fw_priv->allocated_size; > + } > + > + rc = efi_get_embedded_fw(fw_priv->fw_name, &fw_priv->data, &size, max); > + if (rc) { > + dev_warn(dev, "Firmware %s not in EFI\n", fw_priv->fw_name); > + return ret; > + } > + > + rc = security_kernel_post_read_file(NULL, fw_priv->data, size, id); > + if (rc) { > + if (id != READING_FIRMWARE_PREALLOC_BUFFER) { > + vfree(fw_priv->data); > + fw_priv->data = NULL; > + } > + return rc; > + } > + > + dev_dbg(dev, "using efi-embedded fw %s\n", fw_priv->fw_name); > + fw_priv->size = size; > + fw_state_done(fw_priv); > + return 0; > +} -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html