Re: [PATCH net-next v2 9/9] devlink: validate the port index in the rate set request

[netdev-bot+sashiko@kernel.org]

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

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

- [Low] Commit-message characterization / missing `Fixes:` tag.

--

devlink: validate the port index in the rate set request

This adds port-index to the rate-set request attribute list in the devlink
YAML spec and adds the matching NLA_U32 entry to the generated
devlink_rate_set_nl_policy. rate-new is deliberately left unchanged since
it resolves rate nodes by name.

> diff --git a/net/devlink/netlink_gen.c b/net/devlink/netlink_gen.c
> index 43ef6864d462f..9e1b4c081a7b3 100644
> --- a/net/devlink/netlink_gen.c
> +++ b/net/devlink/netlink_gen.c
> @@ -618,6 +618,7 @@ static const struct nla_policy devlink_rate_set_nl_policy[DEVLINK_ATTR_PARENT_DE
>  	[DEVLINK_ATTR_BUS_NAME] = { .type = NLA_NUL_STRING, },
>  	[DEVLINK_ATTR_DEV_NAME] = { .type = NLA_NUL_STRING, },
>  	[DEVLINK_ATTR_INDEX] = NLA_POLICY_FULL_RANGE(NLA_UINT, &devlink_attr_index_range),
> +	[DEVLINK_ATTR_PORT_INDEX] = { .type = NLA_U32, },

[Severity: Low]
This isn't a bug in the change itself, but should this carry a Fixes tag?

The missing policy slot looks bisectable to f2f9dd164db0 ("netlink: specs:
devlink: add the remaining command to generate complete split_ops"), which
moved DEVLINK_CMD_RATE_SET out of small_ops into split_ops. The old
small_ops path used the global devlink_nl_policy in net/devlink/netlink.c,
which does declare:

	[DEVLINK_ATTR_PORT_INDEX] = { .type = NLA_U32 },

so before that conversion a short port-index was rejected, and afterwards
it is not.

The commit message says:

	Since we're reading a linear skb the OOB read is still
	within the same memory allocation, it's just garbage. We also do not
	echo the garbage back to the user so it's not an info leak either.
	Hence not treating this is a real bug fix.

That reasoning checks out, and the op is GENL_ADMIN_PERM so only a caller
that can already reconfigure any port reaches it. The one remaining effect
is behavioural rather than memory-safety related: with the zeroed policy
slot plus GENL_DONT_VALIDATE_STRICT, a zero-length port-index passes
validation, devlink_rate_get_from_info() picks the leaf path purely on
attribute presence:

net/devlink/rate.c:devlink_rate_get_from_info() {
	if (attrs[DEVLINK_ATTR_PORT_INDEX])
		return devlink_rate_leaf_get_from_info(devlink, info);
	...
}

and devlink_port_get_from_attrs() then reads past the payload:

net/devlink/port.c:devlink_port_get_from_attrs() {
	u32 port_index = nla_get_u32(attrs[DEVLINK_ATTR_PORT_INDEX]);
	...
}

If those four bytes happen to name a registered port,
devlink_nl_rate_set_doit() goes on to change that leaf's rate settings
instead of returning -EINVAL. Would a Fixes tag help stable/backport
tooling identify the trees (v6.7 onward) where a malformed rate-set request
is acted upon rather than rejected?

>  	[DEVLINK_ATTR_RATE_NODE_NAME] = { .type = NLA_NUL_STRING, },
>  	[DEVLINK_ATTR_RATE_TX_SHARE] = { .type = NLA_U64, },
>  	[DEVLINK_ATTR_RATE_TX_MAX] = { .type = NLA_U64, },

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915161341.1053476-1-kuba%40kernel.org