Re: [PATCH net 1/1] can: bcm: limit RX operation resource usage per socket

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] can: bcm: bcm_rx_setup() returns 0 on frame array allocation failure
--

commit 3aff0eb4ba8d41c989dbd64aa48ab75c3eecec89
Author: Zihan Xi <zihanx@nebusec.ai>

can: bcm: limit RX operation resource usage per socket

This commit bounds the per-socket number and total memory allocation size of
CAN BCM RX operations, as well as the number of pending RCU receiver
callbacks. When a socket reaches either limit, creating a new RX operation
fails with -ENOBUFS.

Link: https://bugtracker.nebusec.ai/f/11134

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 3d637a1e0ac1a..96d3f050dec54 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c

[ ... ]

> @@ -1425,26 +1518,22 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  		op->nframes = msg_head->nframes;
>  		op->cfsiz = CFSIZ(msg_head->flags);
>  		op->flags = msg_head->flags;
> +		op->rx_mem = rx_mem;
>  
>  		if (msg_head->nframes > 1) {
>  			/* create array for CAN frames and copy the data */
>  			op->frames = kmalloc_array(msg_head->nframes,
>  						   op->cfsiz,
>  						   GFP_KERNEL);
> -			if (!op->frames) {
> -				kfree(op);
> -				return -ENOMEM;
> -			}
> +			if (!op->frames)
> +				goto free_op;
>  
>  			/* create and init array for received CAN frames */
>  			op->last_frames = kcalloc(msg_head->nframes,
>  						  op->cfsiz,
>  						  GFP_KERNEL);
> -			if (!op->last_frames) {
> -				kfree(op->frames);
> -				kfree(op);
> -				return -ENOMEM;
> -			}
> +			if (!op->last_frames)
> +				goto free_op;

[Severity: Medium]
In net/can/bcm.c:bcm_rx_setup(), when creating a new RX operation with
msg_head->nframes > 1, err is initialized to 0 at the start of the function:

	int new_op = 0;
	int err = 0;

If kmalloc_array() for op->frames or kcalloc() for op->last_frames fails,
jumping to free_op without setting err = -ENOMEM executes:

free_op:
	if (op->frames != &op->sframe)
		kfree(op->frames);
	if (op->last_frames != &op->last_sframe)
		kfree(op->last_frames);
	kfree(op);
	bcm_rx_op_uncharge(bo, rx_mem);
	return err;

Doesn't this cause bcm_rx_setup() (and bcm_sendmsg()) to return 0 to
userspace instead of -ENOMEM when either frame array allocation fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793089479.git.zihanx@nebusec.ai?part=1