On 2024-02-19 10:41, Marc Kleine-Budde wrote:
On 19.02.2024 08:39:10, Simon Horman wrote:
+Dan Carpenter
(..)
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.
Hello Marc,
the problem was an incomplete code adoption from another getsockopt()
function CAN_RAW_FILTER. No need to reduce the scope of err here as we
only have one sockopt function at a time.
@Simon: Many thanks for catching this issue!!
Patch:
https://lore.kernel.org/linux-can/20240219200021.12113-1-socketcan@xxxxxxxxxxxx/
Best regards,
Oliver
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
