* Re: [PATCH net-next] virtio-net: fix use-after-free in skb_gro_receive
[not found] <1619151585.3098595-1-xuanzhuo@linux.alibaba.com>
@ 2021-04-23 4:33 ` Jason Wang
0 siblings, 0 replies; 12+ messages in thread
From: Jason Wang @ 2021-04-23 4:33 UTC (permalink / raw)
To: Xuan Zhuo
Cc: Michael S. Tsirkin, David S. Miller, Jakub Kicinski,
virtualization, Ido Schimmel, netdev
在 2021/4/23 下午12:19, Xuan Zhuo 写道:
> On Fri, 23 Apr 2021 12:08:34 +0800, Jason Wang <jasowang@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@linux.alibaba.com>
>>> Reported-by: Ido Schimmel <idosch@nvidia.com>
>>> Tested-by: Ido Schimmel <idosch@nvidia.com>
>>> ---
>>
>> Acked-by: Jason Wang <jasowang@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;
>>> }
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net-next] virtio-net: fix use-after-free in skb_gro_receive
@ 2021-04-23 4:33 ` Jason Wang
0 siblings, 0 replies; 12+ messages in thread
From: Jason Wang @ 2021-04-23 4:33 UTC (permalink / raw)
To: Xuan Zhuo
Cc: Michael S. Tsirkin, Ido Schimmel, virtualization, netdev,
Jakub Kicinski, David S. Miller
在 2021/4/23 下午12:19, Xuan Zhuo 写道:
> On Fri, 23 Apr 2021 12:08:34 +0800, Jason Wang <jasowang@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@linux.alibaba.com>
>>> Reported-by: Ido Schimmel <idosch@nvidia.com>
>>> Tested-by: Ido Schimmel <idosch@nvidia.com>
>>> ---
>>
>> Acked-by: Jason Wang <jasowang@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;
>>> }
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net-next] virtio-net: fix use-after-free in skb_gro_receive
2021-04-23 4:33 ` Jason Wang
@ 2021-05-03 8:00 ` Michael S. Tsirkin
-1 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2021-05-03 8:00 UTC (permalink / raw)
To: Jason Wang
Cc: Xuan Zhuo, David S. Miller, Jakub Kicinski, virtualization,
Ido Schimmel, netdev
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@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@linux.alibaba.com>
> > > > Reported-by: Ido Schimmel <idosch@nvidia.com>
> > > > Tested-by: Ido Schimmel <idosch@nvidia.com>
> > > > ---
> > >
> > > Acked-by: Jason Wang <jasowang@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;
> > > > }
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net-next] virtio-net: fix use-after-free in skb_gro_receive
@ 2021-05-03 8:00 ` Michael S. Tsirkin
0 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2021-05-03 8:00 UTC (permalink / raw)
To: Jason Wang
Cc: Xuan Zhuo, Ido Schimmel, virtualization, netdev, Jakub Kicinski,
David S. Miller
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@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@linux.alibaba.com>
> > > > Reported-by: Ido Schimmel <idosch@nvidia.com>
> > > > Tested-by: Ido Schimmel <idosch@nvidia.com>
> > > > ---
> > >
> > > Acked-by: Jason Wang <jasowang@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;
> > > > }
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <1620030574.9881887-1-xuanzhuo@linux.alibaba.com>]
* Re: [PATCH net-next] virtio-net: fix use-after-free in skb_gro_receive
[not found] <1620030574.9881887-1-xuanzhuo@linux.alibaba.com>
@ 2021-05-06 3:23 ` Jason Wang
0 siblings, 0 replies; 12+ messages in thread
From: Jason Wang @ 2021-05-06 3:23 UTC (permalink / raw)
To: Xuan Zhuo, Michael S. Tsirkin
Cc: David S. Miller, Jakub Kicinski, virtualization, Ido Schimmel, netdev
在 2021/5/3 下午4:29, Xuan Zhuo 写道:
> On Mon, 3 May 2021 04:00:13 -0400, Michael S. Tsirkin<mst@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@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@linux.alibaba.com>
>>>>>> Reported-by: Ido Schimmel<idosch@nvidia.com>
>>>>>> Tested-by: Ido Schimmel<idosch@nvidia.com>
>>>>>> ---
>>>>> Acked-by: Jason Wang<jasowang@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
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net-next] virtio-net: fix use-after-free in skb_gro_receive
@ 2021-05-06 3:23 ` Jason Wang
0 siblings, 0 replies; 12+ messages in thread
From: Jason Wang @ 2021-05-06 3:23 UTC (permalink / raw)
To: Xuan Zhuo, Michael S. Tsirkin
Cc: Jakub Kicinski, Ido Schimmel, netdev, David S. Miller, virtualization
在 2021/5/3 下午4:29, Xuan Zhuo 写道:
> On Mon, 3 May 2021 04:00:13 -0400, Michael S. Tsirkin<mst@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@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@linux.alibaba.com>
>>>>>> Reported-by: Ido Schimmel<idosch@nvidia.com>
>>>>>> Tested-by: Ido Schimmel<idosch@nvidia.com>
>>>>>> ---
>>>>> Acked-by: Jason Wang<jasowang@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
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net-next] virtio-net: fix use-after-free in skb_gro_receive
@ 2021-04-22 15:16 Xuan Zhuo
2021-04-23 4:08 ` Jason Wang
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Xuan Zhuo @ 2021-04-22 15:16 UTC (permalink / raw)
To: netdev
Cc: Michael S. Tsirkin, Jason Wang, David S. Miller, Jakub Kicinski,
Xuan Zhuo, virtualization, Ido Schimmel
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>
---
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;
}
--
2.31.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH net-next] virtio-net: fix use-after-free in skb_gro_receive
2021-04-22 15:16 Xuan Zhuo
@ 2021-04-23 4:08 ` Jason Wang
2021-04-23 20:20 ` patchwork-bot+netdevbpf
2021-05-03 7:59 ` Michael S. Tsirkin
2 siblings, 0 replies; 12+ messages in thread
From: Jason Wang @ 2021-04-23 4:08 UTC (permalink / raw)
To: Xuan Zhuo, netdev
Cc: Michael S. Tsirkin, David S. Miller, Jakub Kicinski,
virtualization, Ido Schimmel
在 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@linux.alibaba.com>
> Reported-by: Ido Schimmel <idosch@nvidia.com>
> Tested-by: Ido Schimmel <idosch@nvidia.com>
> ---
Acked-by: Jason Wang <jasowang@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
> 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;
> }
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net-next] virtio-net: fix use-after-free in skb_gro_receive
@ 2021-04-23 4:08 ` Jason Wang
0 siblings, 0 replies; 12+ messages in thread
From: Jason Wang @ 2021-04-23 4:08 UTC (permalink / raw)
To: Xuan Zhuo, netdev
Cc: Jakub Kicinski, Ido Schimmel, virtualization, David S. Miller,
Michael S. Tsirkin
在 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@linux.alibaba.com>
> Reported-by: Ido Schimmel <idosch@nvidia.com>
> Tested-by: Ido Schimmel <idosch@nvidia.com>
> ---
Acked-by: Jason Wang <jasowang@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
> 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;
> }
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next] virtio-net: fix use-after-free in skb_gro_receive
2021-04-22 15:16 Xuan Zhuo
2021-04-23 4:08 ` Jason Wang
@ 2021-04-23 20:20 ` patchwork-bot+netdevbpf
2021-05-03 7:59 ` Michael S. Tsirkin
2 siblings, 0 replies; 12+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-04-23 20:20 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: netdev, mst, jasowang, davem, kuba, virtualization, idosch
Hello:
This patch was applied to netdev/net-next.git (refs/heads/master):
On Thu, 22 Apr 2021 23:16:20 +0800 you wrote:
> 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>
>
> [...]
Here is the summary with links:
- [net-next] virtio-net: fix use-after-free in skb_gro_receive
https://git.kernel.org/netdev/net-next/c/f80bd740cb7c
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net-next] virtio-net: fix use-after-free in skb_gro_receive
2021-04-22 15:16 Xuan Zhuo
@ 2021-05-03 7:59 ` Michael S. Tsirkin
2021-04-23 20:20 ` patchwork-bot+netdevbpf
2021-05-03 7:59 ` Michael S. Tsirkin
2 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2021-05-03 7:59 UTC (permalink / raw)
To: Xuan Zhuo
Cc: netdev, Jason Wang, David S. Miller, Jakub Kicinski,
virtualization, Ido Schimmel
On Thu, Apr 22, 2021 at 11:16:20PM +0800, Xuan Zhuo wrote:
> 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: Michael S. Tsirkin <mst@redhat.com>
> ---
> 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;
> }
> --
> 2.31.0
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net-next] virtio-net: fix use-after-free in skb_gro_receive
@ 2021-05-03 7:59 ` Michael S. Tsirkin
0 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2021-05-03 7:59 UTC (permalink / raw)
To: Xuan Zhuo
Cc: netdev, virtualization, Ido Schimmel, Jakub Kicinski, David S. Miller
On Thu, Apr 22, 2021 at 11:16:20PM +0800, Xuan Zhuo wrote:
> 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: Michael S. Tsirkin <mst@redhat.com>
> ---
> 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;
> }
> --
> 2.31.0
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-05-06 3:23 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1619151585.3098595-1-xuanzhuo@linux.alibaba.com>
2021-04-23 4:33 ` [PATCH net-next] virtio-net: fix use-after-free in skb_gro_receive Jason Wang
2021-04-23 4:33 ` Jason Wang
2021-05-03 8:00 ` Michael S. Tsirkin
2021-05-03 8:00 ` Michael S. Tsirkin
[not found] <1620030574.9881887-1-xuanzhuo@linux.alibaba.com>
2021-05-06 3:23 ` Jason Wang
2021-05-06 3:23 ` Jason Wang
2021-04-22 15:16 Xuan Zhuo
2021-04-23 4:08 ` Jason Wang
2021-04-23 4:08 ` Jason Wang
2021-04-23 20:20 ` patchwork-bot+netdevbpf
2021-05-03 7:59 ` Michael S. Tsirkin
2021-05-03 7:59 ` Michael S. Tsirkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.