Messages in this thread | | | Subject | Re: [PATCH net] can: raw: fix raw_rcv panic for sock UAF | From | Oliver Hartkopp <> | Date | Wed, 21 Jul 2021 17:13:08 +0200 |
| |
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
ps. your patches have less context than mine. Do you have different settings for -U<n>, --unified=<n> for 'git diff' ?
> if (dev) > dev_put(dev); > > release_sock(sk); > + rtnl_unlock(); > > break; > > @@ -600,10 +607,16 @@ static int raw_setsockopt(struct socket *sock, int level, int optname, > > err_mask &= CAN_ERR_MASK; > > + 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_err; > + } > + } > > /* remove current error mask */ > if (ro->bound) { > @@ -627,6 +640,7 @@ static int raw_setsockopt(struct socket *sock, int level, int optname, > dev_put(dev); > > release_sock(sk); > + rtnl_unlock(); > > break; >
| |