Re: [PATCH 17/17] efi/libstub/arm64: handle randomized TEXT_OFFSET
On 14 May 2018 at 08:47, Ingo Molnar wrote:
>
> * Ard Biesheuvel wrote:
>
>> From: Mark Rutland
>>
>> When CONFIG_RANDOMIZE_TEXT_OFFSET is selected, TEXT_OFFSET is an
>> arbitrary multiple of PAGE_SIZE in the interval [0, 2MB).
>>
>> The EFI stub does not account for the potential misalignment of
>> TEXT_OFFSET relative to EFI_KIMG_ALIGN, and produces a randomized
>> physical offset which is always a round multiple of EFI_KIMG_ALIGN.
>> This may result in statically allocated objects whose alignment exceeds
>> PAGE_SIZE to appear misaligned in memory. This has been observed to
>> result in spurious stack overflow reports and failure to make use of
>> the IRQ stacks, and theoretically could result in a number of other
>> issues.
>>
>> We can OR in the low bits of TEXT_OFFSET to ensure that we have the
>> necessary offset (and hence preserve the misalignment of TEXT_OFFSET
>> relative to EFI_KIMG_ALIGN), so let's do that.
>>
>> Fixes: 6f26b3671184c36d ("arm64: kaslr: increase randomization granularity")
>> Cc: # v4.7+
>> Reported-by: Kim Phillips
>> Signed-off-by: Mark Rutland
>> Tested-by: Kim Phillips
>> [ardb: clarify commit log]
>> Signed-off-by: Ard Biesheuvel
>> ---
>> drivers/firmware/efi/libstub/arm64-stub.c | 7 +++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/firmware/efi/libstub/arm64-stub.c
>> b/drivers/firmware/efi/libstub/arm64-stub.c
>> index b9bd827caa22..541b82fdc8a2 100644
>> --- a/drivers/firmware/efi/libstub/arm64-stub.c
>> +++ b/drivers/firmware/efi/libstub/arm64-stub.c
>> @@ -97,6 +97,13 @@ efi_status_t handle_kernel_image(efi_system_table_t
>> *sys_table_arg,
>> u32 offset = !IS_ENABLED(CONFIG_DEBUG_ALIGN_RODATA) ?
>>(phys_seed >> 32) & mask : TEXT_OFFSET;
>>
>> + /*
>> + * With CONFIG_RANDOMIZE_TEXT_OFFSET, TEXT_OFFSET may not be a
>> + * multiple of EFI_KIMG_ALIGN, and we must ensure that we apply
>> + * the offset below EFI_KIMG_ALIGN.
>> + */
>
> When referring to config variables in comments and changelogs I'd suggest a
> bit
> more verbosity:
>
> s/CONFIG_RANDOMIZE_TEXT_OFFSET
>/CONFIG_RANDOMIZE_TEXT_OFFSET=y
>
> ... because at first I thought (based on the name) that
> CONFIG_RANDOMIZE_TEXT_OFFSET is an actual integer offset value - while it's a
> bool. The =y makes the bool nature obvious.
>
> ( Similarly, when negated the canonical way to refer to it is
> !CONFIG_RANDOMIZE_TEXT_OFFSET. )
>
Fair enough.
>> + offset |= (TEXT_OFFSET % EFI_KIMG_ALIGN);
>
> The parentheses are not needed here I think.
>
Nope.
Will you fix this up when applying? Or should I resend?
--
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] efi/x86: Clean up the eboot code a bit
On 14 May 2018 at 08:43, Ingo Molnar wrote:
>
> So I looked at arch/x86/boot/compressed/eboot.c to improve a printk message
> and
> ended up with the cleanups below.
>
> Only build tested.
>
> Thanks,
>
> Ingo
>
> =>
> Subject: efi/x86: Clean up the eboot code
> From: Ingo Molnar
> Date: Mon May 14 08:33:40 CEST 2018
>
> Various small cleanups:
>
> - Standardize printk messages:
>
> 'alloc' => 'allocate'
> 'mem' => 'memory'
>
>also put variable names in printk messages between quotes.
>
> - Align mass-assignments vertically for better readability
>
> - Break multi-line function prototypes at the name where possible,
>not in the middle of the parameter list
>
> - Use a newline before return statements consistently.
>
> - Use curly braces in a balanced fashion.
>
> - Remove stray newlines.
>
> No change in functionality.
>
> Cc: Ard Biesheuvel
> Cc: Linus Torvalds
> Cc: Matt Fleming
> Cc: Peter Zijlstra
> Cc: Thomas Gleixner
> Cc: linux-efi@vger.kernel.org
> Signed-off-by: Ingo Molnar
Thanks Ingo
Reviewed-by: Ard Biesheuvel
> ---
> arch/x86/boot/compressed/eboot.c | 247
> +++
> 1 file changed, 126 insertions(+), 121 deletions(-)
>
> Index: tip/arch/x86/boot/compressed/eboot.c
> ===
> --- tip.orig/arch/x86/boot/compressed/eboot.c
> +++ tip/arch/x86/boot/compressed/eboot.c
> @@ -34,9 +34,9 @@ static void setup_boot_services##bits(st
> \
> table = (typeof(table))sys_table; \
> \
> - c->runtime_services = table->runtime; \
> - c->boot_services = table->boottime; \
> - c->text_output = table->con_out;\
> + c->runtime_services = table->runtime; \
> + c->boot_services= table->boottime; \
> + c->text_output = table->con_out; \
> }
> BOOT_SERVICES(32);
> BOOT_SERVICES(64);
> @@ -64,6 +64,7 @@ static inline efi_status_t __open_volume
> efi_printk(sys_table, "Failed to open volume\n");
>
> *__fh = fh;
> +
> return status;
> }
>
> @@ -90,6 +91,7 @@ static inline efi_status_t __open_volume
> efi_printk(sys_table, "Failed to open volume\n");
>
> *__fh = fh;
> +
> return status;
> }
>
> @@ -140,16 +142,16 @@ __setup_efi_pci(efi_pci_io_protocol_t *p
>
> status = efi_call_early(allocate_pool, EFI_LOADER_DATA, size, &rom);
> if (status != EFI_SUCCESS) {
> - efi_printk(sys_table, "Failed to alloc mem for rom\n");
> + efi_printk(sys_table, "Failed to allocate memory for
> 'rom'\n");
> return status;
> }
>
> memset(rom, 0, sizeof(*rom));
>
> - rom->data.type = SETUP_PCI;
> - rom->data.len = size - sizeof(struct setup_data);
> - rom->data.next = 0;
> - rom->pcilen = pci->romsize;
> + rom->data.type = SETUP_PCI;
> + rom->data.len = size - sizeof(struct setup_data);
> + rom->data.next = 0;
> + rom->pcilen = pci->romsize;
> *__rom = rom;
>
> status = efi_call_proto(efi_pci_io_protocol, pci.read, pci,
> @@ -186,8 +188,7 @@ free_struct:
> }
>
> static void
> -setup_efi_pci32(struct boot_params *params, void **pci_handle,
> - unsigned long size)
> +setup_efi_pci32(struct boot_params *params, void **pci_handle, unsigned long
> size)
> {
> efi_pci_io_protocol_t *pci = NULL;
> efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID;
> @@ -226,13 +227,11 @@ setup_efi_pci32(struct boot_params *para
> params->hdr.setup_data = (unsigned long)rom;
>
> data = (struct setup_data *)rom;
> -
> }
> }
>
> static void
> -setup_efi_pci64(struct boot_params *params, void **pci_handle,
> - unsigned long size)
> +setup_efi_pci64(struct boot_params *params, void **pci_handle, unsigned long
> size)
> {
> efi_pci_io_protocol_t *pci = NULL;
> efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID;
> @@ -271,7 +270,6 @@ setup_efi_pci64(struct boot_params *para
> params->hdr.setup_data = (unsigned long)rom;
>
> data = (struct setup_data *)rom;
> -
> }
> }
>
> @@ -301,7 +299,7 @@ static void setup_efi_pci(struct boot_pa
> size, (void **)&pci_handle);
>
> if (status != EFI_SUCCESS) {
> - efi_printk(sys_table, "Failed to alloc mem for
> pci_handle\n");
> + efi_printk(sys_table, "Failed to allocate memory for
> 'pci_handle'\n");
> re
Re: [PATCH 17/17] efi/libstub/arm64: handle randomized TEXT_OFFSET
* Ard Biesheuvel wrote:
> From: Mark Rutland
>
> When CONFIG_RANDOMIZE_TEXT_OFFSET is selected, TEXT_OFFSET is an
> arbitrary multiple of PAGE_SIZE in the interval [0, 2MB).
>
> The EFI stub does not account for the potential misalignment of
> TEXT_OFFSET relative to EFI_KIMG_ALIGN, and produces a randomized
> physical offset which is always a round multiple of EFI_KIMG_ALIGN.
> This may result in statically allocated objects whose alignment exceeds
> PAGE_SIZE to appear misaligned in memory. This has been observed to
> result in spurious stack overflow reports and failure to make use of
> the IRQ stacks, and theoretically could result in a number of other
> issues.
>
> We can OR in the low bits of TEXT_OFFSET to ensure that we have the
> necessary offset (and hence preserve the misalignment of TEXT_OFFSET
> relative to EFI_KIMG_ALIGN), so let's do that.
>
> Fixes: 6f26b3671184c36d ("arm64: kaslr: increase randomization granularity")
> Cc: # v4.7+
> Reported-by: Kim Phillips
> Signed-off-by: Mark Rutland
> Tested-by: Kim Phillips
> [ardb: clarify commit log]
> Signed-off-by: Ard Biesheuvel
> ---
> drivers/firmware/efi/libstub/arm64-stub.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/firmware/efi/libstub/arm64-stub.c
> b/drivers/firmware/efi/libstub/arm64-stub.c
> index b9bd827caa22..541b82fdc8a2 100644
> --- a/drivers/firmware/efi/libstub/arm64-stub.c
> +++ b/drivers/firmware/efi/libstub/arm64-stub.c
> @@ -97,6 +97,13 @@ efi_status_t handle_kernel_image(efi_system_table_t
> *sys_table_arg,
> u32 offset = !IS_ENABLED(CONFIG_DEBUG_ALIGN_RODATA) ?
>(phys_seed >> 32) & mask : TEXT_OFFSET;
>
> + /*
> + * With CONFIG_RANDOMIZE_TEXT_OFFSET, TEXT_OFFSET may not be a
> + * multiple of EFI_KIMG_ALIGN, and we must ensure that we apply
> + * the offset below EFI_KIMG_ALIGN.
> + */
When referring to config variables in comments and changelogs I'd suggest a bit
more verbosity:
s/CONFIG_RANDOMIZE_TEXT_OFFSET
/CONFIG_RANDOMIZE_TEXT_OFFSET=y
... because at first I thought (based on the name) that
CONFIG_RANDOMIZE_TEXT_OFFSET is an actual integer offset value - while it's a
bool. The =y makes the bool nature obvious.
( Similarly, when negated the canonical way to refer to it is
!CONFIG_RANDOMIZE_TEXT_OFFSET. )
> + offset |= (TEXT_OFFSET % EFI_KIMG_ALIGN);
The parentheses are not needed here I think.
Thanks,
Ingo
--
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
[PATCH] efi/x86: Clean up the eboot code a bit
So I looked at arch/x86/boot/compressed/eboot.c to improve a printk message and
ended up with the cleanups below.
Only build tested.
Thanks,
Ingo
=>
Subject: efi/x86: Clean up the eboot code
From: Ingo Molnar
Date: Mon May 14 08:33:40 CEST 2018
Various small cleanups:
- Standardize printk messages:
'alloc' => 'allocate'
'mem' => 'memory'
also put variable names in printk messages between quotes.
- Align mass-assignments vertically for better readability
- Break multi-line function prototypes at the name where possible,
not in the middle of the parameter list
- Use a newline before return statements consistently.
- Use curly braces in a balanced fashion.
- Remove stray newlines.
No change in functionality.
Cc: Ard Biesheuvel
Cc: Linus Torvalds
Cc: Matt Fleming
Cc: Peter Zijlstra
Cc: Thomas Gleixner
Cc: linux-efi@vger.kernel.org
Signed-off-by: Ingo Molnar
---
arch/x86/boot/compressed/eboot.c | 247 +++
1 file changed, 126 insertions(+), 121 deletions(-)
Index: tip/arch/x86/boot/compressed/eboot.c
===
--- tip.orig/arch/x86/boot/compressed/eboot.c
+++ tip/arch/x86/boot/compressed/eboot.c
@@ -34,9 +34,9 @@ static void setup_boot_services##bits(st
\
table = (typeof(table))sys_table; \
\
- c->runtime_services = table->runtime; \
- c->boot_services = table->boottime; \
- c->text_output = table->con_out;\
+ c->runtime_services = table->runtime; \
+ c->boot_services= table->boottime; \
+ c->text_output = table->con_out; \
}
BOOT_SERVICES(32);
BOOT_SERVICES(64);
@@ -64,6 +64,7 @@ static inline efi_status_t __open_volume
efi_printk(sys_table, "Failed to open volume\n");
*__fh = fh;
+
return status;
}
@@ -90,6 +91,7 @@ static inline efi_status_t __open_volume
efi_printk(sys_table, "Failed to open volume\n");
*__fh = fh;
+
return status;
}
@@ -140,16 +142,16 @@ __setup_efi_pci(efi_pci_io_protocol_t *p
status = efi_call_early(allocate_pool, EFI_LOADER_DATA, size, &rom);
if (status != EFI_SUCCESS) {
- efi_printk(sys_table, "Failed to alloc mem for rom\n");
+ efi_printk(sys_table, "Failed to allocate memory for 'rom'\n");
return status;
}
memset(rom, 0, sizeof(*rom));
- rom->data.type = SETUP_PCI;
- rom->data.len = size - sizeof(struct setup_data);
- rom->data.next = 0;
- rom->pcilen = pci->romsize;
+ rom->data.type = SETUP_PCI;
+ rom->data.len = size - sizeof(struct setup_data);
+ rom->data.next = 0;
+ rom->pcilen = pci->romsize;
*__rom = rom;
status = efi_call_proto(efi_pci_io_protocol, pci.read, pci,
@@ -186,8 +188,7 @@ free_struct:
}
static void
-setup_efi_pci32(struct boot_params *params, void **pci_handle,
- unsigned long size)
+setup_efi_pci32(struct boot_params *params, void **pci_handle, unsigned long
size)
{
efi_pci_io_protocol_t *pci = NULL;
efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID;
@@ -226,13 +227,11 @@ setup_efi_pci32(struct boot_params *para
params->hdr.setup_data = (unsigned long)rom;
data = (struct setup_data *)rom;
-
}
}
static void
-setup_efi_pci64(struct boot_params *params, void **pci_handle,
- unsigned long size)
+setup_efi_pci64(struct boot_params *params, void **pci_handle, unsigned long
size)
{
efi_pci_io_protocol_t *pci = NULL;
efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID;
@@ -271,7 +270,6 @@ setup_efi_pci64(struct boot_params *para
params->hdr.setup_data = (unsigned long)rom;
data = (struct setup_data *)rom;
-
}
}
@@ -301,7 +299,7 @@ static void setup_efi_pci(struct boot_pa
size, (void **)&pci_handle);
if (status != EFI_SUCCESS) {
- efi_printk(sys_table, "Failed to alloc mem for
pci_handle\n");
+ efi_printk(sys_table, "Failed to allocate memory for
'pci_handle'\n");
return;
}
@@ -347,8 +345,7 @@ static void retrieve_apple_device_proper
status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
size + sizeof(struct setup_data), &new);
if (status != EFI_SUCCESS) {
- efi_printk(sys_table,
-
Re: [PATCH 15/17] efi/x86: Ignore unrealistically large option roms
* Ard Biesheuvel wrote: > + /* > + * Some firmwares contain EFI function pointers at the place where the > + * romimage and romsize fields are supposed to be. Typically the EFI > + * code is mapped at high addresses, translating to an unrealistically > + * large romsize. The UEFI spec limits the size of option ROMs to 16 > + * MiB so we reject any roms over 16 MiB in size to catch this. > + */ JFYI, I fixed this: s/Some firmwares contain /Some firmware images contain and: s/roms /ROMs (Looks good otherwise, no need to resend.) Thanks, Ingo -- 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/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(&
