Jason Wang
2021-May-06 03:23 UTC
[PATCH net-next] virtio-net: fix use-after-free in skb_gro_receive
? 2021/5/3 ??4:29, Xuan Zhuo ??:> On Mon, 3 May 2021 04:00:13 -0400, Michael S. Tsirkin<mst at redhat.com> wrote: >> 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? > Yes, I have completed the refactoring of this part of the code, and the related > testing work is in progress. I will submit the patch after the complete test is > completed. > > Thanks.Cool, one thing in my mind is we'd better pack the correct truesize (e.g PAGE_SIZE in the case of XDP) in the ctx and avoid tricky codes like: ??????????????? /* Buffers with headroom use PAGE_SIZE as alloc size, ???????????????? * see add_recvbuf_mergeable() + get_mergeable_buf_len() ???????????????? */ Thanks>