Re: [PATCH v1 1/1] fpga: m10bmc-sec: add image_load sysfs for N3000 and D5005

On Wed, 15 Jul 2026, [email protected] wrote:

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Lack of synchronization in the `image_load` sysfs handler allows concurrent firmware reloads and races with ongoing secure firmware uploads.
> - [Medium] The 'control' sysfs group creates an empty directory even when the device does not support image loading, failing to completely hide the group.
> - [Low] The image load handler arrays and their pointer in `m10bmc_sec_ops` are not declared as `const`, storing function pointers in mutable memory.

Please show that you have at least considered these, or better mitigated them.

> --
> 
> --- Patch [1]: [PATCH v1 1/1] fpga: m10bmc-sec: add image_load sysfs for N3000 and D5005 ---
> commit d97d9214f834756bd204bc187982a995a0b7658d
> Author: Ionut Nechita <[email protected]>
> 
> fpga: m10bmc-sec: add image_load sysfs for N3000 and D5005
> 
> This commit adds the control/available_images and control/image_load
> sysfs attributes to trigger a reload of a BMC or PKVL retimer firmware
> image. It handles device-specific configuration for N3000 and D5005 cards.
> The control group is intended to be hidden when not supported.
> 
> > diff --git a/drivers/fpga/intel-m10-bmc-sec-update.c b/drivers/fpga/intel-m10-bmc-sec-update.c
> > index 7d23d914df3f2..7a390cd705161 100644
> > --- a/drivers/fpga/intel-m10-bmc-sec-update.c
> > +++ b/drivers/fpga/intel-m10-bmc-sec-update.c
> > @@ -15,8 +15,14 @@
> >  
> >  struct m10bmc_sec;
> >  
> > +struct image_load {
> > +	const char *name;
> > +	int (*load_image)(struct m10bmc_sec *sec);
> > +};
> > +
> >  struct m10bmc_sec_ops {
> >  	int (*rsu_status)(struct m10bmc_sec *sec);
> > +	struct image_load *image_load;		/* terminated with { } member */
> 
> [Severity: Low]
> Should this pointer be declared as const struct image_load * to ensure the
> function pointers remain in read-only memory?
> 
> >  };
> 
> [ ... ]
> 
> > +static struct image_load n3000_image_load_hndlrs[] = {
> > +	{
> > +		.name = "bmc_factory",
> > +		.load_image = m10bmc_sec_bmc_image_load_1,
> > +	},
> > +	{
> > +		.name = "bmc_user",
> > +		.load_image = m10bmc_sec_bmc_image_load_0,
> > +	},
> > +	{
> > +		.name = "retimer_fw",
> > +		.load_image = m10bmc_sec_retimer_eeprom_load,
> > +	},
> > +	{}
> > +};
> > +
> > +static struct image_load d5005_image_load_hndlrs[] = {
> > +	{
> > +		.name = "bmc_factory",
> > +		.load_image = m10bmc_sec_bmc_image_load_0,
> > +	},
> > +	{
> > +		.name = "bmc_user",
> > +		.load_image = m10bmc_sec_bmc_image_load_1,
> > +	},
> > +	{}
> > +};
> 
> [Severity: Low]
> Would it be better to mark these arrays as const? Placing executable
> function targets in mutable memory increases the kernel's attack surface.
> 
> [ ... ]
> 
> > +static ssize_t image_load_store(struct device *dev,
> > +				struct device_attribute *attr,
> > +				const char *buf, size_t count)
> > +{
> > +	struct m10bmc_sec *sec = dev_get_drvdata(dev);
> > +	const struct image_load *hndlr;
> > +	int ret = -EINVAL;
> > +
> > +	for (hndlr = sec->ops->image_load; hndlr->name; hndlr++) {
> > +		if (sysfs_streq(buf, hndlr->name)) {
> > +			ret = hndlr->load_image(sec);
> > +			break;
> > +		}
> > +	}
> > +
> > +	return ret ? : count;
> > +}
> > +static DEVICE_ATTR_WO(image_load);
> 
> [Severity: High]
> Does this sysfs handler need synchronization (such as acquiring bmcfw_lock)?
> 
> Without driver-level locking, concurrent writes to the image_load attribute
> could cause multi-step hardware operations to race. This might also race
> with ongoing secure firmware uploads in the background.
> 
> For example, m10bmc_sec_retimer_eeprom_load() performs a check-then-act
> sequence:
> 
> m10bmc_sec_retimer_eeprom_load()
>     retimer_check_idle(sec);
>     trigger_retimer_eeprom_load(sec);
>     poll_retimer_eeprom_load_done(sec);
> 
> Could concurrent executions silently corrupt the hardware doorbell state?
> 
> > +
> > +static umode_t
> > +m10bmc_control_visible(struct kobject *kobj, struct attribute *attr, int n)
> > +{
> > +	struct m10bmc_sec *sec = dev_get_drvdata(kobj_to_dev(kobj));
> > +
> > +	if (!sec->ops->image_load)
> > +		return 0;
> > +
> > +	return attr->mode;
> > +}
> 
> [Severity: Medium]
> The commit message states the control group is hidden for the N6000:
> "The control group is hidden via its is_visible() callback whenever the
> device ops do not provide an image_load table, so N6000 ... is unaffected."
> 
> However, does returning 0 here actually hide the group directory?
> 
> As per the sysfs API, returning 0 for a named group only suppresses its
> internal attributes, leaving an empty directory visible. Should this
> return SYSFS_GROUP_INVISIBLE to completely hide the control/ directory?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

-- 
Lee Jones