[v10,0/9] firmware: add request_partial_firmware_into_buf
mbox series

Message ID 20200706232309.12010-1-scott.branden@broadcom.com
Headers show
Series
  • firmware: add request_partial_firmware_into_buf
Related show

Message

Scott Branden July 6, 2020, 11:23 p.m. UTC
This patch series adds partial read support via a new call
request_partial_firmware_into_buf.
Such support is needed when the whole file is not needed and/or
only a smaller portion of the file will fit into allocated memory
at any one time.
In order to accept the enhanced API it has been requested that kernel
selftests and upstreamed driver utilize the API enhancement and so
are included in this patch series.

Also in this patch series is the addition of a new Broadcom VK driver
utilizing the new request_firmware_into_buf enhanced API.

Further comment followed to add IMA support of the partial reads
originating from request_firmware_into_buf calls.  And another request
to move existing kernel_read_file* functions to its own include file.

Changes from v9:
 - add patch to move existing kernel_read_file* to its own include file
 - driver fixes
Changes from v8:
 - correct compilation error when CONFIG_FW_LOADER not defined
Changes from v7:
 - removed swiss army knife kernel_pread_* style approach
   and simply add offset parameter in addition to those needed
   in kernel_read_* functions thus removing need for kernel_pread enum
Changes from v6:
 - update ima_post_read_file check on IMA_FIRMWARE_PARTIAL_READ
 - adjust new driver i2c-slave-eeprom.c use of request_firmware_into_buf
 - remove an extern
Changes from v5:
 - add IMA FIRMWARE_PARTIAL_READ support
 - change kernel pread flags to enum
 - removed legacy support from driver
 - driver fixes
Changes from v4:
 - handle reset issues if card crashes
 - allow driver to have min required msix
 - add card utilization information
Changes from v3:
 - fix sparse warnings
 - fix printf format specifiers for size_t
 - fix 32-bit cross-compiling reports 32-bit shifts
 - use readl/writel,_relaxed to access pci ioremap memory,
  removed memory barriers and volatile keyword with such change
 - driver optimizations for interrupt/poll functionalities
Changes from v2:
 - remove unnecessary code and mutex locks in lib/test_firmware.c
 - remove VK_IOCTL_ACCESS_BAR support from driver and use pci sysfs instead
 - remove bitfields
 - remove Kconfig default m
 - adjust formatting and some naming based on feedback
 - fix error handling conditions
 - use appropriate return codes
 - use memcpy_toio instead of direct access to PCIE bar


Scott Branden (9):
  fs: move kernel_read_file* to its own include file
  fs: introduce kernel_pread_file* support
  firmware: add request_partial_firmware_into_buf
  test_firmware: add partial read support for request_firmware_into_buf
  firmware: test partial file reads of request_partial_firmware_into_buf
  bcm-vk: add bcm_vk UAPI
  misc: bcm-vk: add Broadcom VK driver
  MAINTAINERS: bcm-vk: add maintainer for Broadcom VK Driver
  ima: add FIRMWARE_PARTIAL_READ support

 MAINTAINERS                                   |    7 +
 drivers/base/firmware_loader/firmware.h       |    5 +
 drivers/base/firmware_loader/main.c           |   80 +-
 drivers/misc/Kconfig                          |    1 +
 drivers/misc/Makefile                         |    1 +
 drivers/misc/bcm-vk/Kconfig                   |   29 +
 drivers/misc/bcm-vk/Makefile                  |   11 +
 drivers/misc/bcm-vk/bcm_vk.h                  |  419 +++++
 drivers/misc/bcm-vk/bcm_vk_dev.c              | 1357 +++++++++++++++
 drivers/misc/bcm-vk/bcm_vk_msg.c              | 1504 +++++++++++++++++
 drivers/misc/bcm-vk/bcm_vk_msg.h              |  211 +++
 drivers/misc/bcm-vk/bcm_vk_sg.c               |  275 +++
 drivers/misc/bcm-vk/bcm_vk_sg.h               |   61 +
 drivers/misc/bcm-vk/bcm_vk_tty.c              |  352 ++++
 fs/exec.c                                     |   92 +-
 include/linux/firmware.h                      |   12 +
 include/linux/fs.h                            |   39 -
 include/linux/ima.h                           |    1 +
 include/linux/kernel_read_file.h              |   69 +
 include/linux/security.h                      |    1 +
 include/uapi/linux/misc/bcm_vk.h              |   99 ++
 kernel/kexec_file.c                           |    1 +
 kernel/module.c                               |    1 +
 lib/test_firmware.c                           |  154 +-
 security/integrity/digsig.c                   |    1 +
 security/integrity/ima/ima_fs.c               |    1 +
 security/integrity/ima/ima_main.c             |   25 +-
 security/integrity/ima/ima_policy.c           |    1 +
 security/loadpin/loadpin.c                    |    1 +
 security/security.c                           |    1 +
 security/selinux/hooks.c                      |    1 +
 .../selftests/firmware/fw_filesystem.sh       |   80 +
 32 files changed, 4802 insertions(+), 91 deletions(-)
 create mode 100644 drivers/misc/bcm-vk/Kconfig
 create mode 100644 drivers/misc/bcm-vk/Makefile
 create mode 100644 drivers/misc/bcm-vk/bcm_vk.h
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_dev.c
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_msg.c
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_msg.h
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_sg.c
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_sg.h
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_tty.c
 create mode 100644 include/linux/kernel_read_file.h
 create mode 100644 include/uapi/linux/misc/bcm_vk.h

Comments

Kees Cook July 8, 2020, 12:03 a.m. UTC | #1
On Mon, Jul 06, 2020 at 04:23:07PM -0700, Scott Branden wrote:
> Add Broadcom VK driver offload engine.
> This driver interfaces to the VK PCIe offload engine to perform
> should offload functions as video transcoding on multiple streams
> in parallel.  VK device is booted from files loaded using
> request_firmware_into_buf mechanism.  After booted card status is updated
> and messages can then be sent to the card.
> Such messages contain scatter gather list of addresses
> to pull data from the host to perform operations on.
> 
> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
> Signed-off-by: Desmond Yan <desmond.yan@broadcom.com>

nit: your S-o-b chain doesn't make sense (I would expect you at the end
since you're sending it and showing as the Author). Is it Co-developed-by?
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

> [...]
> +
> +		max_buf = SZ_4M;
> +		bufp = dma_alloc_coherent(dev,
> +					  max_buf,
> +					  &boot_dma_addr, GFP_KERNEL);
> +		if (!bufp) {
> +			dev_err(dev, "Error allocating 0x%zx\n", max_buf);
> +			ret = -ENOMEM;
> +			goto err_buf_out;
> +		}
> +
> +		bcm_vk_buf_notify(vk, bufp, boot_dma_addr, max_buf);
> +	} else {
> +		dev_err(dev, "Error invalid image type 0x%x\n", load_type);
> +		ret = -EINVAL;
> +		goto err_buf_out;
> +	}
> +
> +	ret = request_partial_firmware_into_buf(&fw, filename, dev,
> +						bufp, max_buf, 0);

Unless I don't understand what's happening here, this needs to be
reordered if you're going to keep Mimi happy and disallow the device
being able to see the firmware before it has been verified. (i.e. please
load the firmware before mapping DMA across the buffer.)
Scott Branden July 8, 2020, 4:30 a.m. UTC | #2
On 2020-07-07 5:03 p.m., Kees Cook wrote:
> On Mon, Jul 06, 2020 at 04:23:07PM -0700, Scott Branden wrote:
>> Add Broadcom VK driver offload engine.
>> This driver interfaces to the VK PCIe offload engine to perform
>> should offload functions as video transcoding on multiple streams
>> in parallel.  VK device is booted from files loaded using
>> request_firmware_into_buf mechanism.  After booted card status is updated
>> and messages can then be sent to the card.
>> Such messages contain scatter gather list of addresses
>> to pull data from the host to perform operations on.
>>
>> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
>> Signed-off-by: Desmond Yan <desmond.yan@broadcom.com>
> nit: your S-o-b chain doesn't make sense (I would expect you at the end
> since you're sending it and showing as the Author). Is it Co-developed-by?
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
Yes, Co-developed-by.  Will adjust.
>
>> [...]
>> +
>> +		max_buf = SZ_4M;
>> +		bufp = dma_alloc_coherent(dev,
>> +					  max_buf,
>> +					  &boot_dma_addr, GFP_KERNEL);
>> +		if (!bufp) {
>> +			dev_err(dev, "Error allocating 0x%zx\n", max_buf);
>> +			ret = -ENOMEM;
>> +			goto err_buf_out;
>> +		}
>> +
>> +		bcm_vk_buf_notify(vk, bufp, boot_dma_addr, max_buf);
>> +	} else {
>> +		dev_err(dev, "Error invalid image type 0x%x\n", load_type);
>> +		ret = -EINVAL;
>> +		goto err_buf_out;
>> +	}
>> +
>> +	ret = request_partial_firmware_into_buf(&fw, filename, dev,
>> +						bufp, max_buf, 0);
> Unless I don't understand what's happening here, this needs to be
> reordered if you're going to keep Mimi happy and disallow the device
> being able to see the firmware before it has been verified. (i.e. please
> load the firmware before mapping DMA across the buffer.)
I don't understand your concern here.  We request partial firmware into 
a buffer that we allocated.
After loading it we signal the card the firmware has been loaded into 
that memory region.
The card then pulls the data into its internal memory.  And, 
authenticates it.

Even if the card randomly read and writes to that buffer it shouldn't 
matter to the linux kernel security subsystem.
It passed the security check already when placed in the buffer.
If there is a concern could we add an "nosecurity" 
request_partial_firmware_into_buf instead as there is no need for any 
security on this particular request?
Florian Fainelli July 8, 2020, 4:38 a.m. UTC | #3
On 7/6/2020 4:23 PM, Scott Branden wrote:
> This patch series adds partial read support via a new call
> request_partial_firmware_into_buf.
> Such support is needed when the whole file is not needed and/or
> only a smaller portion of the file will fit into allocated memory
> at any one time.
> In order to accept the enhanced API it has been requested that kernel
> selftests and upstreamed driver utilize the API enhancement and so
> are included in this patch series.
> 
> Also in this patch series is the addition of a new Broadcom VK driver
> utilizing the new request_firmware_into_buf enhanced API.
> 
> Further comment followed to add IMA support of the partial reads
> originating from request_firmware_into_buf calls.  And another request
> to move existing kernel_read_file* functions to its own include file.

Do you have any way to separate the VK drivers submission from the
request_partial_firmware_into_buf() work that you are doing? It looks
like it is going to require quite a few iterations of this patch set for
the firmware/fs/IMA part to be ironed out, so if you could get your
driver separated out, it might help you achieve partial success here.
Scott Branden July 8, 2020, 4:51 a.m. UTC | #4
Hi Florian,

On 2020-07-07 9:38 p.m., Florian Fainelli wrote:
>
> On 7/6/2020 4:23 PM, Scott Branden wrote:
>> This patch series adds partial read support via a new call
>> request_partial_firmware_into_buf.
>> Such support is needed when the whole file is not needed and/or
>> only a smaller portion of the file will fit into allocated memory
>> at any one time.
>> In order to accept the enhanced API it has been requested that kernel
>> selftests and upstreamed driver utilize the API enhancement and so
>> are included in this patch series.
>>
>> Also in this patch series is the addition of a new Broadcom VK driver
>> utilizing the new request_firmware_into_buf enhanced API.
>>
>> Further comment followed to add IMA support of the partial reads
>> originating from request_firmware_into_buf calls.  And another request
>> to move existing kernel_read_file* functions to its own include file.
> Do you have any way to separate the VK drivers submission from the
> request_partial_firmware_into_buf() work that you are doing? It looks
> like it is going to require quite a few iterations of this patch set for
> the firmware/fs/IMA part to be ironed out, so if you could get your
> driver separated out, it might help you achieve partial success here.
Originally I did not submit the driver.
But Greg K-H rejected the pread support unless there was an actual user 
in the kernel.
Hence the need to submit this all in the patch series.