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

From: "Ziyang Xuan (William)" <william.xuanziyang@huawei.com>
To: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: <mkl@pengutronix.de>, <davem@davemloft.net>, <kuba@kernel.org>,
	<linux-can@vger.kernel.org>, <netdev@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net v2] can: raw: fix raw_rcv panic for sock UAF
Date: Thu, 22 Jul 2021 20:53:24 +0800	[thread overview]
Message-ID: <dfb8e04b-257d-6de5-280b-e0326b4d0dbe@huawei.com> (raw)
In-Reply-To: <d684ef4d-d6c1-56b0-dc9e-b330fb92ba87@hartkopp.net>

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