Re: [PATCH v2 2/2] can: bcm: add locking when updating filter and timer values

[Oliver Hartkopp]

On 19.02.26 13:47, Oliver Hartkopp via B4 Relay wrote:
> From: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>
>
> KCSAN detected a simultaneous access to timer values that can be
> overwritten in bcm_rx_setup when updating timer and filter content.
> This caused no functional issues in the past as the new values might
> show up at any time without losing its intended functionality.
>
> Btw. the KCSAN report can be resolved by protecting the 'lockless'
> data updates with a spin_lock_bh().
>
> Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
> Reported-by: syzbot+75e5e4ae00c3b4bb544e@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://lore.kernel.org/linux-can/6975d5cf.a00a0220.33ccc7.0022.GAE@xxxxxxxxxx/
> Signed-off-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>

Tested-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>

The bcm KCSAN splats have gone with this patch.

Best regards,
Oliver

> ---
>   net/can/bcm.c | 39 ++++++++++++++++++++++++++++++++-------
>   1 file changed, 32 insertions(+), 7 deletions(-)
>
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index fd9fa072881e..0a3dc5500e14 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -123,10 +123,11 @@ struct bcm_op {
>   	struct canfd_frame sframe;
>   	struct canfd_frame last_sframe;
>   	struct sock *sk;
>   	struct net_device *rx_reg_dev;
>   	spinlock_t bcm_tx_lock; /* protect currframe/count in runtime updates */
> +	spinlock_t bcm_rx_update_lock; /* protect update of filter data */
>   };
>   
>   struct bcm_sock {
>   	struct sock sk;
>   	int bound;
> @@ -1141,10 +1142,12 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>   		return -EINVAL;
>   
>   	/* check the given can_id */
>   	op = bcm_find_op(&bo->rx_ops, msg_head, ifindex);
>   	if (op) {
> +		void *new_frames = NULL;
> +
>   		/* update existing BCM operation */
>   
>   		/*
>   		 * Do we need more space for the CAN frames than currently
>   		 * allocated? -> This is a _really_ unusual use-case and
> @@ -1152,33 +1155,53 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>   		 */
>   		if (msg_head->nframes > op->nframes)
>   			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);
> +			}
>   		}
>   
> +		spin_lock_bh(&op->bcm_rx_update_lock);
>   		op->nframes = msg_head->nframes;
>   		op->flags = msg_head->flags;
>   
> +		if (msg_head->nframes) {
> +			/* update CAN frames content */
> +			memcpy(op->frames, new_frames,
> +			       msg_head->nframes * op->cfsiz);
> +
> +			/* clear last_frames to indicate 'nothing received' */
> +			memset(op->last_frames, 0,
> +			       msg_head->nframes * op->cfsiz);
> +		}
> +		spin_unlock_bh(&op->bcm_rx_update_lock);
> +
> +		/* free temporary frames / kfree(NULL) is safe */
> +		kfree(new_frames);
> +
>   		/* Only an update -> do not call can_rx_register() */
>   		do_rx_register = 0;
>   
>   	} else {
>   		/* insert new BCM operation for the given can_id */
>   		op = kzalloc(OPSIZ, GFP_KERNEL);
>   		if (!op)
>   			return -ENOMEM;
>   
>   		spin_lock_init(&op->bcm_tx_lock);
> +		spin_lock_init(&op->bcm_rx_update_lock);
>   		op->can_id = msg_head->can_id;
>   		op->nframes = msg_head->nframes;
>   		op->cfsiz = CFSIZ(msg_head->flags);
>   		op->flags = msg_head->flags;
>   
> @@ -1261,24 +1284,26 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>   
>   	} else {
>   		if (op->flags & SETTIMER) {
>   
>   			/* set timer value */
> +			spin_lock_bh(&op->bcm_rx_update_lock);
>   			op->ival1 = msg_head->ival1;
>   			op->ival2 = msg_head->ival2;
>   			op->kt_ival1 = bcm_timeval_to_ktime(msg_head->ival1);
>   			op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2);
> +			op->kt_lastmsg = 0;
> +			spin_unlock_bh(&op->bcm_rx_update_lock);
>   
>   			/* disable an active timer due to zero value? */
>   			if (!op->kt_ival1)
>   				hrtimer_cancel(&op->timer);
>   
>   			/*
>   			 * In any case cancel the throttle timer, flush
>   			 * potentially blocked msgs and reset throttle handling
>   			 */
> -			op->kt_lastmsg = 0;
>   			hrtimer_cancel(&op->thrtimer);
>   			bcm_rx_thr_flush(op);
>   		}
>   
>   		if ((op->flags & STARTTIMER) && op->kt_ival1)