| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20240219-activist-smartly-87263f328a0c-mkl@pengutronix.de>
Date: Mon, 19 Feb 2024 10:41:34 +0100
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Simon Horman <horms@...nel.org>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
linux-can@...r.kernel.org, kernel@...gutronix.de, Oliver Hartkopp <socketcan@...tkopp.net>,
Dan Carpenter <dan.carpenter@...aro.org>
Subject: Re: [PATCH net-next 22/23] can: canxl: add virtual CAN network
identifier support
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@...tkopp.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@...tkopp.net>
> > Link: https://lore.kernel.org/all/20240212213550.18516-1-socketcan@hartkopp.net
> > Signed-off-by: Marc Kleine-Budde <mkl@...gutronix.de>
>
> 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 |
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists