Re: [PATCH v3 2/5] efi: Add embedded peripheral firmware support
On Thu, Jun 07, 2018 at 09:49:50AM -0700, Bjorn Andersson wrote: > On Tue 08 May 09:10 PDT 2018, Luis R. Rodriguez wrote: > > > On Tue, May 08, 2018 at 03:38:05PM +, Luis R. Rodriguez wrote: > > > On Fri, May 04, 2018 at 12:44:37PM -0700, Martijn Coenen wrote: > > > > On Wed, Apr 25, 2018 at 10:55 AM, Luis R. Rodriguez > > > > wrote: > [..] > > > > 2) Most of those paths are not mounted by the time the corresponding > > > > drivers are loaded, because pretty much all Android kernels today are > > > > built without module support, and therefore drivers are loaded well > > > > before the firmware partition is mounted > > > > I've given this some more thought and you can address this with initramfs, > > this is how other Linux distributions are addressing this. One way to > > address this automatically is to scrape the drivers built-in or needed > > early on > > boot in initamfs and if the driver has a MODULE_FIRMWARE() its respective > > firmware is added to initramfs as well. > > > > That could be done, but it would not change the fact that the > /sys/class/firmware is ABI and you may not break it. Right, this is now well documented and also the latest changes to the firmware API have made the sysfs fallback loader an option through a sysctl knob. The code should be much easier to follow and test now. > And it doesn't change the fact that the ramdisk would have to be over > 100mb to facilitate this. Indeed, this is now acknowledged in the latest Kconfig for the firmware loader. > > If you *don't* use initramfs, then yes you can obviously run into issues > > where your firmware may not be accessible if the driver is somehow loaded > > early. > > > > This is still a problem that lacks a solution. The firmwared solution capturing uevents and using the sysfs fallback mechanism should resolve this. Its also now properly documented on the firmware loader Kconfig. > > > > 3) I think we use _FALLBACK because doing this with uevents is just > > > > the easiest thing to do; our init code has a firmware helper that > > > > deals with this and searches the paths that we care about > > > > > > > > 2) will change at some point, because Android is moving towards a > > > > model where device-specific peripheral drivers will be loaded as > > > > modules, and since those modules would likely come from the same > > > > partition as the firmware, it's possible that the direct load would > > > > succeed (depending on whether the custom path is configured there or > > > > not). But I don't think we can rely on the direct loader even in those > > > > cases, unless we could configure it with multiple custom paths. > > > > Using initramfs will help, but because of the custom path needs -- you're > > right, we don't have anything for that yet, its also a bit unclear if > > something nice and clean can be drawn up for it. So perhaps dealing with > > the fallback mechanism is the way to go for this for sure, since we already > > have support for it. > > > > Just keep in mind that the fallback mechanism costs you about ~13436 bytes. > > > > Remember that putting the firmware in the ramdisk would cost about > 1x (yes, ten thousand times) more ram. Indeed, this is now documented on the Kconfig too. > > So, if someone comes up with a clean interface for custom paths I'd love > > to consider it to avoid those 13436 bytes. > > > > Combined with a way of synchronizing this with the availability of the > firmware, this would be a nice thing! The firmwared solution seems to be the way to go for now. 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 v3 2/5] efi: Add embedded peripheral firmware support
On Tue 08 May 09:10 PDT 2018, Luis R. Rodriguez wrote: > On Tue, May 08, 2018 at 03:38:05PM +, Luis R. Rodriguez wrote: > > On Fri, May 04, 2018 at 12:44:37PM -0700, Martijn Coenen wrote: > > > On Wed, Apr 25, 2018 at 10:55 AM, Luis R. Rodriguez > > > wrote: [..] > > > 2) Most of those paths are not mounted by the time the corresponding > > > drivers are loaded, because pretty much all Android kernels today are > > > built without module support, and therefore drivers are loaded well > > > before the firmware partition is mounted > > I've given this some more thought and you can address this with initramfs, > this is how other Linux distributions are addressing this. One way to > address this automatically is to scrape the drivers built-in or needed early > on > boot in initamfs and if the driver has a MODULE_FIRMWARE() its respective > firmware is added to initramfs as well. > That could be done, but it would not change the fact that the /sys/class/firmware is ABI and you may not break it. And it doesn't change the fact that the ramdisk would have to be over 100mb to facilitate this. > If you *don't* use initramfs, then yes you can obviously run into issues > where your firmware may not be accessible if the driver is somehow loaded > early. > This is still a problem that lacks a solution. > > > 3) I think we use _FALLBACK because doing this with uevents is just > > > the easiest thing to do; our init code has a firmware helper that > > > deals with this and searches the paths that we care about > > > > > > 2) will change at some point, because Android is moving towards a > > > model where device-specific peripheral drivers will be loaded as > > > modules, and since those modules would likely come from the same > > > partition as the firmware, it's possible that the direct load would > > > succeed (depending on whether the custom path is configured there or > > > not). But I don't think we can rely on the direct loader even in those > > > cases, unless we could configure it with multiple custom paths. > > Using initramfs will help, but because of the custom path needs -- you're > right, we don't have anything for that yet, its also a bit unclear if > something nice and clean can be drawn up for it. So perhaps dealing with > the fallback mechanism is the way to go for this for sure, since we already > have support for it. > > Just keep in mind that the fallback mechanism costs you about ~13436 bytes. > Remember that putting the firmware in the ramdisk would cost about 1x (yes, ten thousand times) more ram. > So, if someone comes up with a clean interface for custom paths I'd love > to consider it to avoid those 13436 bytes. > Combined with a way of synchronizing this with the availability of the firmware, this would be a nice thing! Regards, Bjorn -- 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
On Tue 24 Apr 22:00 PDT 2018, 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: [..] > > > > 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. > > The call sequence: > qcom_mdt_load() -> qcom_scm_pas_init_image() -> dma_alloc_coherent() > qcom_mdt_load() is passed a struct firmware object, which "data" is passed into qcom_scm_pas_init_image(), which uses dma_alloc_coherent() to allocate scratch memory to perform a call into secure world. So the dma_alloc_coherent() here has nothing to do with firmware loading. qcom_mdt_load() will then use request_firmware_into_buf() to load other files into the passed void *mem_region, which either comes from a memremap() or dma_alloc_coherent() call. > If dma_alloc_coherent() isn't allocating a DMA buffer, then the > function name is misleading/confusing. > It does allocate memory. > > > > 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? > I would expect that it's possible to read the content of the buffer from a secondary execution context before the request_firmware_into_buf() has verified the content of the buffer, but I would be considered a seriously broken driver. Regards, Bjorn -- 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
On Tue, May 08, 2018 at 03:38:05PM +, Luis R. Rodriguez wrote: > On Fri, May 04, 2018 at 12:44:37PM -0700, Martijn Coenen wrote: > > On Wed, Apr 25, 2018 at 10:55 AM, Luis R. Rodriguez > > wrote: > > > Is ptr below > > > > > > ret = request_firmware_into_buf(&seg_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? > > > > I think the Qualcomm folks owning this (Andy, David, Bjorn, already > > cc'd here) are better suited to answer that question. > > Andy, David, Bjorn? Andy, David, Bjorn? Note: as-is we have no option but to assume this is DMA memory for now. We cannot keep IMA's guarantees with the current prealloc firmware API buffer, so I've suggested: a) The prealloc buffer API be expanded to enable the caller to descrbe it b) Have the qcom driver say this is DMA c) IMA would reject it to ensure it stays true to what it needs to gaurantee d) Future platforms which want to use IMA but want to trust DMA buffers would need to devise a way to describe IMA can trust some of these calls. I'll leave it up to you guys (Andy, David, Bjorn) to come up with the code for d) once and if you guys want to use IMA later. But since what is pressing here is to stay to true to IMA, with a-c IMA would reject such calls for now. 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 v3 2/5] efi: Add embedded peripheral firmware support
On Tue, May 08, 2018 at 03:38:05PM +, Luis R. Rodriguez wrote: > On Fri, May 04, 2018 at 12:44:37PM -0700, Martijn Coenen wrote: > > On Wed, Apr 25, 2018 at 10:55 AM, Luis R. Rodriguez > > wrote: > > > Android became the primary user of CONFIG_FW_LOADER_USER_HELPER_FALLBACK. > > > > > > It would be good for us to hear from Android folks if their current use of > > > request_firmware_into_buf() is designed in practice to *never* use the > > > direct > > > filesystem firmware loading interface, and always rely instead on the > > > fallback mechanism. > > > > It's hard to answer this question for Android in general. As far as I > > can tell the reasons we use CONFIG_FW_LOADER_USER_HELPER(_FALLBACK) > > are: > > 1) We have multiple different paths on our devices where firmware can > > be located, and the direct loader only supports one custom path FWIW I'd love to consider patches to address this, if this is something you may find a need for in the future to *avoid* the fallback, however would like a clean solution. > > 2) Most of those paths are not mounted by the time the corresponding > > drivers are loaded, because pretty much all Android kernels today are > > built without module support, and therefore drivers are loaded well > > before the firmware partition is mounted I've given this some more thought and you can address this with initramfs, this is how other Linux distributions are addressing this. One way to address this automatically is to scrape the drivers built-in or needed early on boot in initamfs and if the driver has a MODULE_FIRMWARE() its respective firmware is added to initramfs as well. If you *don't* use initramfs, then yes you can obviously run into issues where your firmware may not be accessible if the driver is somehow loaded early. > > 3) I think we use _FALLBACK because doing this with uevents is just > > the easiest thing to do; our init code has a firmware helper that > > deals with this and searches the paths that we care about > > > > 2) will change at some point, because Android is moving towards a > > model where device-specific peripheral drivers will be loaded as > > modules, and since those modules would likely come from the same > > partition as the firmware, it's possible that the direct load would > > succeed (depending on whether the custom path is configured there or > > not). But I don't think we can rely on the direct loader even in those > > cases, unless we could configure it with multiple custom paths. Using initramfs will help, but because of the custom path needs -- you're right, we don't have anything for that yet, its also a bit unclear if something nice and clean can be drawn up for it. So perhaps dealing with the fallback mechanism is the way to go for this for sure, since we already have support for it. Just keep in mind that the fallback mechanism costs you about ~13436 bytes. So, if someone comes up with a clean interface for custom paths I'd love to consider it to avoid those 13436 bytes. 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 v3 2/5] efi: Add embedded peripheral firmware support
On Fri, May 04, 2018 at 12:44:37PM -0700, Martijn Coenen wrote: > On Wed, Apr 25, 2018 at 10:55 AM, Luis R. Rodriguez wrote: > > Android became the primary user of CONFIG_FW_LOADER_USER_HELPER_FALLBACK. > > > > It would be good for us to hear from Android folks if their current use of > > request_firmware_into_buf() is designed in practice to *never* use the > > direct > > filesystem firmware loading interface, and always rely instead on the > > fallback mechanism. > > It's hard to answer this question for Android in general. As far as I > can tell the reasons we use CONFIG_FW_LOADER_USER_HELPER(_FALLBACK) > are: > 1) We have multiple different paths on our devices where firmware can > be located, and the direct loader only supports one custom path > 2) Most of those paths are not mounted by the time the corresponding > drivers are loaded, because pretty much all Android kernels today are > built without module support, and therefore drivers are loaded well > before the firmware partition is mounted > 3) I think we use _FALLBACK because doing this with uevents is just > the easiest thing to do; our init code has a firmware helper that > deals with this and searches the paths that we care about > > 2) will change at some point, because Android is moving towards a > model where device-specific peripheral drivers will be loaded as > modules, and since those modules would likely come from the same > partition as the firmware, it's possible that the direct load would > succeed (depending on whether the custom path is configured there or > not). But I don't think we can rely on the direct loader even in those > cases, unless we could configure it with multiple custom paths. > > I have no reason to believe request_firmware_into_buf() is special in > this regard; drivers that depend on it may have their corresponding > firmware in different locations, so just depending on the direct > loader would not be good enough. Thanks! This is very useful! This provides yet-another justification and use case to document for the fallback mechanism. I'll go and extend it. > > > > Is ptr below > > > > ret = request_firmware_into_buf(&seg_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? > > I think the Qualcomm folks owning this (Andy, David, Bjorn, already > cc'd here) are better suited to answer that question. Andy, David, Bjorn? 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 v3 2/5] efi: Add embedded peripheral firmware support
On Wed, Apr 25, 2018 at 10:55 AM, Luis R. Rodriguez wrote: > Android became the primary user of CONFIG_FW_LOADER_USER_HELPER_FALLBACK. > > It would be good for us to hear from Android folks if their current use of > request_firmware_into_buf() is designed in practice to *never* use the direct > filesystem firmware loading interface, and always rely instead on the > fallback mechanism. It's hard to answer this question for Android in general. As far as I can tell the reasons we use CONFIG_FW_LOADER_USER_HELPER(_FALLBACK) are: 1) We have multiple different paths on our devices where firmware can be located, and the direct loader only supports one custom path 2) Most of those paths are not mounted by the time the corresponding drivers are loaded, because pretty much all Android kernels today are built without module support, and therefore drivers are loaded well before the firmware partition is mounted 3) I think we use _FALLBACK because doing this with uevents is just the easiest thing to do; our init code has a firmware helper that deals with this and searches the paths that we care about 2) will change at some point, because Android is moving towards a model where device-specific peripheral drivers will be loaded as modules, and since those modules would likely come from the same partition as the firmware, it's possible that the direct load would succeed (depending on whether the custom path is configured there or not). But I don't think we can rely on the direct loader even in those cases, unless we could configure it with multiple custom paths. I have no reason to believe request_firmware_into_buf() is special in this regard; drivers that depend on it may have their corresponding firmware in different locations, so just depending on the direct loader would not be good enough. > > Is ptr below > > ret = request_firmware_into_buf(&seg_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? I think the Qualcomm folks owning this (Andy, David, Bjorn, already cc'd here) are better suited to answer that question. Thanks, Martijn -- 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
On Thu, May 3, 2018 at 5:21 PM, Luis R. Rodriguez wrote: > 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: Sorry, figuring out who's the right person to answer this, will get back to you ASAP. Martijn > > 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(&seg_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 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
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(&seg_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 uns
Re: [PATCH v3 2/5] efi: Add embedded peripheral firmware support
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: > > > > On 23-04-18 23:11, Luis R. Rodriguez wrote: > > > > > Hans, please see use of READING_FIRMWARE_PREALLOC_BUFFER, we'll need > > > > > a new ID > > > > > and security for this type of request so IMA can reject it if the > > > > > policy is > > > > > configured for it. > > > > > > > > Hmm, interesting, actually it seems like the whole existence > > > > of READING_FIRMWARE_PREALLOC_BUFFER is a mistake, > > > > request_firmware_into_buf() was merged without my own review, however, > > the ID thing did get review from Mimi: > > > > https://patchwork.kernel.org/patch/9074611/ > > > > The ID is not for IMA alone, its for any LSM to decide what to do. > > Note Mimi asked for READING_FIRMWARE_DMA if such buffer was in DMA, > > otherise READING_FIRMWARE_PREALLOC_BUFFER was suggested. > > > > Mimi why did you want a separate ID for it back before? > > The point of commit a098ecd2fa7d ("firmware: support loading into a > pre-allocated buffer") is to avoid reading the firmware into kernel > memory and then copying it "to it's final resting place". My concern > is that if the device driver has access to the buffer, it could access > the buffer prior to the firmware's signature having been verified by > the kernel. If request_firmware_into_buf() is used and the firmware was found in /lib/firmware/ paths then the driver will *not* use the firmware prior to any LSM doing any firmware signature verification because kernel_read_file_from_path() and in turn security_kernel_read_file(). The firmware API has a fallback mechanism [0] though, and if that is used then security_kernel_post_read_file() is used once the firmware is loaded through the sysfs interface *prior* to handing the firmware data to the driver. As Hans noted though security_kernel_post_read_file() currently *only* uses READING_FIRMWARE, so this needs to be fixed. Also note though that LSMs get a hint of what is going to happen *soon* prior to the fallback mechanism kicking on as we travere the /lib/firmware/ paths for direct filesystem loading. If this is not sufficient to cover LSM appraisals *one* option could be to have security_kernel_read_file() return a special error of some sort for READING_FIRMWARE_PREALLOC_BUFFER so that kernel_read_file_from_path() users could *know* to fatally give up. Currently the device drivers using request_firmware_into_buf() can end up getting the buffer with firmware stashed in it without having the kernel do any firmware signature verification at all through its LSMs. The LSM hooks added to the firmware loader long ago by Kees via commit 6593d9245bc66 ("firmware_class: perform new LSM checks") on v3.17 added an LSM for direct filesystem lookups, but on the fallback mechanism seems to have only added a post LSM hook security_kernel_fw_from_file(). There is also a custom fallback mechanism [1] which can be used if the path to the firmware may be out of the /lib/firmware/ paths or perhaps the firmware requires some very custom fetching of some sort. The only thing this does though is just *not* issue a uevent when we don't find the firmware and also sets the timeout to a practically never-ending value. The custom fallback mechanism is only usable for request_firmware_nowait() though. In retrospect the custom fallback mechanism is pure crap and these days we've acknowledged that even in crazy custom firmware fetching cases folks should be able to accomplish this by relying on uevents and using the firmwared [2] or forking it, or a different similar proprietary similar solution, which would just monitor for uevents for firmware and just Do The Right Thing (TM). Consider some mobile devices which may want to fetch it from some custom partition which only it can know how to get. There is a kernel config option which enables the fallback mechanism always, This is now easily readable as follows: drivers/base/firmware_loader/fallback_table.c struct firmware_fallback_config fw_fallback_config = { .force_sysfs_fallback = IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK), .loading_timeout = 60, .old_timeout = 60, }; Even if this is used we always do direct fs lookups first. Android became the primary user of CONFIG_FW_LOADER_USER_HELPER_FALLBACK. It would be good for us to hear from Android folks if their current use of request_firmware_into_buf() is designed in practice to *never* use the direct filesystem firmware loading interface, and always rely instead on the fallback mechanism. That would answer help your appraisal question in practice today. [0] https://www.kernel.org/doc/html/latest/driver-api/firmware/fallback-mechanisms.html [1] https://www.kernel.org/doc/html/latest/driver-api/firmware/fallback-mechanisms.html#firmware-custom-
Re: [PATCH v3 2/5] efi: Add embedded peripheral firmware support
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: > > > Hi, > > > > > > On 23-04-18 23:11, Luis R. Rodriguez wrote: > > > > Hans, please see use of READING_FIRMWARE_PREALLOC_BUFFER, we'll need a > > > > new ID > > > > and security for this type of request so IMA can reject it if the > > > > policy is > > > > configured for it. > > > > > > Hmm, interesting, actually it seems like the whole existence > > > of READING_FIRMWARE_PREALLOC_BUFFER is a mistake, > > request_firmware_into_buf() was merged without my own review, however, > the ID thing did get review from Mimi: > > https://patchwork.kernel.org/patch/9074611/ > > The ID is not for IMA alone, its for any LSM to decide what to do. > Note Mimi asked for READING_FIRMWARE_DMA if such buffer was in DMA, > otherise READING_FIRMWARE_PREALLOC_BUFFER was suggested. > > > > the IMA > > > framework really does not care if we are loading the firmware > > > into memory allocated by the firmware-loader code, or into > > > memory allocated by the device-driver requesting the firmware. > > That's up to LSM folks to decide. We have these so far: > > #define __kernel_read_file_id(id) \ > > id(UNKNOWN, unknown)\ > > id(FIRMWARE, firmware) \ > > id(FIRMWARE_PREALLOC_BUFFER, firmware) \ > > id(MODULE, kernel-module) \ > > id(KEXEC_IMAGE, kexec-image)\ > > id(KEXEC_INITRAMFS, kexec-initramfs)\ > > id(POLICY, security-policy) \ > > id(X509_CERTIFICATE, x509-certificate) \ > > id(MAX_ID, ) > > The first type of IDs added was about type of files the kernel > LSMs may want to do different things for. > > Mimi why did you want a separate ID for it back before? The point of commit a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer") is to avoid reading the firmware into kernel memory and then copying it "to it's final resting place". My concern is that if the device driver has access to the buffer, it could access the buffer prior to the firmware's signature having been verified by the kernel. In tightly controlled environments interested in limiting which signed firmware version is loaded, require's the device driver not having access to the buffer until after the signature has been verified by the kernel (eg. IMA-appraisal). > > I should note now that request_firmware_into_buf() and its > READING_FIRMWARE_PREALLOC_BUFFER was to enable a driver on memory constrained > devices. The files are large (commit says 16 MiB). > > I've heard of larger possible files with remoteproc and with Android using > the custom fallback mechanism -- which could mean a proprietary tool > fetching firmware from a random special place on a device. > > I could perhaps imagine an LSM which may be aware of such type of > arrangement may want to do its own vetting of some sort, but this > would not be specific to READING_FIRMWARE_PREALLOC_BUFFER, but rather > the custom fallback mechaism. > > Whether or not the buffer was preallocated by the driver seems a little > odd for security folks to do something different with it. Security LSM > folks please chime in. > > I could see a bit more of a use case for an ID for firmware scraped > from EFI, which Hans' patch will provide. But that *also* should get > good review from other LSM folks. > > One of the issues with accepting more IDs loosely is where do we > stop though? If no one really is using READING_FIRMWARE_PREALLOC_BUFFER > I'd say lets remove it. Likewise, for this EFI thing I'd like an idea > if we really are going to have users for it. > > 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,
Re: [PATCH v3 2/5] efi: Add embedded peripheral firmware support
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: > > Hi, > > > > On 23-04-18 23:11, Luis R. Rodriguez wrote: > > > Hans, please see use of READING_FIRMWARE_PREALLOC_BUFFER, we'll need a > > > new ID > > > and security for this type of request so IMA can reject it if the policy > > > is > > > configured for it. > > > > Hmm, interesting, actually it seems like the whole existence > > of READING_FIRMWARE_PREALLOC_BUFFER is a mistake, request_firmware_into_buf() was merged without my own review, however, the ID thing did get review from Mimi: https://patchwork.kernel.org/patch/9074611/ The ID is not for IMA alone, its for any LSM to decide what to do. Note Mimi asked for READING_FIRMWARE_DMA if such buffer was in DMA, otherise READING_FIRMWARE_PREALLOC_BUFFER was suggested. > > the IMA > > framework really does not care if we are loading the firmware > > into memory allocated by the firmware-loader code, or into > > memory allocated by the device-driver requesting the firmware. That's up to LSM folks to decide. We have these so far: #define __kernel_read_file_id(id) \ id(UNKNOWN, unknown)\ id(FIRMWARE, firmware) \ id(FIRMWARE_PREALLOC_BUFFER, firmware) \ id(MODULE, kernel-module) \ id(KEXEC_IMAGE, kexec-image)\ id(KEXEC_INITRAMFS, kexec-initramfs)\ id(POLICY, security-policy) \ id(X509_CERTIFICATE, x509-certificate) \ id(MAX_ID, ) The first type of IDs added was about type of files the kernel LSMs may want to do different things for. Mimi why did you want a separate ID for it back before? I should note now that request_firmware_into_buf() and its READING_FIRMWARE_PREALLOC_BUFFER was to enable a driver on memory constrained devices. The files are large (commit says 16 MiB). I've heard of larger possible files with remoteproc and with Android using the custom fallback mechanism -- which could mean a proprietary tool fetching firmware from a random special place on a device. I could perhaps imagine an LSM which may be aware of such type of arrangement may want to do its own vetting of some sort, but this would not be specific to READING_FIRMWARE_PREALLOC_BUFFER, but rather the custom fallback mechaism. Whether or not the buffer was preallocated by the driver seems a little odd for security folks to do something different with it. Security LSM folks please chime in. I could see a bit more of a use case for an ID for firmware scraped from EFI, which Hans' patch will provide. But that *also* should get good review from other LSM folks. One of the issues with accepting more IDs loosely is where do we stop though? If no one really is using READING_FIRMWARE_PREALLOC_BUFFER I'd say lets remove it. Likewise, for this EFI thing I'd like an idea if we really are going to have users for it. 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, Are we going to add more IDs for more types of firmware? What type of *different* decisions could LSMs take if the firmware was being written to a buffer? Or in this new case that is coming up, if the file came scraping EFI, would having that information be useful? > > 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. 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 a
Re: [PATCH v3 2/5] efi: Add embedded peripheral firmware support
Hi, On 24-04-18 18:07, Mimi Zohar wrote: On Tue, 2018-04-24 at 17:09 +0200, Hans de Goede wrote: Hi, On 23-04-18 23:11, Luis R. Rodriguez wrote: Hans, please see use of READING_FIRMWARE_PREALLOC_BUFFER, we'll need a new ID and security for this type of request so IMA can reject it if the policy is configured for it. Hmm, interesting, actually it seems like the whole existence of READING_FIRMWARE_PREALLOC_BUFFER is a mistake, the IMA framework really does not care if we are loading the firmware into memory allocated by the firmware-loader code, or into memory allocated by the device-driver requesting the firmware. 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, will determine how IMA-appraisal addresses READING_FIRMWARE_PREALLOC_BUFFER. Ah I see. So this probably means that the IMA integration for my EFI embedded firmware code should also pass READING_FIRMWARE or READING_FIRMWARE_PREALLOC_BUFFER depending on if a pre-allocated buffer is used. Hmm, the security_kernel_post_read_file() call in drivers/base/firmware_loader/fallback.c Unconditionally passes READING_FIRMWARE, it should probably check fw_priv->is_paged_buf and base the id to pass on that. And yes it is possible AFAICT for the firmware_request_into_buf() method to fallback to the userspace helper, this can happen if the fw_fallback_config.force_sysfs_fallback flag is set. 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 v3 2/5] efi: Add embedded peripheral firmware support
On Tue, 2018-04-24 at 17:09 +0200, Hans de Goede wrote: > Hi, > > On 23-04-18 23:11, Luis R. Rodriguez wrote: > > Hans, please see use of READING_FIRMWARE_PREALLOC_BUFFER, we'll need a new > > ID > > and security for this type of request so IMA can reject it if the policy is > > configured for it. > > Hmm, interesting, actually it seems like the whole existence > of READING_FIRMWARE_PREALLOC_BUFFER is a mistake, the IMA > framework really does not care if we are loading the firmware > into memory allocated by the firmware-loader code, or into > memory allocated by the device-driver requesting the firmware. > > 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, will determine how IMA-appraisal addresses READING_FIRMWARE_PREALLOC_BUFFER. Mimi > here > are bits of code from: security/integrity/ima/ima_main.c: > > static int read_idmap[READING_MAX_ID] = { > [READING_FIRMWARE] = FIRMWARE_CHECK, > [READING_MODULE] = MODULE_CHECK, > [READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK, > [READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK, > [READING_POLICY] = POLICY_CHECK > }; > > int ima_post_read_file(struct file *file, void *buf, loff_t size, > ... > if (!file && read_id == READING_FIRMWARE) { > if ((ima_appraise & IMA_APPRAISE_FIRMWARE) && > (ima_appraise & IMA_APPRAISE_ENFORCE)) > return -EACCES; /* INTEGRITY_UNKNOWN */ > return 0; > } > > Which show that the IMA code is not handling > READING_FIRMWARE_PREALLOC_BUFFER as it should (I believe it > should handle it the same as READING_FIRMWARE). > > Now we could fix that, but the only user of > READING_FIRMWARE_PREALLOC_BUFFER is the code which originally > introduced it: > > https://patchwork.kernel.org/patch/9162011/ > > So I believe it might be better to instead replace it > with just READING_FIRMWARE and find another way to tell > kernel_read_file() that there is a pre-allocated buffer, > perhaps the easiest way there is that *buf must be > NULL when the caller wants kernel_read_file() to > vmalloc the mem. This would of course require auditing > all callers that the buf which the pass in is initialized > to NULL. > > Either way adding a third READING_FIRMWARE_FOO to the > kernel_read_file_id enum seems like a bad idea, from > the IMA pov firmware is firmware. > > What this whole exercise has shown me though is that > I need to call security_kernel_post_read_file() when > loading EFI embedded firmware. I will add a call to > security_kernel_post_read_file() for v4 of the patch-set. > > > Please Cc Kees in future patches. > > Will do. > > 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 v3 2/5] efi: Add embedded peripheral firmware support
Hi, On 23-04-18 23:11, Luis R. Rodriguez wrote: Hans, please see use of READING_FIRMWARE_PREALLOC_BUFFER, we'll need a new ID and security for this type of request so IMA can reject it if the policy is configured for it. Hmm, interesting, actually it seems like the whole existence of READING_FIRMWARE_PREALLOC_BUFFER is a mistake, the IMA framework really does not care if we are loading the firmware into memory allocated by the firmware-loader code, or into memory allocated by the device-driver requesting the firmware. As such the current IMA code (from v4.17-rc2) actually does not handle READING_FIRMWARE_PREALLOC_BUFFER at all, here are bits of code from: security/integrity/ima/ima_main.c: static int read_idmap[READING_MAX_ID] = { [READING_FIRMWARE] = FIRMWARE_CHECK, [READING_MODULE] = MODULE_CHECK, [READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK, [READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK, [READING_POLICY] = POLICY_CHECK }; int ima_post_read_file(struct file *file, void *buf, loff_t size, ... if (!file && read_id == READING_FIRMWARE) { if ((ima_appraise & IMA_APPRAISE_FIRMWARE) && (ima_appraise & IMA_APPRAISE_ENFORCE)) return -EACCES; /* INTEGRITY_UNKNOWN */ return 0; } Which show that the IMA code is not handling READING_FIRMWARE_PREALLOC_BUFFER as it should (I believe it should handle it the same as READING_FIRMWARE). Now we could fix that, but the only user of READING_FIRMWARE_PREALLOC_BUFFER is the code which originally introduced it: https://patchwork.kernel.org/patch/9162011/ So I believe it might be better to instead replace it with just READING_FIRMWARE and find another way to tell kernel_read_file() that there is a pre-allocated buffer, perhaps the easiest way there is that *buf must be NULL when the caller wants kernel_read_file() to vmalloc the mem. This would of course require auditing all callers that the buf which the pass in is initialized to NULL. Either way adding a third READING_FIRMWARE_FOO to the kernel_read_file_id enum seems like a bad idea, from the IMA pov firmware is firmware. What this whole exercise has shown me though is that I need to call security_kernel_post_read_file() when loading EFI embedded firmware. I will add a call to security_kernel_post_read_file() for v4 of the patch-set. Please Cc Kees in future patches. Will do. 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 v3 2/5] efi: Add embedded peripheral firmware support
Hi, On 16-04-18 10:28, Ard Biesheuvel wrote: On 8 April 2018 at 19:40, 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 Signed-off-by: Hans de Goede --- Changes in v2: -Rebased on driver-core/driver-core-next -Add documentation describing the EFI embedded firmware mechanism to: Documentation/driver-api/firmware/request_firmware.rst -Add a new EFI_EMBEDDED_FIRMWARE Kconfig bool and only build the embedded fw support if this is set. This is an invisible option which should be selected by drivers which need this -Remove the efi_embedded_fw_desc and dmi_system_id-s for known devices from the efi-embedded-fw code, instead drivers using this are expected to export a dmi_system_id array, with each entries' driver_data pointing to a efi_embedded_fw_desc struct and register this with the efi-embedded-fw code -Use kmemdup to make a copy instead of efi_mem_reserve()-ing the firmware, this avoids us messing with the EFI memmap and avoids the need to make changes to efi_mem_desc_lookup() -Make the firmware-loader code only fallback to efi_get_embedded_fw() if the passed in device has the "efi-embedded-firmware" device-property bool set -Skip usermodehelper fallback when "efi-embedded-firmware" device-property is set Changes in v3: -Fix the docs using "efi-embedded-fw" as property name instead of "efi-embedded-firmware" --- .../driver-api/firmware/request_firmware.rst | 70 + drivers/base/firmware_loader/main.c | 33 drivers/firmware/efi/Kconfig | 6 + drivers/firmware/efi/Makefile | 1 + drivers/firmware/efi/embedded-firmware.c | 148 ++ include/linux/efi.h | 6 + include/linux/efi_embedded_fw.h | 25 +++ init/main.c | 1 + 8 files changed, 290 insertions(+) create mode 100644 drivers/firmware/efi/embedded-firmware.c create mode 100644 include/linux/efi_embedded_fw.h diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst index 20f21ed427a5..189b02f815c9 100644 --- a/Documentation/driver-api/firmware/request_firmware.rst +++ b/Documentation/driver-api/firmware/request_firmware.rst @@ -68,3 +68,73 @@ If something went wrong request_firmware() returns non-zero and fw_entry is set to NULL. Once your driver is done with processing the firmware it can call call release_firmware(fw_entry) to release the firmware image and any related resource. + +EFI embedded firmware support += + +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. + +A device driver which needs this can describe the firmware it needs +using an efi_embedded_fw_desc struct: + +.. kernel-doc:: include/linux/efi_embedded_fw.h + :functions: efi_embedded_fw_desc + +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
Re: [PATCH v3 2/5] efi: Add embedded peripheral firmware support
Hans, please see use of READING_FIRMWARE_PREALLOC_BUFFER, we'll need a new ID and security for this type of request so IMA can reject it if the policy is configured for it. Please Cc Kees in future patches. 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 v3 2/5] efi: Add embedded peripheral firmware support
Hi, On 17-04-18 02:17, Luis R. Rodriguez wrote: On Sun, Apr 08, 2018 at 07:40:11PM +0200, Hans de Goede wrote: static void firmware_free_data(const struct firmware *fw) { @@ -576,6 +600,15 @@ _request_firmware(const struct firmware **firmware_p, const char *name, goto out; ret = fw_get_filesystem_firmware(device, fw->priv); +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE + if (ret && device && + device_property_read_bool(device, "efi-embedded-firmware")) { + ret = fw_get_efi_embedded_fw(device, fw->priv, ret); + if (ret == 0) + ret = assign_fw(fw, device, opt_flags | FW_OPT_NOCACHE); + goto out; + } +#endif So thinking some more about this, I can put the device_property check inside the fw_get_efi_embedded_fw() call, as well as modify opt_flags there to or in FW_OPT_NOCACHE on success, then together with the discussed changed to drop the #ifdef, the code would look like this: ret = fw_get_filesystem_firmware(device, fw->priv); if (ret) fw_get_efi_embedded_fw(device, fw->priv, &opt_flags, ret); if (ret) if (!(opt_flags & FW_OPT_NO_WARN)) dev_warn(device, ... With just these 2 lines being new: if (ret) fw_get_efi_embedded_fw(device, fw->priv, &opt_flags, ret); So the main.c changes will be nice and clean then. 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 v3 2/5] efi: Add embedded peripheral firmware support
Hi, On 17-04-18 02:17, Luis R. Rodriguez wrote: On Sun, Apr 08, 2018 at 07:40:11PM +0200, Hans de Goede wrote: static void firmware_free_data(const struct firmware *fw) { @@ -576,6 +600,15 @@ _request_firmware(const struct firmware **firmware_p, const char *name, goto out; ret = fw_get_filesystem_firmware(device, fw->priv); +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE + if (ret && device && + device_property_read_bool(device, "efi-embedded-firmware")) { + ret = fw_get_efi_embedded_fw(device, fw->priv, ret); + if (ret == 0) + ret = assign_fw(fw, device, opt_flags | FW_OPT_NOCACHE); + goto out; + } +#endif You mussed what I asked for in terms of adding a new flag, (please work on top of Andre's patches as those likely will be merged first, and also have kdocs for the flags) Ok I will base my next version on top of Andres' series. and then a new firmware API to wrap the above into a function which would only do something if the driver *asked* for it on their firmware API call. Ie, please add a new firmware_request_efi_fw(). As I tried to explain in the changelog the problem with doing this, is that this makes it a driver decision, where it really needs to be platform-code driven, not driver driven. Take for example the drivers/input/touchscreen/silead.c code that is used on a lot of 32 bit ARM platforms too, which don't have EFI at all, so if that needs to call request_firmware_efi() then should I add: #ifdef CONFIG_X86 fw = request_firmware_efi(...); #else fw = request_firmware(...); #endif ? But even on x86 only some devices with a silead touchscreen have EFI embedded firmware, so then I would need something like: #ifdef CONFIG_X86 if (device_property_get_bool(dev, "some-prop-name")) fw = request_firmware_efi(...); else #else fw = request_firmware(...); #endif That is assuming I still want the normal fallback path in the case no EFI firmware is available, which I do because then something like packagekit may see if the firmware is packaged in one of the configured distro repositories. We already have (x86) platform code in place to attach properties (like a board specific firmware filename) to the device using device-properties so that drivers like silead.c don't get filled / polluted with board/platform specific knowledge, which IMHO is the place where the knowledge fallback to an EFI embedded firmware copy belongs. As the further patches in v3 of this series shows, this actually works quite nicely, because this also allows bundling the EFI-embedded firmware info (prefix, length, crc, name) together with the other board specific properties. TL;DR: using request_firmware_efi() vs request_firmware() is a driver decision, but whether EFI firmware fallback should be is board/platform specific not driver specific, therefor I believe that using a device-property to signal this is better. If you insist on me adding a request_firmware_efi() I can give this a shot, but I know that Dmitry (the input maintainer) will very much dislike the silead.c changes that implies... Still a question for lets sat we go that route, what do we then do with request_firmware_efi() when CONFIG_EFI is not set ? Should it be defined then or not, and if it should be defined when CONFIG_EFI is not set what should it do then? Also if you see the work I've done to remove the ifdefs over fallback mechanism you'll see it helps split code and make it easier to read. We should strive to not add any more ifdefery and instead make tehis code read easily. So looking at how the CONFIG_FW_LOADER_USER_HELPER stuff deals with this, I should: 1) Move the definition of fw_get_efi_embedded_fw() to a new drivers/base/firmware_loader/fallback_efi.c, which only gets build if CONFIG_EFI_EMBEDDED_FIRMWARE is set 2) Put the following in fallback.h: #ifdef CONFIG_EFI_EMBEDDED_FIRMWARE int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv, int ret); #else static inline int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv, int ret) { return ret; } #endif have I got that right? 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 v3 2/5] efi: Add embedded peripheral firmware support
On Sun, Apr 08, 2018 at 07:40:11PM +0200, Hans de Goede wrote: > static void firmware_free_data(const struct firmware *fw) > { > @@ -576,6 +600,15 @@ _request_firmware(const struct firmware **firmware_p, > const char *name, > goto out; > > ret = fw_get_filesystem_firmware(device, fw->priv); > +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE > + if (ret && device && > + device_property_read_bool(device, "efi-embedded-firmware")) { > + ret = fw_get_efi_embedded_fw(device, fw->priv, ret); > + if (ret == 0) > + ret = assign_fw(fw, device, opt_flags | FW_OPT_NOCACHE); > + goto out; > + } > +#endif You mussed what I asked for in terms of adding a new flag, (please work on top of Andre's patches as those likely will be merged first, and also have kdocs for the flags) and then a new firmware API to wrap the above into a function which would only do something if the driver *asked* for it on their firmware API call. Ie, please add a new firmware_request_efi_fw(). Also if you see the work I've done to remove the ifdefs over fallback mechanism you'll see it helps split code and make it easier to read. We should strive to not add any more ifdefery and instead make tehis code read easily. 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 v3 2/5] efi: Add embedded peripheral firmware support
On 8 April 2018 at 19:40, 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 > Signed-off-by: Hans de Goede > --- > Changes in v2: > -Rebased on driver-core/driver-core-next > -Add documentation describing the EFI embedded firmware mechanism to: > Documentation/driver-api/firmware/request_firmware.rst > -Add a new EFI_EMBEDDED_FIRMWARE Kconfig bool and only build the embedded > fw support if this is set. This is an invisible option which should be > selected by drivers which need this > -Remove the efi_embedded_fw_desc and dmi_system_id-s for known devices > from the efi-embedded-fw code, instead drivers using this are expected to > export a dmi_system_id array, with each entries' driver_data pointing to a > efi_embedded_fw_desc struct and register this with the efi-embedded-fw code > -Use kmemdup to make a copy instead of efi_mem_reserve()-ing the firmware, > this avoids us messing with the EFI memmap and avoids the need to make > changes to efi_mem_desc_lookup() > -Make the firmware-loader code only fallback to efi_get_embedded_fw() if the > passed in device has the "efi-embedded-firmware" device-property bool set > -Skip usermodehelper fallback when "efi-embedded-firmware" device-property > is set > > Changes in v3: > -Fix the docs using "efi-embedded-fw" as property name instead of > "efi-embedded-firmware" > --- > .../driver-api/firmware/request_firmware.rst | 70 + > drivers/base/firmware_loader/main.c | 33 > drivers/firmware/efi/Kconfig | 6 + > drivers/firmware/efi/Makefile | 1 + > drivers/firmware/efi/embedded-firmware.c | 148 ++ > include/linux/efi.h | 6 + > include/linux/efi_embedded_fw.h | 25 +++ > init/main.c | 1 + > 8 files changed, 290 insertions(+) > create mode 100644 drivers/firmware/efi/embedded-firmware.c > create mode 100644 include/linux/efi_embedded_fw.h > > diff --git a/Documentation/driver-api/firmware/request_firmware.rst > b/Documentation/driver-api/firmware/request_firmware.rst > index 20f21ed427a5..189b02f815c9 100644 > --- a/Documentation/driver-api/firmware/request_firmware.rst > +++ b/Documentation/driver-api/firmware/request_firmware.rst > @@ -68,3 +68,73 @@ If something went wrong request_firmware() returns > non-zero and fw_entry > is set to NULL. Once your driver is done with processing the firmware it > can call call release_firmware(fw_entry) to release the firmware image > and any related resource. > + > +EFI embedded firmware support > += > + > +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. > + > +A device driver which needs this can describe the firmware it needs > +using an efi_embedded_fw_desc struct: > + > +.. kernel-doc:: include/linux/efi_embedded_fw.h > + :functions: efi_embedded_fw_desc > + > +The EFI embedded-fw code works by scanning all EFI_BOOT_SERVICES_CODE memory > +segme
[PATCH v3 2/5] efi: Add embedded peripheral firmware support
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 Signed-off-by: Hans de Goede --- Changes in v2: -Rebased on driver-core/driver-core-next -Add documentation describing the EFI embedded firmware mechanism to: Documentation/driver-api/firmware/request_firmware.rst -Add a new EFI_EMBEDDED_FIRMWARE Kconfig bool and only build the embedded fw support if this is set. This is an invisible option which should be selected by drivers which need this -Remove the efi_embedded_fw_desc and dmi_system_id-s for known devices from the efi-embedded-fw code, instead drivers using this are expected to export a dmi_system_id array, with each entries' driver_data pointing to a efi_embedded_fw_desc struct and register this with the efi-embedded-fw code -Use kmemdup to make a copy instead of efi_mem_reserve()-ing the firmware, this avoids us messing with the EFI memmap and avoids the need to make changes to efi_mem_desc_lookup() -Make the firmware-loader code only fallback to efi_get_embedded_fw() if the passed in device has the "efi-embedded-firmware" device-property bool set -Skip usermodehelper fallback when "efi-embedded-firmware" device-property is set Changes in v3: -Fix the docs using "efi-embedded-fw" as property name instead of "efi-embedded-firmware" --- .../driver-api/firmware/request_firmware.rst | 70 + drivers/base/firmware_loader/main.c | 33 drivers/firmware/efi/Kconfig | 6 + drivers/firmware/efi/Makefile | 1 + drivers/firmware/efi/embedded-firmware.c | 148 ++ include/linux/efi.h | 6 + include/linux/efi_embedded_fw.h | 25 +++ init/main.c | 1 + 8 files changed, 290 insertions(+) create mode 100644 drivers/firmware/efi/embedded-firmware.c create mode 100644 include/linux/efi_embedded_fw.h diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst index 20f21ed427a5..189b02f815c9 100644 --- a/Documentation/driver-api/firmware/request_firmware.rst +++ b/Documentation/driver-api/firmware/request_firmware.rst @@ -68,3 +68,73 @@ If something went wrong request_firmware() returns non-zero and fw_entry is set to NULL. Once your driver is done with processing the firmware it can call call release_firmware(fw_entry) to release the firmware image and any related resource. + +EFI embedded firmware support += + +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. + +A device driver which needs this can describe the firmware it needs +using an efi_embedded_fw_desc struct: + +.. kernel-doc:: include/linux/efi_embedded_fw.h + :functions: efi_embedded_fw_desc + +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. + +To avoid doing this some