[PATCH net 0/2] can: bittiming: fix two defects in the userspace bitrate conversion

[PATCH net 0/2] can: bittiming: fix two defects in the userspace bitrate conversion

[Sureshkumar S]

can_calc_bittiming() converts a userspace supplied bitrate into hardware
timing parameters. Two defects in that conversion are fixed here, both
reachable through IFLA_CAN_BITTIMING on any CAN device that provides a
bittiming_const, and both caused by 32 bit arithmetic on a value that
userspace fully controls.

Patch 1 fixes a divide-by-zero. The 32 bit tsegall * bt->bitrate product
wraps to zero for bitrates carrying enough factors of two, and the
following division faults. A bitrate of 16777216 is already enough,
which is below the 20 Mbit/s CAN XL data bitrate ceiling.

Patch 2 fixes the candidate rating, which applies abs() to an unsigned
subtraction. A wrapped difference is read back as a small error, so a
bitrate far outside what the controller can reach passes the 5% gate and
is silently accepted instead of rejected.

Both were found with a netlink fuzzer against dummy_can and verified
under KASAN in QEMU. Testing covered the unpatched tree, patch 1 alone
and the full series: bitrates from 125 kbit/s to 10 Mbit/s produce
byte-identical bittiming at every step, patch 1 alone removes the oops,
and patch 2 turns the silently accepted out-of-range bitrates into
-EINVAL.

Sureshkumar S (2):
  can: bittiming: fix divide-by-zero in can_calc_bittiming()
  can: bittiming: fix bitrate error calculation on unsigned operands

 drivers/net/can/dev/calc_bittiming.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)


base-commit: af39eb111ce6b5eba9c08513b62c4868eb7e7fd5
-- 
2.43.0

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

[Sureshkumar S]

can_calc_bittiming() scans the possible time segment combinations and
computes the prescaler for each of them as:

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

bt->bitrate is supplied by userspace via IFLA_CAN_BITTIMING and
tsegall * bt->bitrate is a 32 bit multiplication, so the product wraps
to zero as soon as bt->bitrate carries enough factors of two for the
tsegall values walked by the loop. The division then faults:

  Oops: divide error: 0000 [#1] SMP KASAN NOPTI
  RIP: 0010:can_calc_bittiming+0x32e/0xcc0
  Call Trace:
   can_changelink+0x8ba/0x2060
   __rtnl_newlink+0x1013/0x18a0
   rtnl_newlink+0x6b/0xa0
   rtnetlink_rcv_msg+0x6f9/0xb70
   netlink_rcv_skb+0x11f/0x350
   netlink_unicast+0x5f5/0x860
   netlink_sendmsg+0x70a/0xba0

This does not require an absurd bitrate. With the segment limits of a
typical controller tsegall reaches 256, so a bitrate of 16777216 is
already enough to wrap the product, and that value is below the
20 Mbit/s CAN XL data bitrate ceiling. Any CAN driver providing a
bittiming_const is affected; reproduced on dummy_can. Triggering it
needs CAP_NET_ADMIN in the netns owning the device.

priv->bitrate_max cannot guard against this: it is populated from the
optional "max-bitrate" device tree property, so it is zero for most
drivers, and can_changelink() only consults it after can_get_bittiming()
has already returned.

Compute the product with mul_u32_u32(), as can_fixup_bittiming() already
does for bt->brp * NSEC_PER_SEC, and divide with div64_u64(). div_u64()
cannot be used here because its divisor is a u32, which would truncate
the product back to the faulting value.

Fixes: 39549eef3587 ("can: CAN Network device driver and Netlink interface")
Signed-off-by: Sureshkumar S <ssureshmsd7@gmail.com>
---
 drivers/net/can/dev/calc_bittiming.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/dev/calc_bittiming.c b/drivers/net/can/dev/calc_bittiming.c
index 42498e9d3f38..4809f5e0c96e 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;
-- 
2.43.0

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

[Sureshkumar S]

can_calc_bittiming() rates each candidate against the requested bitrate
with:

	bitrate_error = abs(bt->bitrate - bitrate);

Both operands are unsigned int, so the subtraction wraps instead of
becoming negative, and abs() resolves an unsigned int argument to its
int branch. A wrapped difference is therefore reinterpreted as a small
positive value instead of the large error it actually represents.

A requested bitrate far above anything the controller can reach then
passes the CAN_CALC_MAX_ERROR gate. On a dummy_can device with a 160 MHz
clock, requesting 4294967294 bps reports an error of 0.01%, configures
415584 bps and returns success to userspace, where -EINVAL is expected.

Use abs_diff(), which subtracts the smaller operand from the larger one
and keeps the whole comparison unsigned.

Fixes: 7da29f97d6c8 ("can: dev: can-calc-bit-timing(): better sample point calculation")
Signed-off-by: Sureshkumar S <ssureshmsd7@gmail.com>
---
 drivers/net/can/dev/calc_bittiming.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/dev/calc_bittiming.c b/drivers/net/can/dev/calc_bittiming.c
index 4809f5e0c96e..2789b99ab6a8 100644
--- a/drivers/net/can/dev/calc_bittiming.c
+++ b/drivers/net/can/dev/calc_bittiming.c
@@ -132,7 +132,7 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
 			continue;
 
 		bitrate = priv->clock.freq / (brp * tsegall);
-		bitrate_error = abs(bt->bitrate - bitrate);
+		bitrate_error = abs_diff(bt->bitrate, bitrate);
 
 		/* tseg brp biterror */
 		if (bitrate_error > best_bitrate_error)
-- 
2.43.0

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

[sashiko-bot]

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 <ssureshmsd7@gmail.com>

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/20260803091426.29050-1-ssureshmsd7@gmail.com?part=2

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

[sashiko-bot]

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 <ssureshmsd7@gmail.com>

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/20260803091426.29050-1-ssureshmsd7@gmail.com?part=1