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

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.
--

--- 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