lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <6AADFAC011213A4C87B956458587ADB4021F9A34@dggeml532-mbs.china.huawei.com> Date: Mon, 29 Apr 2019 03:53:32 +0000 From: "weiyongjun (A)" <weiyongjun1@...wei.com> To: Jason Wang <jasowang@...hat.com>, Cong Wang <xiyou.wangcong@...il.com> CC: yuehaibing <yuehaibing@...wei.com>, "davem@...emloft.net" <davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>, "brouer@...hat.com" <brouer@...hat.com>, "mst@...hat.com" <mst@...hat.com>, "lirongqing@...du.com" <lirongqing@...du.com>, nicolas dichtel <nicolas.dichtel@...nd.com>, "3chas3@...il.com" <3chas3@...il.com>, "wangli39@...du.com" <wangli39@...du.com>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, Peter Xu <peterx@...hat.com> Subject: RE: [PATCH] tun: Fix use-after-free in tun_net_xmit > > On Sun, Apr 28, 2019 at 12:51 AM Jason Wang <jasowang@...hat.com> > wrote: > >>> tun_net_xmit() doesn't have the chance to > >>> access the change because it holding the rcu_read_lock(). > >> > >> > >> The problem is the following codes: > >> > >> > >> --tun->numqueues; > >> > >> ... > >> > >> synchronize_net(); > >> > >> We need make sure the decrement of tun->numqueues be visible to > readers > >> after synchronize_net(). And in tun_net_xmit(): > > > > It doesn't matter at all. Readers are okay to read it even they still use the > > stale tun->numqueues, as long as the tfile is not freed readers can read > > whatever they want... > > This is only true if we set SOCK_RCU_FREE, isn't it? > > > > > The decrement of tun->numqueues is just how we unpublish the old > > tfile, it is still valid for readers to read it _after_ unpublish, we only need > > to worry about free, not about unpublish. This is the whole spirit of RCU. > > > > The point is we don't convert tun->numqueues to RCU but use > synchronize_net(). > > > You need to rethink about my SOCK_RCU_FREE patch. > > The code is wrote before SOCK_RCU_FREE is introduced and assume no > de-reference from device after synchronize_net(). It doesn't harm to > figure out the root cause which may give us more confidence to the fix > (e.g like SOCK_RCU_FREE). > > I don't object to fix with SOCK_RCU_FREE, but then we should remove > the redundant synchronize_net(). But I still prefer to synchronize > everything explicitly like (completely untested): > > From df91f77d35a6aa7943b6f2a7d4b329990896a0fe Mon Sep 17 00:00:00 > 2001 > From: Jason Wang <jasowang@...hat.com> > Date: Mon, 29 Apr 2019 10:21:06 +0800 > Subject: [PATCH] tuntap: synchronize through tfiles instead of numqueues > > Signed-off-by: Jason Wang <jasowang@...hat.com> > --- > drivers/net/tun.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 80bff1b4ec17..03715f605fb5 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -698,6 +698,7 @@ static void __tun_detach(struct tun_file *tfile, bool > clean) > > rcu_assign_pointer(tun->tfiles[index], > tun->tfiles[tun->numqueues - 1]); > + rcu_assign_pointer(tun->tfiles[tun->numqueues], NULL); Should be "rcu_assign_pointer(tun->tfiles[tun->numqueues - 1], NULL);" > ntfile = rtnl_dereference(tun->tfiles[index]); > ntfile->queue_index = index; > > @@ -1082,7 +1083,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff > *skb, struct net_device *dev) > tfile = rcu_dereference(tun->tfiles[txq]); > > /* Drop packet if interface is not attached */ > - if (txq >= tun->numqueues) > + if (!tfile) > goto drop; > > if (!rcu_dereference(tun->steering_prog)) > @@ -1305,15 +1306,13 @@ static int tun_xdp_xmit(struct net_device *dev, > int n, > > rcu_read_lock(); > > - numqueues = READ_ONCE(tun->numqueues); > - if (!numqueues) { > + tfile = rcu_dereference(tun->tfiles[smp_processor_id() % > + tun->numqueues]); > + if (!tfile) { > rcu_read_unlock(); > return -ENXIO; /* Caller will free/return all frames */ > } > > - tfile = rcu_dereference(tun->tfiles[smp_processor_id() % > - numqueues]); > - > spin_lock(&tfile->tx_ring.producer_lock); > for (i = 0; i < n; i++) { > struct xdp_frame *xdp = frames[i]; > -- > 2.19.1
Powered by blists - more mailing lists