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

From: Ziyang Xuan (William)
Date: Thu Jul 22 2021 - 08:53:34 EST


>> diff --git a/net/can/raw.c b/net/can/raw.c
>> index ed4fcb7ab0c3..cd5a49380116 100644
>> --- a/net/can/raw.c
>> +++ b/net/can/raw.c
>> @@ -546,10 +546,18 @@ 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) {
>
>
>> +                if (count > 1)
>> +                    kfree(filter);
>
> This was NOT suggested!
>
> I've been talking about removing the other kfree() "improvement" you suggested.
>
> The kfree() should only be done when ro->bound and ro->ifindex are cleared.
>
> So when you remove these two lines it should be ok.
>
> Please try to increase the context in the diff.
>
> Thanks,
> Oliver

Sorry, I am a little confused.

The following codes are the latest raw_setsockopt function realization(ignore some non-key parts)
with my patch. Now we assume the condition that count more than 1, ro->bound and ro->ifindex
are not zero, dev_get_by_index() will return NULL. We analyze the code logic.

static int raw_setsockopt(struct socket *sock, int level, int optname,
sockptr_t optval, unsigned int optlen)
{
......
struct can_filter *filter = NULL;
......

switch (optname) {
case CAN_RAW_FILTER:
......

if (count > 1) {
/* filter does not fit into dfilter => alloc space */
filter = memdup_sockptr(optval, optlen); // filter point to a heap memory
if (IS_ERR(filter))
return PTR_ERR(filter);
} else if (count == 1) {
......
}

rtnl_lock();
lock_sock(sk);

if (ro->bound && ro->ifindex) {
dev = dev_get_by_index(sock_net(sk), ro->ifindex);

/*
* dev == NULL is exception. The function will exit abnormally.
* Memory pointed by filer does not forward to anyone for maintenance.
* If we do not kfree(filter) here, memory will be leaked after function exit.
*/
if (!dev) {
if (count > 1)
kfree(filter);
err = -ENODEV;
goto out_fil;
}
}

if (ro->bound) {
/* (try to) register the new filters */
if (count == 1)
err = raw_enable_filters(sock_net(sk), dev, sk,
&sfilter, 1);
else
err = raw_enable_filters(sock_net(sk), dev, sk,
filter, count);
if (err) {
if (count > 1)
kfree(filter);
goto out_fil;
}

/* remove old filter registrations */
raw_disable_filters(sock_net(sk), dev, sk, ro->filter,
ro->count);
}

/* remove old filter space */
if (ro->count > 1)
kfree(ro->filter);

/* link new filters to the socket */
if (count == 1) {
/* copy filter data for single filter */
ro->dfilter = sfilter;
filter = &ro->dfilter;
}
ro->filter = filter;
ro->count = count;

out_fil:
if (dev)
dev_put(dev);

release_sock(sk);
rtnl_unlock();

break;
......

return err;
}

So I think my modification is right. Thank you.