Re: [PATCH net-next v16 1/6] net: dsa: add devlink flash_update callback to dsa_switch_ops

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] Installing `.flash_update = dsa_devlink_flash_update`
  unconditionally in the single shared `dsa_devlink_ops` removes the…

--

net: dsa: add devlink flash_update callback to dsa_switch_ops

This adds a devlink_flash_update callback to dsa_switch_ops and a
trampoline in net/dsa/devlink.c that forwards to it, returning
-EOPNOTSUPP when the driver does not implement the callback. The
.flash_update op is installed unconditionally in the shared
dsa_devlink_ops table.

> diff --git a/net/dsa/devlink.c b/net/dsa/devlink.c
> index ed342f345692b..25311a87cbc56 100644
> --- a/net/dsa/devlink.c
> +++ b/net/dsa/devlink.c
> @@ -20,6 +20,18 @@ static int dsa_devlink_info_get(struct devlink *dl,
>  	return -EOPNOTSUPP;
>  }
>  
> +static int dsa_devlink_flash_update(struct devlink *dl,
> +				    struct devlink_flash_update_params *params,
> +				    struct netlink_ext_ack *extack)
> +{
> +	struct dsa_switch *ds = dsa_devlink_to_ds(dl);
> +
> +	if (!ds->ops->devlink_flash_update)
> +		return -EOPNOTSUPP;
> +
> +	return ds->ops->devlink_flash_update(ds, params, extack);
> +}
> +

[ ... ]

> @@ -169,6 +181,7 @@ dsa_devlink_sb_occ_tc_port_bind_get(struct devlink_port *dlp,
>  
>  static const struct devlink_ops dsa_devlink_ops = {
>  	.info_get			= dsa_devlink_info_get,
> +	.flash_update			= dsa_devlink_flash_update,

[Severity: Low]
This isn't a bug as such, since the commit message states the trade-off,
but is the full extent of the behaviour change on switches without the
callback covered?

Since dsa_devlink_ops is shared by every DSA switch, the core gate in
devlink_nl_flash_update_doit() now always passes:

net/devlink/dev.c:devlink_nl_flash_update_doit() {
	if (!devlink->ops->flash_update)
		return -EOPNOTSUPP;
	...
	file_name = nla_data(nla_file_name);
	ret = request_firmware_direct(&params.fw, file_name, devlink->dev);
	...
	devlink_flash_update_begin_notify(devlink);
	ret = devlink->ops->flash_update(devlink, &params, info->extack);
	devlink_flash_update_end_notify(devlink);
}

So on a switch that does not set ds->ops->devlink_flash_update, a flash
request now reads the entire userspace-named file from the firmware search
paths into kernel memory before dsa_devlink_flash_update() gets to return
-EOPNOTSUPP.

Beyond the -ENOENT versus -EOPNOTSUPP change already described, does this
also mean that on such switches a flash request becomes a file-existence
probe of the firmware search paths, performs an unbounded read of a
/lib/firmware file into kernel memory, and emits a FLASH_UPDATE /
FLASH_UPDATE_END pair to devlink monitors for an operation that can never
succeed?

For reference, path traversal is not a concern here:
_request_firmware() rejects names containing ".." via
name_contains_dotdot(), and the operation requires CAP_NET_ADMIN.

Would it be worth mentioning the firmware read and the notifications for
switches without the callback in the commit message, in addition to the
errno change?

>  	.sb_pool_get			= dsa_devlink_sb_pool_get,
>  	.sb_pool_set			= dsa_devlink_sb_pool_set,
>  	.sb_port_pool_get		= dsa_devlink_sb_port_pool_get,

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1789477568.git.daniel%40makrotopia.org