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>] [day] [month] [year] [list]

Message-ID: <770ce8e6-4948-2977-4d63-7d82075e7fc8@redhat.com>
Date:   Thu, 6 May 2021 11:23:32 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
        "Michael S. Tsirkin" <mst@...hat.com>
Cc:     "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


在 2021/5/3 下午4:29, Xuan Zhuo 写道:
> On Mon, 3 May 2021 04:00:13 -0400, Michael S. Tsirkin<mst@...hat.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@...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?
> 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


>