Re: [PATCH net] can: netlink: fix CAN XL PWM attribute sizing

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] can_get_size() underestimates the netlink message size by not using nla_total_size() for the IFLA_CAN_BITRATE_MAX attribute.
--

commit 65a98e250b77c16c65b20dbe5d32fe88f1c06e1b
Author: Zhu Ling <zhuling0805@qq.com>

can: netlink: fix CAN XL PWM attribute sizing

can_pwm_fill_info() emits the PWM capability limits whenever pwm_const is
present, regardless of whether TMS is enabled. In contrast,
can_pwm_get_size() currently returns zero when TMS is disabled. It
therefore underestimates the required netlink message size by the nested
attribute header and six u32 attributes.

Always account for the PWM capability attributes when pwm_const is
present, and account for the three runtime PWM values only when TMS is
enabled.

> diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> index 766d455950f55..0eac5a3f577ec 100644
> --- a/drivers/net/can/dev/netlink.c
> +++ b/drivers/net/can/dev/netlink.c
> @@ -780,7 +780,7 @@ static size_t can_pwm_get_size(const struct can_pwm_const *pwm_const,
>  {
>  	size_t size;
>  
> -	if (!pwm_const || !pwm_on)
> +	if (!pwm_const)
>  		return 0;
>  
>  	size = nla_total_size(0);			/* nest IFLA_CAN_PWM */

[ ... ]

> @@ -792,9 +792,11 @@ static size_t can_pwm_get_size(const struct can_pwm_const *pwm_const,
>  	size += nla_total_size(sizeof(u32));		/* IFLA_CAN_PWM_PWMO_MIN */
>  	size += nla_total_size(sizeof(u32));		/* IFLA_CAN_PWM_PWMO_MAX */
>  
> -	size += nla_total_size(sizeof(u32));		/* IFLA_CAN_PWM_PWMS */
> -	size += nla_total_size(sizeof(u32));		/* IFLA_CAN_PWM_PWML */
> -	size += nla_total_size(sizeof(u32));		/* IFLA_CAN_PWM_PWMO */
> +	if (pwm_on) {
> +		size += nla_total_size(sizeof(u32));	/* IFLA_CAN_PWM_PWMS */
> +		size += nla_total_size(sizeof(u32));	/* IFLA_CAN_PWM_PWML */
> +		size += nla_total_size(sizeof(u32));	/* IFLA_CAN_PWM_PWMO */
> +	}
>  
>  	return size;
>  }

[Severity: High]
This is a pre-existing issue, but while looking at the netlink message size
calculations, I noticed a potential under-allocation in the caller:

drivers/net/can/dev/netlink.c:can_get_size() {
	...
	if (priv->bitrate_const)				/* IFLA_CAN_BITRATE_CONST */
		size += nla_total_size(sizeof(*priv->bitrate_const) *
				       priv->bitrate_const_cnt);
	size += sizeof(priv->bitrate_max);			/* IFLA_CAN_BITRATE_MAX */
	size += can_ctrlmode_ext_get_size();			/* IFLA_CAN_CTRLMODE_EXT */
	...
}

Does the IFLA_CAN_BITRATE_MAX size calculation underestimate the required
netlink message size? It appears to add only the payload size instead of
wrapping it in nla_total_size() to account for the NLA header.

If the allocation doesn't happen to have enough slack to cover the calculation
error, couldn't this cause userspace netlink requests (like `ip link show`) to
randomly fail with -EMSGSIZE and truncate the response?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_9A525E0656A4F7D4CA9CFA1AB1A4BFC6D706@qq.com?part=1