Re: [PATCH net 08/19] can: bcm: add locking when updating filter and timer values

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

New issues:
- [Medium] A race condition in bcm_can_tx's new heuristic skips the first frame of a restarted multi-frame sequence if the concurrent transmission happens to be at index 0.

Pre-existing issues:
- [High] Missing payload length validation for RX_RTR_FRAME operations allows out-of-bounds memory reads when transmitting replies.
- [High] Modifying the CAN_FD_FRAME flag on an existing BCM RX operation creates a mismatch between op->flags and op->cfsiz, leading to out-of-bounds reads and an information leak.
--

--- Patch [8]: [PATCH net 08/19] can: bcm: add locking when updating filter and timer values ---
commit c6674a9f75532d12e818868002b6c50a902d8b3e
Author: Oliver Hartkopp <[email protected]>

can: bcm: add locking when updating filter and timer values

This patch introduces a new per-op lock (bcm_rx_update_lock) to protect
concurrent accesses to timer and filter data during CAN traffic reception.
It also addresses race conditions related to modifying frames and ensures
consistent RTR frame preparation.

Link: https://patch.msgid.link/[email protected]

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index b612135b017d1..1e5f8d65d351b 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c

[ ... ]

> @@ -341,14 +348,20 @@ static void bcm_can_tx(struct bcm_op *op)
>  	if (!err)
>  		op->frames_abs++;
>  
> -	op->currframe++;
> +	/* only advance the cyclic sequence if nothing reset currframe while
> +	 * we were sending - a concurrent TX_RESET_MULTI_IDX means this
> +	 * frame's bookkeeping belongs to a sequence that no longer exists
> +	 */
> +	if (!cyclic || op->currframe == idx) {
> +		op->currframe++;

[Severity: Medium]
Does this new heuristic skip the first frame of a restarted multi-frame
sequence if the concurrent transmission happens to be at index 0?

If a concurrent bcm_tx_setup() resets the sequence by setting
op->currframe = 0 while bcm_can_tx() is actively transmitting frame
index 0 (where idx == 0), the condition will evaluate to true.
This would increment op->currframe to 1, causing the newly configured
sequence to incorrectly skip the first frame.

> -	/* reached last frame? */
> -	if (op->currframe >= op->nframes)
> -		op->currframe = 0;
> +		/* reached last frame? */
> +		if (op->currframe >= op->nframes)
> +			op->currframe = 0;

[ ... ]

> @@ -1130,6 +1173,24 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  	return err;
>  }
>  
> +static void bcm_rx_setup_rtr_check(struct bcm_msg_head *msg_head,
> +				   struct bcm_op *op, void *new_frames)
> +{
> +	/* funny feature in RX(!)_SETUP only for RTR-mode:
> +	 * copy can_id into frame BUT without RTR-flag to
> +	 * prevent a full-load-loopback-test ... ;-]
> +	 * normalize this on the staged buffer, before it is
> +	 * ever installed into op->frames.
> +	 */
> +	if (msg_head->flags & RX_RTR_FRAME) {

[Severity: High]
This is a pre-existing issue, but does bcm_rx_setup_rtr_check() fail to
validate the len field of the user-supplied RTR frame?

If a user configures an RX_RTR_FRAME operation without providing a valid
len field (for example, passing a payload length larger than the CAN limits
of 8 or 64), it appears the length goes unchecked before installation.
When a matching RTR request is received, bcm_rx_handler() will trigger
bcm_can_tx() to send this stored frame. The underlying CAN driver might
then trust the user-supplied length to read the payload from the buffer,
which could result in an out-of-bounds memory read.

> +		struct canfd_frame *frame0 = new_frames;
> +
> +		if ((msg_head->flags & TX_CP_CAN_ID) ||
> +		    frame0->can_id == op->can_id)
> +			frame0->can_id = op->can_id & ~CAN_RTR_FLAG;
> +	}
> +}

[ ... ]

> @@ -1175,19 +1238,48 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  			return -E2BIG;
>  
>  		if (msg_head->nframes) {
> -			/* update CAN frames content */
> -			err = memcpy_from_msg(op->frames, msg,
> +			/* get new CAN frames content before locking */
> +			new_frames = kmalloc(msg_head->nframes * op->cfsiz,
> +					     GFP_KERNEL);
> +			if (!new_frames)
> +				return -ENOMEM;
> +
> +			err = memcpy_from_msg(new_frames, msg,
>  					      msg_head->nframes * op->cfsiz);
> -			if (err < 0)
> +			if (err < 0) {
> +				kfree(new_frames);
>  				return err;
> +			}
>  
> -			/* clear last_frames to indicate 'nothing received' */
> -			memset(op->last_frames, 0, msg_head->nframes * op->cfsiz);
> +			bcm_rx_setup_rtr_check(msg_head, op, new_frames);
>  		}
>  
> +		spin_lock_bh(&op->bcm_rx_update_lock);
>  		op->nframes = msg_head->nframes;
>  		op->flags = msg_head->flags;

[Severity: High]
This is a pre-existing issue, but can updating an existing operation's
flags create a dangerous mismatch between op->flags and op->cfsiz?

By overwriting op->flags with the newly supplied flags while leaving
op->cfsiz at its previously configured value, the CAN_FD_FRAME state can
diverge from its statically determined allocation size.

For instance, if a user modifies an existing standard operation to include
the CAN_FD_FRAME flag, later checks like the one in bcm_rx_cmp_to_index()
might assume a larger layout and perform an out-of-bounds read past the
end of the heap-allocated op->frames buffer. Conversely, dropping the flag
could cause functions like bcm_rx_update_and_send() to copy an excessive
number of bytes from a smaller SKB buffer.

> +		if (msg_head->nframes) {
> +			/* update CAN frames content */
> +			memcpy(op->frames, new_frames,
> +			       msg_head->nframes * op->cfsiz);

[ ... ]

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