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

[Hans de Goede]

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] efi/capsule-loader: Don't output reset log when header flags is not set

[Ard Biesheuvel]

On 2 May 2018 at 03:29, Yang, Shunyong  wrote:
> Hi, Ard,
>
> On Tue, 2018-05-01 at 11:54 +0200, Ard Biesheuvel wrote:
>> On 25 April 2018 at 05:10, Shunyong Yang > com> wrote:
>> >
>> > It means firmware attempts to immediately process or launch the
>> > capsule
>> > when flags in capsule header is not set. Moreover, reset is not
>> > needed
>> > in this case. Current code will output log to indicate reset.
>> >
>> > This patch adds a branch to avoid reset log output when the flags
>> > is not
>> > set.
>> >
>> > Cc: Joey Zheng 
>> > Signed-off-by: Shunyong Yang 
>> > ---
>> >  drivers/firmware/efi/capsule-loader.c | 13 +
>> >  1 file changed, 9 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/firmware/efi/capsule-loader.c
>> > b/drivers/firmware/efi/capsule-loader.c
>> > index e456f4602df1..a63b8e5bde23 100644
>> > --- a/drivers/firmware/efi/capsule-loader.c
>> > +++ b/drivers/firmware/efi/capsule-loader.c
>> > @@ -134,10 +134,15 @@ static ssize_t
>> > efi_capsule_submit_update(struct capsule_info *cap_info)
>> >
>> > /* Indicate capsule binary uploading is done */
>> > cap_info->index = NO_FURTHER_WRITE_ACTION;
>> > -   pr_info("Successfully upload capsule file with reboot type
>> > '%s'\n",
>> > -   !cap_info->reset_type ? "RESET_COLD" :
>> > -   cap_info->reset_type == 1 ? "RESET_WARM" :
>> > -   "RESET_SHUTDOWN");
>> > +
>> > +   if (cap_info->header.flags)
>> You should check for the relevant flag bits here, because the 16
>> lower
>> bits are platform dependent.
> In current three bits in flags, CAPSULE_FLAGS_POPULATE_SYSTEM_TABLE and
> CAPSULE_FLAGS_INITIATE_RESET are dependent on
> CAPSULE_FLAGS_PERSIST_ACROSS_RESET.
>
> So, maybe I only need to check CAPSULE_FLAGS_PERSIST_ACROSS_RESET?
>

Yes I think that's fine

>>
>> >
>> > +   pr_info("Successfully upload capsule file with
>> > reboot type '%s'\n",
>> > +   !cap_info->reset_type ? "RESET_COLD" :
>> > +   cap_info->reset_type == 1 ? "RESET_WARM" :
>> > +   "RESET_SHUTDOWN");
>> > +   else
>> > +   pr_info("Successfully upload, process and launch
>> > capsule file\n");
>> > +
>> > return 0;
>> >  }
>> >
>> > --
>> > 1.8.3.1
>> > --
> 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
--
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: Fix the size not consistent issue when unmapping memory map

[Lee, Chun-Yi]

When using kdump, SOMETIMES the "size not consistent" warning message
shows up when the crash kernel boots with early_ioremap_debug parameter:

WARNING: CPU: 0 PID: 0 at ../mm/early_ioremap.c:182 early_iounmap+0x4f/0x12c()
early_iounmap(ff200180, 0118) [0] size not consistent 0120

The root cause is that the unmapping size of memory map doesn't
match with the original size when mapping:

in __efi_memmap_init()
map.map = early_memremap(phys_map, data->size);

in efi_memmap_unmap()
size = efi.memmap.desc_size * efi.memmap.nr_map;
early_memunmap(efi.memmap.map, size);

But the efi.memmap.nr_map is from __efi_memmap_init(). The remainder
of size was discarded when calculating the nr_map:
map.nr_map = data->size / data->desc_size;

When the original size of memory map region does not equal to the
result of multiplication. The "size not consistent" warning
will be triggered.

This issue sometimes was hit by kdump because kexec set the efi map
size to align with 16 when loading crash kernel image:

in bzImage64_load()
efi_map_sz = efi_get_runtime_map_size();
efi_map_sz = ALIGN(efi_map_sz, 16);

Dave Young's a841aa83d patch fixed kexec issue. On UEFI side, this
patch changes the logic in the unmapping function. Using the end
address of map to calcuate original size.

Thank Randy Wright for his report and testing. And also thank
Takashi Iwai for his help to trace issue.

Cc: Ard Biesheuvel 
Cc: Takashi Iwai 
Cc: Vivek Goyal 
Cc: Ingo Molnar 
Tested-by: Randy Wright 
Signed-off-by: "Lee, Chun-Yi" 
---
 drivers/firmware/efi/memmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
index 5fc7052..1f592d8 100644
--- a/drivers/firmware/efi/memmap.c
+++ b/drivers/firmware/efi/memmap.c
@@ -121,7 +121,7 @@ void __init efi_memmap_unmap(void)
if (!efi.memmap.late) {
unsigned long size;
 
-   size = efi.memmap.desc_size * efi.memmap.nr_map;
+   size = efi.memmap.map_end - efi.memmap.map;
early_memunmap(efi.memmap.map, size);
} else {
memunmap(efi.memmap.map);
-- 
2.10.2

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