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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing bounds checking on `bittiming_const` parameters allows malicious USB devices to trigger divide-by-zero crashes or infinite loops in `can_calc_bittiming()`.
--

commit 0fc6576c2e9b35990a8bf86f43e72c0d477992ea
Author: Sureshkumar S <ssureshmsd7@gmail.com>

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

This commit prevents a divide-by-zero panic in can_calc_bittiming() when
evaluating prescalers for large bitrates. It does this by computing the
product of tsegall and bt->bitrate in 64 bits to avoid wrap-around.

Link: https://patch.msgid.link/20260803091426.29050-2-ssureshmsd7@gmail.com

> 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--) {

[Severity: High]
This is a pre-existing issue, but if a malicious CAN USB device (such as one
using the gs_usb driver) provides a USB descriptor where btc->tseg1_min and
btc->tseg2_min are both 0, the unsigned loop condition (tseg >= 0) becomes
always true.

Could this result in an integer wrap-around and billions of loop iterations,
leading to a system freeze (soft lockup)?

Since can_bittiming_const_valid() in drivers/net/can/dev/dev.c validates these
constants, should it be updated to verify that tseg1_min and tseg2_min are
non-zero?

>  		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 isn't a bug introduced by this patch, but if btc->brp_inc is 0 due to an
unvalidated value from the USB device, would this calculation cause an
immediate divide-by-zero panic?

Additionally, if btc->brp_min is 0, brp can evaluate to 0 in some loop
iterations. Looking further down in can_calc_bittiming():

		bitrate = priv->clock.freq / (brp * tsegall);

Can this lead to another divide-by-zero in the same loop?

Similarly, if fclk_can (mapped to priv->clock.freq) is provided as 0 by the
device, would the following call near the end of can_calc_bittiming() also
result in a divide-by-zero panic?

	do_div(v64, priv->clock.freq);

Would it make sense to enhance can_bittiming_const_valid() to ensure that
parameters like brp_inc, brp_min, and clock.freq are strictly greater than
zero before they are used here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826121036.2706424-1-mkl@pengutronix.de?part=3