[PATCH net-next] virtio-net: fix use-after-free in skb_gro_receive

[PATCH net-next] virtio-net: fix use-after-free in skb_gro_receive

[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>
---
 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

Re: [PATCH net-next] virtio-net: fix use-after-free in skb_gro_receive

[Jason Wang]

在 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;
>   	}

Re: [PATCH net-next] virtio-net: fix use-after-free in skb_gro_receive

[patchwork-bot+netdevbpf]

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

Re: [PATCH net-next] virtio-net: fix use-after-free in skb_gro_receive

[Michael S. Tsirkin]

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

Re: [PATCH net-next] virtio-net: fix use-after-free in skb_gro_receive

[Jason Wang]

在 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


>

Re: [PATCH net-next] virtio-net: fix use-after-free in skb_gro_receive

[Michael S. Tsirkin]

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;
> > > >    	}

Re: [PATCH net-next] virtio-net: fix use-after-free in skb_gro_receive

[Jason Wang]

在 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;
>>>    	}