On 19.02.2024 08:39:10, Simon Horman wrote: > +Dan Carpenter > > On Tue, Feb 13, 2024 at 12:25:25PM +0100, Marc Kleine-Budde wrote: > > From: Oliver Hartkopp <socketcan@xxxxxxxxxxxx> > > > > 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@xxxxxxxxxxxx> > > Link: https://lore.kernel.org/all/20240212213550.18516-1-socketcan@xxxxxxxxxxxx > > Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> > > Hi Oliver and Marc, > > I understand this pull-request has been accepted. ACK > But I noticed the problem described below which > seems worth bringing to your attention. Thanks Simon. > > ... > > > @@ -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. And "err" is not evaluated, too. Oliver, please send a fix and squash in this chance to reduce the scope of "err" to the cases where it's actually used. diff --git a/net/can/raw.c b/net/can/raw.c index cb8e6f788af8..d4e27877c143 100644 --- a/net/can/raw.c +++ b/net/can/raw.c @@ -756,7 +756,6 @@ static int raw_getsockopt(struct socket *sock, int level, int optname, struct raw_sock *ro = raw_sk(sk); int len; void *val; - int err = 0; if (level != SOL_CAN_RAW) return -EINVAL; @@ -766,7 +765,9 @@ static int raw_getsockopt(struct socket *sock, int level, int optname, return -EINVAL; switch (optname) { - case CAN_RAW_FILTER: + case CAN_RAW_FILTER: { + int err = 0; + lock_sock(sk); if (ro->count > 0) { int fsize = ro->count * sizeof(struct can_filter); @@ -791,7 +792,7 @@ static int raw_getsockopt(struct socket *sock, int level, int optname, if (!err) err = put_user(len, optlen); return err; - + } case CAN_RAW_ERR_FILTER: if (len > sizeof(can_err_mask_t)) len = sizeof(can_err_mask_t); @@ -822,7 +823,9 @@ static int raw_getsockopt(struct socket *sock, int level, int optname, val = &ro->xl_frames; break; - case CAN_RAW_XL_VCID_OPTS: + case CAN_RAW_XL_VCID_OPTS: { + int err = 0; + /* user space buffer to small for VCID opts? */ if (len < sizeof(ro->raw_vcid_opts)) { /* return -ERANGE and needed space in optlen */ @@ -836,7 +839,7 @@ static int raw_getsockopt(struct socket *sock, int level, int optname, err = -EFAULT; } break; - + } case CAN_RAW_JOIN_FILTERS: if (len > sizeof(int)) len = sizeof(int); regards, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Attachment:
signature.asc
Description: PGP signature
