Re: [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[off-list ref]  wrote:

quoted

On Fri, Apr 23, 2021 at 12:33:09PM +0800, Jason Wang wrote:

quoted

在 2021/4/23 下午12:19, Xuan Zhuo 写道:

quoted

On Fri, 23 Apr 2021 12:08:34 +0800, Jason Wang[off-list ref]  wrote:

quoted

在 2021/4/22 下午11:16, Xuan Zhuo 写道:

quoted

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@linux.alibaba.com>
Reported-by: Ido Schimmel<idosch@nvidia.com>
Tested-by: Ido Schimmel<idosch@nvidia.com>
---

Acked-by: Jason Wang<redacted>

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