Re: [PATCH net] can: raw: fix raw_rcv panic for sock UAF
From: Oliver Hartkopp <socketcan@hartkopp.net>
Date: 2021-07-21 15:13:42
Also in:
linux-can, lkml, stable
On 21.07.21 13:37, Ziyang Xuan (William) wrote:
On 7/21/2021 5:24 PM, Oliver Hartkopp wrote:
quoted hunk ↗ jump to hunk
quoted
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' ?
quoted hunk ↗ jump to hunk
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;