Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Hans de Goede]

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 

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Hans de Goede]

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), 

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Ard Biesheuvel]

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 

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Hans de Goede]

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=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

[Hans de Goede]

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

[Hans de Goede]

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