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

[Ard Biesheuvel]

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

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

[Ard Biesheuvel]

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 v3 2/5] efi: Add embedded peripheral firmware support

[Luis R. Rodriguez]

Android folks, poke below. otherwise we'll have no option but to seriously
consider Mimi's patch to prevent these calls when IMA appraisal is enforced:

http://lkml.kernel.org/r/1525182503-13849-7-git-send-email-zo...@linux.vnet.ibm.com

Please read below

On Wed, Apr 25, 2018 at 05:55:57PM +, Luis R. Rodriguez wrote:
> On Wed, Apr 25, 2018 at 01:00:09AM -0400, Mimi Zohar wrote:
> > On Tue, 2018-04-24 at 23:42 +, Luis R. Rodriguez wrote:
> > > On Tue, Apr 24, 2018 at 12:07:01PM -0400, Mimi Zohar wrote:
> > > > On Tue, 2018-04-24 at 17:09 +0200, Hans de Goede wrote:
> > > If its of any help --
> > > 
> > > drivers/soc/qcom/mdt_loader.c is the only driver currently using
> > > request_firmware_into_buf() however I'll note qcom_mdt_load() is used in 
> > > many
> > > other drivers so they are wrappers around request_firmware_into_buf():
> > > 
> > > drivers/gpu/drm/msm/adreno/a5xx_gpu.c:   * adreno_request_fw() handles 
> > > this, but qcom_mdt_load() does
> > > drivers/gpu/drm/msm/adreno/a5xx_gpu.c:  ret = qcom_mdt_load(dev, 
> > > fw, fwname, GPU_PAS_ID,
> > > drivers/gpu/drm/msm/adreno/a5xx_gpu.c:  ret = qcom_mdt_load(dev, 
> > > fw, newname, GPU_PAS_ID,
> > > drivers/media/platform/qcom/venus/firmware.c:   ret = qcom_mdt_load(dev, 
> > > mdt, fwname, VENUS_PAS_ID, mem_va, mem_phys,
> > > drivers/remoteproc/qcom_adsp_pil.c: return qcom_mdt_load(adsp->dev, 
> > > fw, rproc->firmware, adsp->pas_id,
> > > drivers/remoteproc/qcom_wcnss.c:return qcom_mdt_load(wcnss->dev, 
> > > fw, rproc->firmware, WCNSS_PAS_ID,
> > > 
> > > > > As such the current IMA code (from v4.17-rc2) actually does
> > > > > not handle READING_FIRMWARE_PREALLOC_BUFFER at all, 
> > > > 
> > > > Right, it doesn't yet address READING_FIRMWARE_PREALLOC_BUFFER, but
> > > > should.
> > > > 
> > > > Depending on whether the device requesting the firmware has access to
> > > > the DMA memory, before the signature verification, 
> > > 
> > > It would seem from the original patch review about 
> > > READING_FIRMWARE_PREALLOC_BUFFER
> > > that this is not a DMA buffer.

To be very clear I believe Stephen implied this was not DMA buffer. Mimi
asked for READING_FIRMWARE_DMA if it was:

https://patchwork.kernel.org/patch/9074611/

> > The call sequence:
> > qcom_mdt_load() -> qcom_scm_pas_init_image() -> dma_alloc_coherent()
> > 
> > If dma_alloc_coherent() isn't allocating a DMA buffer, then the
> > function name is misleading/confusing.
> 
> Hah, by *definition* the device *and* processor has immediate access
> to data written *immediately* when dma_alloc_coherent() is used. From
> Documentation/DMA-API.txt:
> 
> ---
> Part Ia - Using large DMA-coherent buffers
>   
> --
>   
>   
>   
> ::
>   
>   
>   
> void *
>   
> dma_alloc_coherent(struct device *dev, size_t size,   
>   
>dma_addr_t *dma_handle, gfp_t flag)
>   
>   
>   
> Consistent memory is memory for which a write by either the device or 
>   
> the processor can immediately be read by the processor or device  
>   
> without having to worry about caching effects.  (You may however need 
>   
> to make sure to flush the processor's write buffers before telling
>   
> devices to read that memory.)  
> 
> 
> Is ptr below
> 
>   ret = request_firmware_into_buf(_fw, fw_name, dev,  
>   ptr, phdr->p_filesz); 
> 
> Also part of the DMA buffer allocated earlier via:
> 
> ret = qcom_scm_pas_init_image(pas_id, fw->data, fw->size);
>   
> 
> Android folks?

Android folks?

> > > The device driver should have access to the buffer pointer with write 
> > > given
> > > that with request_firmware_into_buf() the driver is giving full write 
> > > access to
> > > the memory pointer so that the firmware API can stuff the firmware it 
> > > finds
> > > there.
> > > 
> > > Firmware signature verification would be up to the device hardware to do 
> > > upon
> > > load *after* request_firmware_into_buf().
> > 
> > We're discussing the kernel's signature verification, not the device
> > hardware's signature verification.  Can the device driver access the
> > buffer, before IMA-appraisal has verified the firmware's signature?
> 
> It will depend on the above question.

  Luis
--
To 

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

[Luis R. Rodriguez]

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

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

[Mimi Zohar]

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

[Andy Lutomirski]

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.


> 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

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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] efi: Fix the size not consistent issue when unmapping memory map

[Ard Biesheuvel]

On 2 May 2018 at 08:17, Lee, Chun-Yi  wrote:
> 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.
>

Why do we still need this patch? I.e., in which circumstances will
efi_memory_map_data::size assume a value that is rounded up or
otherwise incorrect?

> 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

Re: [PATCH v2] efi/capsule-loader: Don't output reset log when reset flags are not set

[Ard Biesheuvel]

On 3 May 2018 at 07:45, Shunyong Yang  wrote:
> It means firmware attempts to immediately process or launch the capsule
> when reset flags in capsule header are not set. Moreover, reset is not
> needed in this case. The current code will output log to indicate reset.
>
> This patch adds a branch to avoid reset log output when the flags are not
> set.
>
> Cc: Joey Zheng 
> Signed-off-by: Shunyong Yang 

Queued in efi/next

Thanks.

> ---
>
> Changes in v2:
>   *Add EFI_CAPSULE_PERSIST_ACROSS_RESET check according to Ard's
>suggestion.
>
> ---
>  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..344785ef8539 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 & EFI_CAPSULE_PERSIST_ACROSS_RESET)
> +   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

[PATCH v2] efi/capsule-loader: Don't output reset log when reset flags are not set

[Shunyong Yang]

It means firmware attempts to immediately process or launch the capsule
when reset flags in capsule header are not set. Moreover, reset is not
needed in this case. The current code will output log to indicate reset.

This patch adds a branch to avoid reset log output when the flags are not
set.

Cc: Joey Zheng 
Signed-off-by: Shunyong Yang 
---

Changes in v2:
  *Add EFI_CAPSULE_PERSIST_ACROSS_RESET check according to Ard's
   suggestion.
  
---
 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..344785ef8539 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 & EFI_CAPSULE_PERSIST_ACROSS_RESET)
+   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