Thread (16 messages) 16 messages, 5 authors, 2021-03-02

Re: [PATCH] net/core/skbuff.c: __netdev_alloc_skb fix when len is greater than KMALLOC_MAX_SIZE

From: Pavel Skripkin <hidden>
Date: 2021-03-01 13:41:27
Also in: lkml

Hi, thanks for your reply!

On Mon, 2021-03-01 at 14:09 +0100, Eric Dumazet wrote:
On 2/26/21 8:11 PM, Pavel Skripkin wrote:
quoted
syzbot found WARNING in __alloc_pages_nodemask()[1] when order >=
MAX_ORDER.
It was caused by __netdev_alloc_skb(), which doesn't check len
value after adding NET_SKB_PAD.
Order will be >= MAX_ORDER and passed to __alloc_pages_nodemask()
if size > KMALLOC_MAX_SIZE.

static void *kmalloc_large_node(size_t size, gfp_t flags, int node)
{
	struct page *page;
	void *ptr = NULL;
	unsigned int order = get_order(size);
...
	page = alloc_pages_node(node, flags, order);
...

[1] WARNING in __alloc_pages_nodemask+0x5f8/0x730
mm/page_alloc.c:5014
Call Trace:
 __alloc_pages include/linux/gfp.h:511 [inline]
 __alloc_pages_node include/linux/gfp.h:524 [inline]
 alloc_pages_node include/linux/gfp.h:538 [inline]
 kmalloc_large_node+0x60/0x110 mm/slub.c:3999
 __kmalloc_node_track_caller+0x319/0x3f0 mm/slub.c:4496
 __kmalloc_reserve net/core/skbuff.c:150 [inline]
 __alloc_skb+0x4e4/0x5a0 net/core/skbuff.c:210
 __netdev_alloc_skb+0x70/0x400 net/core/skbuff.c:446
 netdev_alloc_skb include/linux/skbuff.h:2832 [inline]
 qrtr_endpoint_post+0x84/0x11b0 net/qrtr/qrtr.c:442
 qrtr_tun_write_iter+0x11f/0x1a0 net/qrtr/tun.c:98
 call_write_iter include/linux/fs.h:1901 [inline]
 new_sync_write+0x426/0x650 fs/read_write.c:518
 vfs_write+0x791/0xa30 fs/read_write.c:605
 ksys_write+0x12d/0x250 fs/read_write.c:658
 do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
 entry_SYSCALL_64_after_hwframe+0x44/0xa9

Reported-by: syzbot+80dccaee7c6630fa9dcf@syzkaller.appspotmail.com
Signed-off-by: Pavel Skripkin <redacted>
Change-Id: I480a6d6f818a4c0a387db0cd3f230b68a7daeb16
---
 net/core/skbuff.c | 3 +++
 1 file changed, 3 insertions(+)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 785daff48030..dc28c8f7bf5f 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -443,6 +443,9 @@ struct sk_buff *__netdev_alloc_skb(struct
net_device *dev, unsigned int len,
 	if (len <= SKB_WITH_OVERHEAD(1024) ||
 	    len > SKB_WITH_OVERHEAD(PAGE_SIZE) ||
 	    (gfp_mask & (__GFP_DIRECT_RECLAIM | GFP_DMA))) {
+		if (len > KMALLOC_MAX_SIZE)
+			return NULL;
+
 		skb = __alloc_skb(len, gfp_mask, SKB_ALLOC_RX,
NUMA_NO_NODE);
 		if (!skb)
 			goto skb_fail;

No, please fix the offender instead.
Yesterday I already send newer patch version to Alexander Lobakin,
where I added __GFP_NOWARN in qrtr_endpoint_post(). I think, You can
check it in this thread. 
Offender tentative fix was in 

commit 2a80c15812372e554474b1dba0b1d8e467af295d
Author: Sabyrzhan Tasbolatov [off-list ref]
Date:   Tue Feb 2 15:20:59 2021 +0600

    net/qrtr: restrict user-controlled length in
qrtr_tun_write_iter()
This patch fixes kzalloc() call, but the warning was caused by
__netdev_alloc_skb().  
qrtr maintainers have to tell us what is the maximum datagram length
they need to support.

With regards,
Pavel Skripkin

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help