Thread (6 messages) 6 messages, 3 authors, 2021-07-24

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

From: Ziyang Xuan (William) <hidden>
Date: 2021-07-22 12:53:33
Also in: linux-can, lkml

quoted
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) {
quoted
+                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.



Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help