Jason Wang
2021-Apr-23 04:33 UTC
[PATCH net-next] virtio-net: fix use-after-free in skb_gro_receive
? 2021/4/23 ??12:19, Xuan Zhuo ??:> On Fri, 23 Apr 2021 12:08:34 +0800, Jason Wang <jasowang at redhat.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 at linux.alibaba.com> >>> Reported-by: Ido Schimmel <idosch at nvidia.com> >>> Tested-by: Ido Schimmel <idosch at nvidia.com> >>> --- >> >> Acked-by: Jason Wang <jasowang at redhat.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. 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; >>> }
Michael S. Tsirkin
2021-May-03 08:00 UTC
[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 at redhat.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 at linux.alibaba.com> > > > > Reported-by: Ido Schimmel <idosch at nvidia.com> > > > > Tested-by: Ido Schimmel <idosch at nvidia.com> > > > > --- > > > > > > Acked-by: Jason Wang <jasowang at redhat.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; > > > > }