Re: [PATCH net] can: raw: fix raw_rcv panic for sock UAF

["Ziyang Xuan (William)" <william.xuanziyang@huawei.com>]

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