Messages in this thread | | | Date | Wed, 21 Dec 2022 11:14:59 -0800 (PST) | From | matthew.gerlach@linux ... | Subject | Re: [PATCH v7 3/4] fpga: dfl: add basic support for DFHv1 |
| |
On Tue, 20 Dec 2022, Andy Shevchenko wrote:
> On Tue, Dec 20, 2022 at 08:36:51AM -0800, matthew.gerlach@linux.intel.com wrote: >> From: Matthew Gerlach <matthew.gerlach@linux.intel.com> >> >> Version 1 of the Device Feature Header (DFH) definition adds >> functionality to the DFL bus. >> >> A DFHv1 header may have one or more parameter blocks that >> further describes the HW to SW. Add support to the DFL bus >> to parse the MSI-X parameter. >> >> The location of a feature's register set is explicitly >> described in DFHv1 and can be relative to the base of the DFHv1 >> or an absolute address. Parse the location and pass the information >> to DFL driver. > > ... > >> +/** >> + * dfh_find_param() - find data for the given parameter id >> + * @dfl_dev: dfl device >> + * @param: id of dfl parameter >> + * >> + * Return: pointer to parameter header on success, NULL otherwise. > > header is a bit confusing here, does it mean we give and ID and we got > something more than just a data as summary above suggests?
Yes, the summary is not correct. It should say "find the parameter block for the given parameter id".
> > In such case summary and this text should clarify what exactly we get > and layout of the data. Since this is a pointer, who is responsible of > checking out-of-boundary accesses? For instance, if the parameters are > variadic by length the length should be returned as well. Otherwise it > should be specified as a constant somewhere, right?
The parameter header has the next/size field; so the caller of dfh_find_param should perform boundary checking as part of interpreting the parameter data. I think a function to perform this checking and data interpretation would help here.
> >> + */ >> +u64 *dfh_find_param(struct dfl_device *dfl_dev, int param_id) >> +{ >> + return find_param(dfl_dev->params, dfl_dev->param_size, param_id); >> +} >> +EXPORT_SYMBOL_GPL(dfh_find_param); > > ... > >> + finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL); > > It sounds like a candidate for struct_size() from overflow.h. > I.o.w. check that header and come up with the best what can > suit your case.
finfo = kzalloc(struct_size(finfo, params, dfh_psize/sizeof(u64)), GFP_KERNEL);
Does seem better.
Thanks for the suggestion, Matthew Gerlach
> >> if (!finfo) >> return -ENOMEM; > > -- > With Best Regards, > Andy Shevchenko > > >
| |