[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