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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2828dbd5-9bea-4b2e-9a4f-ebd0582c8f73@hartkopp.net>
Date: Mon, 19 Feb 2024 21:15:19 +0100
From: Oliver Hartkopp <socketcan@...tkopp.net>
To: Marc Kleine-Budde <mkl@...gutronix.de>, Simon Horman <horms@...nel.org>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
 linux-can@...r.kernel.org, kernel@...gutronix.de,
 Dan Carpenter <dan.carpenter@...aro.org>
Subject: Re: [PATCH net-next 22/23] can: canxl: add virtual CAN network
 identifier support



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@hartkopp.net/

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
>