Re: [PATCH net] can: ctucanfd: use self-test mode for PRESUME_ACK

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

Pre-existing issues:
- [Critical] Missing bounds check on `RWCNT` when calculating RX frame length can lead to kernel heap overflow.
--

--- Patch [1]: [PATCH net] can: ctucanfd: use self-test mode for PRESUME_ACK ---
commit 84cf60cfa3d4dbe2be47f85be01d189d34ce05a7
Author: Avi Weiss <[email protected]>

can: ctucanfd: use self-test mode for PRESUME_ACK

This commit uses self-test mode for CAN_CTRLMODE_PRESUME_ACK so transmitted
frames can complete without receiving an ACK. It fixes an issue where the ACK
forbidden mode prevented the controller from acknowledging received frames.

> diff --git a/drivers/net/can/ctucanfd/ctucanfd_base.c b/drivers/net/can/ctucanfd/ctucanfd_base.c
> index 0ea1ff28dfce8..8e0a606f2c84e 100644
> --- a/drivers/net/can/ctucanfd/ctucanfd_base.c
> +++ b/drivers/net/can/ctucanfd/ctucanfd_base.c
> @@ -340,8 +340,8 @@ static void ctucan_set_mode(struct ctucan_priv *priv, const struct can_ctrlmode
>  			(mode_reg & ~REG_MODE_FDE);
>  
>  	mode_reg = (mode->flags & CAN_CTRLMODE_PRESUME_ACK) ?
> -			(mode_reg | REG_MODE_ACF) :
> -			(mode_reg & ~REG_MODE_ACF);
> +			(mode_reg | REG_MODE_STM) :
> +			(mode_reg & ~REG_MODE_STM);
>  
>  	mode_reg = (mode->flags & CAN_CTRLMODE_FD_NON_ISO) ?
>  			(mode_reg | REG_MODE_NISOFD) :

[Severity: Critical]
This is a pre-existing issue, but can this code in ctucan_read_rx_frame()
cause a heap buffer overflow?

drivers/net/can/ctucanfd/ctucanfd_base.c:ctucan_read_rx_frame() {
    ...
	wc = FIELD_GET(REG_FRAME_FORMAT_W_RWCNT, ffw) - 3;

	/* DLC */
	if (FIELD_GET(REG_FRAME_FORMAT_W_DLC, ffw) <= 8) {
		len = FIELD_GET(REG_FRAME_FORMAT_W_DLC, ffw);
	} else {
		if (FIELD_GET(REG_FRAME_FORMAT_W_FDF, ffw))
			len = wc << 2;
		else
			len = 8;
	}
	cf->len = len;
	if (unlikely(len > wc * 4))
		len = wc * 4;
    ...
}

If the hardware returns RWCNT < 3, doesn't wc underflow to a very large
unsigned integer?

This appears to allow len to become large enough to cause the subsequent loop
to write out-of-bounds. Furthermore, if RWCNT is between 20 and 31, wc could
be up to 28, making len up to 112.

Since cf->data only has space for 64 bytes (CANFD_MAX_DLEN), could this copy
more bytes than allocated and overflow the heap backing the SKB?

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