Re: [PATCH net-next 22/23] can: canxl: add virtual CAN network identifier support

[Simon Horman <horms@kernel.org>]

+Dan Carpenter

On Tue, Feb 13, 2024 at 12:25:25PM +0100, Marc Kleine-Budde wrote:
> From: Oliver Hartkopp <socketcan@hartkopp.net>
> 
> CAN XL data frames contain an 8-bit virtual CAN network identifier (VCID).
> A VCID value of zero represents an 'untagged' CAN XL frame.
> 
> To receive and send these optional VCIDs via CAN_RAW sockets a new socket
> option CAN_RAW_XL_VCID_OPTS is introduced to define/access VCID content:
> 
> - tx: set the outgoing VCID value by the kernel (one fixed 8-bit value)
> - tx: pass through VCID values from the user space (e.g. for traffic replay)
> - rx: apply VCID receive filter (value/mask) to be passed to the user space
> 
> With the 'tx pass through' option CAN_RAW_XL_VCID_TX_PASS all valid VCID
> values can be sent, e.g. to replay full qualified CAN XL traffic.
> 
> The VCID value provided for the CAN_RAW_XL_VCID_TX_SET option will
> override the VCID value in the struct canxl_frame.prio defined for
> CAN_RAW_XL_VCID_TX_PASS when both flags are set.
> 
> With a rx_vcid_mask of zero all possible VCID values (0x00 - 0xFF) are
> passed to the user space when the CAN_RAW_XL_VCID_RX_FILTER flag is set.
> Without this flag only untagged CAN XL frames (VCID = 0x00) are delivered
> to the user space (default).
> 
> The 8-bit VCID is stored inside the CAN XL prio element (only in CAN XL
> frames!) to not interfere with other CAN content or the CAN filters
> provided by the CAN_RAW sockets and kernel infrastruture.
> 
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> Link: https://lore.kernel.org/all/20240212213550.18516-1-socketcan@hartkopp.net
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

Hi Oliver and Marc,

I understand this pull-request has been accepted.
But I noticed the problem described below which
seems worth bringing to your attention.

...

> @@ -786,6 +822,21 @@ static int raw_getsockopt(struct socket *sock, int level, int optname,
>  		val = &ro->xl_frames;
>  		break;
>  
> +	case CAN_RAW_XL_VCID_OPTS:
> +		/* user space buffer to small for VCID opts? */
> +		if (len < sizeof(ro->raw_vcid_opts)) {
> +			/* return -ERANGE and needed space in optlen */
> +			err = -ERANGE;
> +			if (put_user(sizeof(ro->raw_vcid_opts), optlen))
> +				err = -EFAULT;
> +		} else {
> +			if (len > sizeof(ro->raw_vcid_opts))
> +				len = sizeof(ro->raw_vcid_opts);
> +			if (copy_to_user(optval, &ro->raw_vcid_opts, len))
> +				err = -EFAULT;
> +		}
> +		break;
> +
>  	case CAN_RAW_JOIN_FILTERS:
>  		if (len > sizeof(int))
>  			len = sizeof(int);

At the end of the switch statement the following code is present:


	if (put_user(len, optlen))
		return -EFAULT;
	if (copy_to_user(optval, val, len))
		return -EFAULT;
	return 0;

And the call to copy_to_user() depends on val being set.

It appears that for all other cases handled by the switch statement,
either val is set or the function returns. But neither is the
case for CAN_RAW_XL_VCID_OPTS which seems to mean that val may be used
uninitialised.

Flagged by Smatch.

...