Received-SPF: pass (google.com: domain of linux-kernel+bounces-72575-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <785e60d5-a73e-4f5f-ada0-a9d1891e9c75@hartkopp.net>
Date: Tue, 20 Feb 2024 09:37:01 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] can: raw: raw_getsockopt(): reduce scope of err
To: Eric Dumazet <edumazet@google.com>, Marc Kleine-Budde <mkl@pengutronix.de>
Cc: "David S. Miller" <davem@davemloft.net>, Jakub Kicinski
 <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
 linux-can@vger.kernel.org, netdev@vger.kernel.org,
 linux-kernel@vger.kernel.org
References: <20240220-raw-setsockopt-v1-1-7d34cb1377fc@pengutronix.de>
 <CANn89iL7N-1zvBBLoz0qhCApVJRF1LKu=jSyC0yz-wHa3JLGNg@mail.gmail.com>
Content-Language: en-US
From: Oliver Hartkopp <socketcan@hartkopp.net>
In-Reply-To: <CANn89iL7N-1zvBBLoz0qhCApVJRF1LKu=jSyC0yz-wHa3JLGNg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


On 2024-02-20 09:25, Eric Dumazet wrote:
> On Tue, Feb 20, 2024 at 9:16 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>>
>> Reduce the scope of the variable "err" to the individual cases. This
>> is to avoid the mistake of setting "err" in the mistaken belief that
>> it will be evaluated later.
>>
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>> ---
>>   net/can/raw.c | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/can/raw.c b/net/can/raw.c
>> index 897ffc17d850..2bb3eab98af0 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 */
>> @@ -839,6 +842,7 @@ static int raw_getsockopt(struct socket *sock, int level, int optname,
>>                          err = put_user(len, optlen);
>>                  return err;
>>
>> +       }
>>          case CAN_RAW_JOIN_FILTERS:
>>                  if (len > sizeof(int))
>>                          len = sizeof(int);
>>
>> ---
>> base-commit: c8fba5d6df5e476aa791db4f1f014dad2bb5e904
>> change-id: 20240220-raw-setsockopt-f6e173cdbbbb
> 
> What is the target tree ?
> 
> In net-next tree, syzbot complained about a bug added in
> 
> commit c83c22ec1493c0b7cc77327bedbd387e295872b6
> Author: Oliver Hartkopp <socketcan@hartkopp.net>
> Date:   Mon Feb 12 22:35:50 2024 +0100
> 
>      can: canxl: add virtual CAN network identifier support
> 
> Patch to fix the issue has not been sent yet ?

The patch with the fix has been sent yesterday and Marc applied it to 
his linux-can-next tree for a pull request to net-next.

https://lore.kernel.org/linux-can/20240220-mobility-thigh-8ddfb02bfab9-mkl@pengutronix.de/T/#t

Best regards,
Oliver

> 
> diff --git a/net/can/raw.c b/net/can/raw.c
> index cb8e6f788af84ac65830399baac6d1cf3d093e08..897ffc17d850670003e5cf3402477e8fc201f61e
> 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -835,7 +835,9 @@ static int raw_getsockopt(struct socket *sock, int
> level, int optname,
>                          if (copy_to_user(optval, &ro->raw_vcid_opts, len))
>                                  err = -EFAULT;
>                  }
> -               break;
> +               if (!err)
> +                       err = put_user(len, optlen);
> +               return err;
> 
>          case CAN_RAW_JOIN_FILTERS:
>                  if (len > sizeof(int))
>