Messages in this thread | | | Subject | Re: [PATCH net v2] can: raw: fix raw_rcv panic for sock UAF | From | "Ziyang Xuan (William)" <> | Date | Thu, 22 Jul 2021 20:53:24 +0800 |
| |
>> 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.
| |