Re: [PATCH net 2/2] can: bittiming: fix bitrate error calculation on unsigned operands

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

Pre-existing issues:
- [High] Unchecked bit-timing hardware constraints from USB devices can cause kernel panics due to division by zero in can_calc_bittiming().
- [High] An unchecked minimum time segment configuration from a malicious USB CAN device leads to an infinite loop, hanging the kernel.
--

--- Patch [2]: [PATCH net 2/2] can: bittiming: fix bitrate error calculation on unsigned operands ---
commit 93441c10d7d0dd29814ef57910bbfdffcd4e5112
Author: Sureshkumar S <[email protected]>

can: bittiming: fix bitrate error calculation on unsigned operands

This patch fixes a wrap-around issue in can_calc_bittiming() caused by
subtracting unsigned operands when rating candidate bitrates. It updates
the code to use abs_diff() so that large bitrate errors are correctly
identified rather than being misinterpreted as small errors.

> diff --git a/drivers/net/can/dev/calc_bittiming.c b/drivers/net/can/dev/calc_bittiming.c
> index 4809f5e0c96e9..2789b99ab6a83 100644
> --- a/drivers/net/can/dev/calc_bittiming.c
> +++ b/drivers/net/can/dev/calc_bittiming.c

[Severity: High]
This isn't a bug introduced by this patch, but could a malicious USB CAN
device cause an infinite loop in this function?

Looking at can_calc_bittiming(), if a device advertises tseg1_min = 0 and
tseg2_min = 0:

	for (tseg = (btc->tseg1_max + btc->tseg2_max) * 2 + 1;
	     tseg >= (btc->tseg1_min + btc->tseg2_min) * 2; tseg--) {

Since tseg is an unsigned int, the loop condition tseg >= 0 becomes a
tautology. This would cause tseg to wrap around to UINT_MAX, resulting in
an infinite loop that hangs the kernel.

> @@ -132,7 +132,7 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
>  			continue;
>  
>  		bitrate = priv->clock.freq / (brp * tsegall);

[Severity: High]
This is also a pre-existing issue not introduced by this patch, but is it
possible for this division to cause a kernel panic?

If a malicious USB device provides unvalidated values, could brp evaluate
to 0 if btc->brp_min == 0 or priv->clock.freq == 0?

Also, earlier in the same function there is another division:

		brp = (brp / btc->brp_inc) * btc->brp_inc;

Could this divide by zero if btc->brp_inc is provided as 0 by the hardware?

> -		bitrate_error = abs(bt->bitrate - bitrate);
> +		bitrate_error = abs_diff(bt->bitrate, bitrate);
>  
>  		/* tseg brp biterror */
>  		if (bitrate_error > best_bitrate_error)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2