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

[Jason Wang]

On 2019/4/26 上午12:04, YueHaibing wrote:
On 2019/4/24 20:25, YueHaibing wrote:
On 2019/4/24 17:11, Jason Wang wrote:
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
  I configured 8 queues with virtio driver to start vm

2) using pktgen to inject packets to all queues through tap
  inject packet into tap nic in host

3) using ethtool to change the combined channels in guest in a loop
  repeat do as follow in vm:
        ethtool -L eth0 combined 4
        ethtool -L eth0 combined 8

4) kill the guest

This cannot reproduce the issue.


Thanks for the testing. If possible, please keep doing the test. I will 
try to come some patch in one or two days.




Ok, I will try this.

Thanks


Thanks.
.


.