| Message ID | 20200706232309.12010-1-scott.branden@broadcom.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | firmware: add request_partial_firmware_into_buf | expand |
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.)
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?
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.
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.