| Message ID | 20200508002739.19360-1-scott.branden@broadcom.com |
|---|---|
| Headers | show |
| Series | firmware: add partial read support in request_firmware_into_buf | expand |
Hi Luis, On 2020-05-12 5:38 p.m., Luis Chamberlain wrote: > On Thu, May 07, 2020 at 05:27:38PM -0700, Scott Branden wrote: >> +#if defined(CONFIG_REQ_FW_INTO_BUF_PRIV) >> + >> +#define KERNEL_PREAD_FLAG_PART 0x0001 /* Allow reading part of file */ >> +#define REQUEST_FIRMWARE_INTO_BUF request_firmware_into_buf_priv >> +int request_firmware_into_buf_priv(const struct firmware **firmware_p, >> + const char *name, struct device *device, >> + void *buf, size_t size, >> + size_t offset, unsigned int pread_flags); >> + >> +#else >> + >> +#define REQUEST_FIRMWARE_INTO_BUF request_firmware_into_buf >> + >> +#endif >> + >> +#endif > Please clean this up, the code must reflect only the code upstream. No > config stuff like this should be used on the driver. I had to stop my > review here. The CONFIG_ prefix shouldn't have been there as there is no Kconfig option to select this. Would like to just change it to a normal define without CONFIG_ prefix instead? This code is here to allow a limited version of the driver to run on older kernels which do not have the necessary partial file read support. By having it in the upstream codebase we don't need to maintain an internal version of the driver. User can take the upstream kernel module and compile it against an old version of the kernel by via the define. > Luis Regards, Scott
On Wed, May 13, 2020 at 11:35:06AM -0700, Scott Branden wrote: > On 2020-05-12 5:33 p.m., Luis Chamberlain wrote: > > On Thu, May 07, 2020 at 05:27:34PM -0700, Scott Branden wrote: > > flags? But its a single variable enum! > fw_opt is an existing enum which doesn't really act like an enum. > It is a series of BIT defines in an enum that are then OR'd together in the > (existing) code? Indeed, in retrospect that is odd, it should be a u32 then. Please feel free to fix. Luis
On Fri, May 15, 2020 at 04:28:08PM -0700, Scott Branden wrote: > Hi Luis, > > On 2020-05-15 1:47 p.m., Luis Chamberlain wrote: > > On Wed, May 13, 2020 at 12:23:59PM -0400, Mimi Zohar wrote: > > > Hi Scott, > > > > > > On Thu, 2020-05-07 at 17:27 -0700, Scott Branden wrote: > > > > Please consider this version series ready for upstream acceptance. > > > > > > > > This patch series adds partial read support in request_firmware_into_buf. > > > > 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. > > > Up to now, the firmware blob was read into memory allowing IMA to > > > verify the file signature. With this change, ima_post_read_file() > > > will not be able to verify the file signature. > > > > > > (I don't think any of the other LSMs are on this hook, but you might > > > want to Cc the LSM or integrity mailing list.) > > Scott, so it sounds we need a resolution for pread for IMA for file > > signature verification. It seems that can be addressed though. Feel > > free to submit the u32 flag changes which you picked up on though in > > the meantime. > Sure, I can submit a new patch to change the existing enum to > a u32. Great thanks! > For the new pread flags I am adding I could also leave as > a u32 or change from a u32 to an enum since there is currently only > one flag in use. Then, in the future if another flag was added we would > need > to change it back to a u32. Yes that approach works well, enum for now. Luis