netdev - Re: [PATCH net] can: raw: fix raw

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] [day] [month] [year] [list]
Date:   Thu, 22 Jul 2021 15:06:40 +0800
From:   "Ziyang Xuan (William)" <william.xuanziyang@...wei.com>
To:     Oliver Hartkopp <socketcan@...tkopp.net>,
        Greg KH <gregkh@...uxfoundation.org>
CC:     <davem@...emloft.net>, <kuba@...nel.org>, <mkl@...gutronix.de>,
        <netdev@...r.kernel.org>, <linux-can@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net] can: raw: fix raw_rcv panic for sock UAF



On 7/21/2021 11:13 PM, Oliver Hartkopp wrote:
> 
> 
> On 21.07.21 13:37, Ziyang Xuan (William) wrote:
>> On 7/21/2021 5:24 PM, Oliver Hartkopp wrote:
> 
>>>
>>> Can you please resend the below patch as suggested by Greg KH and add my
>>>
>>> Signed-off-by: Oliver Hartkopp <socketcan@...tkopp.net>
>>>
>>> as it also adds the dev_get_by_index() return check.
>>>
>>> diff --git a/net/can/raw.c b/net/can/raw.c
>>> index ed4fcb7ab0c3..d3cbc32036c7 100644
>>> --- a/net/can/raw.c
>>> +++ b/net/can/raw.c
>>> @@ -544,14 +544,18 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
>>>           } else if (count == 1) {
>>>               if (copy_from_sockptr(&sfilter, optval, sizeof(sfilter)))
>>>                   return -EFAULT;
>>>           }
>>>
>>> +        rtnl_lock();
>>>           lock_sock(sk);
>>>
>>> -        if (ro->bound && ro->ifindex)
>>> +        if (ro->bound && ro->ifindex) {
>>>               dev = dev_get_by_index(sock_net(sk), ro->ifindex);
>>> +            if (!dev)
>>> +                goto out_fil;
>>> +        }
>> At first, I also use this modification. After discussion with my partner, we found that
>> it is impossible scenario if we use rtnl_lock to protect net_device object.
>> We can see two sequences:
>> 1. raw_setsockopt first get rtnl_lock, unregister_netdevice_many later.
>> It can be simplified to add the filter in raw_setsockopt, then remove the filter in raw_notify.
>>
>> 2. unregister_netdevice_many first get rtnl_lock, raw_setsockopt later.
>> raw_notify will set ro->ifindex, ro->bound and ro->count to zero firstly. The filter will not
>> be added to any filter_list in raw_notify.
>>
>> So I selected the current modification. Do you think so?
>>
>> My first modification as following:
>>
>> diff --git a/net/can/raw.c b/net/can/raw.c
>> index ed4fcb7ab0c3..a0ce4908317f 100644
>> --- a/net/can/raw.c
>> +++ b/net/can/raw.c
>> @@ -546,10 +546,16 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
>>                                  return -EFAULT;
>>                  }
>>
>> +               rtnl_lock();
>>                  lock_sock(sk);
>>
>> -               if (ro->bound && ro->ifindex)
>> +               if (ro->bound && ro->ifindex) {
>>                          dev = dev_get_by_index(sock_net(sk), ro->ifindex);
>> +                       if (!dev) {
>> +                               err = -ENODEV;
>> +                               goto out_fil;
>> +                       }
>> +               }
>>
>>                  if (ro->bound) {
>>                          /* (try to) register the new filters */
>> @@ -559,11 +565,8 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
>>                          else
>>                                  err = raw_enable_filters(sock_net(sk), dev, sk,
>>                                                           filter, count);
>> -                       if (err) {
>> -                               if (count > 1)
>> -                                       kfree(filter);
>> +                       if (err)
>>                                  goto out_fil;
>> -                       }
>>
>>                          /* remove old filter registrations */
>>                          raw_disable_filters(sock_net(sk), dev, sk, ro->filter,
>> @@ -584,10 +587,14 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
>>                  ro->count  = count;
>>
>>    out_fil:
>> +               if (err && count > 1)
>> +                       kfree(filter);
>> +
> 
> Setting the err variable to -ENODEV is a good idea but I do not like the movement of kfree(filter).
> 
> The kfree() has a tight relation inside the if-statement for ro->bound which makes it easier to understand.
> 
> Regards,
> Oliver

I will submit the v2 patch for the problem according to your suggestions. Than you.