Re: BUG: KASAN: use-after-free Read in tun_net_xmit

[Jason Wang]

On 2019/4/24 上午12:41, Cong Wang wrote:
On Mon, Apr 22, 2019 at 11:42 PM Jason Wang <jasow...@redhat.com> wrote:

On 2019/4/23 下午2:00, Cong Wang wrote:
On Mon, Apr 22, 2019 at 2:41 AM Jason Wang <jasow...@redhat.com> wrote:
On 2019/4/22 上午11:57, YueHaibing wrote:
We get a KASAN report as below, but don't have any reproducer.

Any comments are appreciated.

==================================================================
BUG: KASAN: use-after-free in tun_net_xmit+0x1670/0x1750 drivers/net/tun.c:1104
Read of size 8 at addr ffff88836cc26a70 by task swapper/3/0
Which kernel version did you use? The calltrace points out the a use
after free for tun_file structure which should be synchronized through
RCU + RTNL lock.
The tfile socket has to be marked with SOCK_RCU_FREE in order
to fully respect the RCU grace period.

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index e9ca1c088d0b..31c3210288cb 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -3431,6 +3431,7 @@ static int tun_chr_open(struct inode *inode,
struct file * file)
          file->private_data = tfile;
          INIT_LIST_HEAD(&tfile->next);

+       sock_set_flag(&tfile->sk, SOCK_RCU_FREE);
          sock_set_flag(&tfile->sk, SOCK_ZEROCOPY);

          return 0;

We did a synchronize_net() when socket is detached from netdevice in
__tun_detach() so it looks to me this is unnecessary.
I knew, but it is only called conditionally, that is:

  695         if (tun && !tfile->detached) {
...
  710
  711                 synchronize_net();

And it looks like syzbot just skipped this condition,


If tfile is detached, it should have gone for the path of 
synchronize_net() before. If tfile is never attached, tun_net_xmit() 
doesn't have the chance to access that. I wonder whether or not we 
should use WRITE_ONCE() for tun->numqueues-- in this fucntion. If the 
value was not committed to memory before synchronize_net(), we may race 
with tun_net_xmit() which check txq against tun->numqueues.


this is why I believe
you still need to respect RCU grace period _unconditionally_ for tfile.


This is true if I miss subtle race in the code.


Haibing: could you please try the following test?

1) start VM with multiple queue

2) using pktgen to inject packets to all queues through tap

3) using ethtool to change the combined channels in guest in a loop

4) kill the guest


Thanks


Thanks.