netdev - Re: [PATCH net-next] virtio-net: fix use-after-free in skb_gro

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20210503035959-mutt-send-email-mst@kernel.org>
Date:   Mon, 3 May 2021 04:00:13 -0400
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Jason Wang <jasowang@...hat.com>
Cc:     Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        virtualization@...ts.linux-foundation.org,
        Ido Schimmel <idosch@...dia.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] virtio-net: fix use-after-free in
 skb_gro_receive

On Fri, Apr 23, 2021 at 12:33:09PM +0800, Jason Wang wrote:
> 
> 在 2021/4/23 下午12:19, Xuan Zhuo 写道:
> > On Fri, 23 Apr 2021 12:08:34 +0800, Jason Wang <jasowang@...hat.com> wrote:
> > > 在 2021/4/22 下午11:16, Xuan Zhuo 写道:
> > > > When "headroom" > 0, the actual allocated memory space is the entire
> > > > page, so the address of the page should be used when passing it to
> > > > build_skb().
> > > > 
> > > > BUG: KASAN: use-after-free in skb_gro_receive (net/core/skbuff.c:4260)
> > > > Write of size 16 at addr ffff88811619fffc by task kworker/u9:0/534
> > > > CPU: 2 PID: 534 Comm: kworker/u9:0 Not tainted 5.12.0-rc7-custom-16372-gb150be05b806 #3382
> > > > Hardware name: QEMU MSN2700, BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
> > > > Workqueue: xprtiod xs_stream_data_receive_workfn [sunrpc]
> > > > Call Trace:
> > > >    <IRQ>
> > > > dump_stack (lib/dump_stack.c:122)
> > > > print_address_description.constprop.0 (mm/kasan/report.c:233)
> > > > kasan_report.cold (mm/kasan/report.c:400 mm/kasan/report.c:416)
> > > > skb_gro_receive (net/core/skbuff.c:4260)
> > > > tcp_gro_receive (net/ipv4/tcp_offload.c:266 (discriminator 1))
> > > > tcp4_gro_receive (net/ipv4/tcp_offload.c:316)
> > > > inet_gro_receive (net/ipv4/af_inet.c:1545 (discriminator 2))
> > > > dev_gro_receive (net/core/dev.c:6075)
> > > > napi_gro_receive (net/core/dev.c:6168 net/core/dev.c:6198)
> > > > receive_buf (drivers/net/virtio_net.c:1151) virtio_net
> > > > virtnet_poll (drivers/net/virtio_net.c:1415 drivers/net/virtio_net.c:1519) virtio_net
> > > > __napi_poll (net/core/dev.c:6964)
> > > > net_rx_action (net/core/dev.c:7033 net/core/dev.c:7118)
> > > > __do_softirq (./arch/x86/include/asm/jump_label.h:25 ./include/linux/jump_label.h:200 ./include/trace/events/irq.h:142 kernel/softirq.c:346)
> > > > irq_exit_rcu (kernel/softirq.c:221 kernel/softirq.c:422 kernel/softirq.c:434)
> > > > common_interrupt (arch/x86/kernel/irq.c:240 (discriminator 14))
> > > > </IRQ>
> > > > 
> > > > Fixes: fb32856b16ad ("virtio-net: page_to_skb() use build_skb when there's sufficient tailroom")
> > > > Signed-off-by: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
> > > > Reported-by: Ido Schimmel <idosch@...dia.com>
> > > > Tested-by: Ido Schimmel <idosch@...dia.com>
> > > > ---
> > > 
> > > Acked-by: Jason Wang <jasowang@...hat.com>
> > > 
> > > The codes became hard to read, I think we can try to do some cleanups on
> > > top to make it easier to read.
> > > 
> > > Thanks
> > Yes, this piece of code needs to be sorted out. Especially the big and mergeable
> > scenarios should be handled separately. Remove the mergeable code from this
> > function, and mergeable uses a new function alone.
> 
> 
> Right, another thing is that we may consider to relax the checking of len <
> GOOD_COPY_LEN.


Want to post a patch on top?

> Our QE still see low PPS compared with the code before 3226b158e67c ("net:
> avoid 32 x truesize under-estimation for tiny skbs").
> 
> Thanks
> 
> 
> > 
> > Thanks.
> > 
> > > 
> > > >    drivers/net/virtio_net.c | 12 +++++++++---
> > > >    1 file changed, 9 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index 74d2d49264f3..7fda2ae4c40f 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -387,7 +387,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> > > >    	unsigned int copy, hdr_len, hdr_padded_len;
> > > >    	struct page *page_to_free = NULL;
> > > >    	int tailroom, shinfo_size;
> > > > -	char *p, *hdr_p;
> > > > +	char *p, *hdr_p, *buf;
> > > > 
> > > >    	p = page_address(page) + offset;
> > > >    	hdr_p = p;
> > > > @@ -403,11 +403,15 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> > > >    	 * space are aligned.
> > > >    	 */
> > > >    	if (headroom) {
> > > > -		/* The actual allocated space size is PAGE_SIZE. */
> > > > +		/* Buffers with headroom use PAGE_SIZE as alloc size,
> > > > +		 * see add_recvbuf_mergeable() + get_mergeable_buf_len()
> > > > +		 */
> > > >    		truesize = PAGE_SIZE;
> > > >    		tailroom = truesize - len - offset;
> > > > +		buf = page_address(page);
> > > >    	} else {
> > > >    		tailroom = truesize - len;
> > > > +		buf = p;
> > > >    	}
> > > > 
> > > >    	len -= hdr_len;
> > > > @@ -416,11 +420,13 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> > > > 
> > > >    	shinfo_size = SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> > > > 
> > > > +	/* copy small packet so we can reuse these pages */
> > > >    	if (!NET_IP_ALIGN && len > GOOD_COPY_LEN && tailroom >= shinfo_size) {
> > > > -		skb = build_skb(p, truesize);
> > > > +		skb = build_skb(buf, truesize);
> > > >    		if (unlikely(!skb))
> > > >    			return NULL;
> > > > 
> > > > +		skb_reserve(skb, p - buf);
> > > >    		skb_put(skb, len);
> > > >    		goto ok;
> > > >    	}