Re: [PATCH net 1/2] can: bittiming: fix divide-by-zero in can_calc_bittiming()

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

Pre-existing issues:
- [High] Missing validation for hardware-provided bit-timing constants `brp_inc` and `brp_min` allows malicious or misbehaving CAN devices to trigger divide-by-zero kernel panics.
- [High] A 32-bit integer overflow in the `brp * tsegall` calculation can wrap to zero, causing a divide-by-zero kernel panic.
--

--- Patch [1]: [PATCH net 1/2] can: bittiming: fix divide-by-zero in can_calc_bittiming() ---
commit 1d93cc0a0370ce5feb4636b0a8c6900dd609c628
Author: Sureshkumar S <[email protected]>

can: bittiming: fix divide-by-zero in can_calc_bittiming()

This commit patches a potential divide-by-zero in can_calc_bittiming() when
calculating the prescaler. It uses 64-bit math to prevent the 32-bit integer
multiplication of tsegall and bitrate from wrapping to zero for large bitrates.

> diff --git a/drivers/net/can/dev/calc_bittiming.c b/drivers/net/can/dev/calc_bittiming.c
> index 42498e9d3f38d..4809f5e0c96e9 100644
> --- a/drivers/net/can/dev/calc_bittiming.c
> +++ b/drivers/net/can/dev/calc_bittiming.c
> @@ -119,8 +119,12 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
>  	     tseg >= (btc->tseg1_min + btc->tseg2_min) * 2; tseg--) {
>  		tsegall = CAN_SYNC_SEG + tseg / 2;
>  
> -		/* Compute all possible tseg choices (tseg=tseg1+tseg2) */
> -		brp = priv->clock.freq / (tsegall * bt->bitrate) + tseg % 2;
> +		/* Compute all possible tseg choices (tseg=tseg1+tseg2).
> +		 * A 32 bit tsegall * bt->bitrate can wrap to zero for large
> +		 * userspace bitrates, so compute the product in 64 bit.
> +		 */
> +		brp = div64_u64(priv->clock.freq,
> +				mul_u32_u32(tsegall, bt->bitrate)) + tseg % 2;
>  
>  		/* choose brp step which is possible in system */
>  		brp = (brp / btc->brp_inc) * btc->brp_inc;

[Severity: High]
This is a pre-existing issue, but missing validation for hardware-provided
bit-timing constants might allow a divide-by-zero here.

If a misbehaving or malicious USB adapter (like gs_usb) provides 0 for
btc->brp_inc, won't this division directly panic the kernel when a user with
CAP_NET_ADMIN configures the interface?

>  		if (brp < btc->brp_min || brp > btc->brp_max)
>  			continue;
>  
>  		bitrate = priv->clock.freq / (brp * tsegall);

[Severity: High]
This isn't a bug introduced by this patch, but doesn't this calculation have
the exact same 32-bit overflow vulnerability that the patch fixes above?

Since both brp and tsegall are 32-bit unsigned integers, couldn't their product
wrap to zero for large values derived from untrusted hardware limits, causing a
divide-by-zero panic on this line?

Additionally, if the hardware provides a btc->brp_min of 0, the bounds
check above allows brp to evaluate to 0. Wouldn't that also result in a
divide-by-zero here?

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