netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net 0/2] net: skb_put_padto() fixes
@ 2020-09-09  8:27 Eric Dumazet
  2020-09-09  8:27 ` [PATCH net 1/2] net: qrtr: check skb_put_padto() return value Eric Dumazet
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Eric Dumazet @ 2020-09-09  8:27 UTC (permalink / raw)
  To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet

sysbot reported a bug in qrtr leading to use-after-free.

First patch fixes the issue.

Second patch addes __must_check attribute to avoid similar
issues in the future.

Eric Dumazet (2):
  net: qrtr: check skb_put_padto() return value
  net: add __must_check to skb_put_padto()

 include/linux/skbuff.h |  7 ++++---
 net/qrtr/qrtr.c        | 21 +++++++++++----------
 2 files changed, 15 insertions(+), 13 deletions(-)

-- 
2.28.0.526.ge36021eeef-goog


^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH net 1/2] net: qrtr: check skb_put_padto() return value
  2020-09-09  8:27 [PATCH net 0/2] net: skb_put_padto() fixes Eric Dumazet
@ 2020-09-09  8:27 ` Eric Dumazet
  2020-09-09 15:30   ` Manivannan Sadhasivam
  2020-09-09 16:41   ` Bjorn Andersson
  2020-09-09  8:27 ` [PATCH net 2/2] net: add __must_check to skb_put_padto() Eric Dumazet
  2020-09-09 18:05 ` [PATCH net 0/2] net: skb_put_padto() fixes David Miller
  2 siblings, 2 replies; 6+ messages in thread
From: Eric Dumazet @ 2020-09-09  8:27 UTC (permalink / raw)
  To: David S . Miller
  Cc: netdev, Eric Dumazet, Eric Dumazet, syzbot, Carl Huang, Wen Gong,
	Bjorn Andersson, Manivannan Sadhasivam

If skb_put_padto() returns an error, skb has been freed.
Better not touch it anymore, as reported by syzbot [1]

Note to qrtr maintainers : this suggests qrtr_sendmsg()
should adjust sock_alloc_send_skb() second parameter
to account for the potential added alignment to avoid
reallocation.

[1]

BUG: KASAN: use-after-free in __skb_insert include/linux/skbuff.h:1907 [inline]
BUG: KASAN: use-after-free in __skb_queue_before include/linux/skbuff.h:2016 [inline]
BUG: KASAN: use-after-free in __skb_queue_tail include/linux/skbuff.h:2049 [inline]
BUG: KASAN: use-after-free in skb_queue_tail+0x6b/0x120 net/core/skbuff.c:3146
Write of size 8 at addr ffff88804d8ab3c0 by task syz-executor.4/4316

CPU: 1 PID: 4316 Comm: syz-executor.4 Not tainted 5.9.0-rc4-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x1d6/0x29e lib/dump_stack.c:118
 print_address_description+0x66/0x620 mm/kasan/report.c:383
 __kasan_report mm/kasan/report.c:513 [inline]
 kasan_report+0x132/0x1d0 mm/kasan/report.c:530
 __skb_insert include/linux/skbuff.h:1907 [inline]
 __skb_queue_before include/linux/skbuff.h:2016 [inline]
 __skb_queue_tail include/linux/skbuff.h:2049 [inline]
 skb_queue_tail+0x6b/0x120 net/core/skbuff.c:3146
 qrtr_tun_send+0x1a/0x40 net/qrtr/tun.c:23
 qrtr_node_enqueue+0x44f/0xc00 net/qrtr/qrtr.c:364
 qrtr_bcast_enqueue+0xbe/0x140 net/qrtr/qrtr.c:861
 qrtr_sendmsg+0x680/0x9c0 net/qrtr/qrtr.c:960
 sock_sendmsg_nosec net/socket.c:651 [inline]
 sock_sendmsg net/socket.c:671 [inline]
 sock_write_iter+0x317/0x470 net/socket.c:998
 call_write_iter include/linux/fs.h:1882 [inline]
 new_sync_write fs/read_write.c:503 [inline]
 vfs_write+0xa96/0xd10 fs/read_write.c:578
 ksys_write+0x11b/0x220 fs/read_write.c:631
 do_syscall_64+0x31/0x70 arch/x86/entry/common.c:46
 entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x45d5b9
Code: 5d b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 2b b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f84b5b81c78 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 0000000000038b40 RCX: 000000000045d5b9
RDX: 0000000000000055 RSI: 0000000020001240 RDI: 0000000000000003
RBP: 00007f84b5b81ca0 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 000000000000000f
R13: 00007ffcbbf86daf R14: 00007f84b5b829c0 R15: 000000000118cf4c

Allocated by task 4316:
 kasan_save_stack mm/kasan/common.c:48 [inline]
 kasan_set_track mm/kasan/common.c:56 [inline]
 __kasan_kmalloc+0x100/0x130 mm/kasan/common.c:461
 slab_post_alloc_hook+0x3e/0x290 mm/slab.h:518
 slab_alloc mm/slab.c:3312 [inline]
 kmem_cache_alloc+0x1c1/0x2d0 mm/slab.c:3482
 skb_clone+0x1b2/0x370 net/core/skbuff.c:1449
 qrtr_bcast_enqueue+0x6d/0x140 net/qrtr/qrtr.c:857
 qrtr_sendmsg+0x680/0x9c0 net/qrtr/qrtr.c:960
 sock_sendmsg_nosec net/socket.c:651 [inline]
 sock_sendmsg net/socket.c:671 [inline]
 sock_write_iter+0x317/0x470 net/socket.c:998
 call_write_iter include/linux/fs.h:1882 [inline]
 new_sync_write fs/read_write.c:503 [inline]
 vfs_write+0xa96/0xd10 fs/read_write.c:578
 ksys_write+0x11b/0x220 fs/read_write.c:631
 do_syscall_64+0x31/0x70 arch/x86/entry/common.c:46
 entry_SYSCALL_64_after_hwframe+0x44/0xa9

Freed by task 4316:
 kasan_save_stack mm/kasan/common.c:48 [inline]
 kasan_set_track+0x3d/0x70 mm/kasan/common.c:56
 kasan_set_free_info+0x17/0x30 mm/kasan/generic.c:355
 __kasan_slab_free+0xdd/0x110 mm/kasan/common.c:422
 __cache_free mm/slab.c:3418 [inline]
 kmem_cache_free+0x82/0xf0 mm/slab.c:3693
 __skb_pad+0x3f5/0x5a0 net/core/skbuff.c:1823
 __skb_put_padto include/linux/skbuff.h:3233 [inline]
 skb_put_padto include/linux/skbuff.h:3252 [inline]
 qrtr_node_enqueue+0x62f/0xc00 net/qrtr/qrtr.c:360
 qrtr_bcast_enqueue+0xbe/0x140 net/qrtr/qrtr.c:861
 qrtr_sendmsg+0x680/0x9c0 net/qrtr/qrtr.c:960
 sock_sendmsg_nosec net/socket.c:651 [inline]
 sock_sendmsg net/socket.c:671 [inline]
 sock_write_iter+0x317/0x470 net/socket.c:998
 call_write_iter include/linux/fs.h:1882 [inline]
 new_sync_write fs/read_write.c:503 [inline]
 vfs_write+0xa96/0xd10 fs/read_write.c:578
 ksys_write+0x11b/0x220 fs/read_write.c:631
 do_syscall_64+0x31/0x70 arch/x86/entry/common.c:46
 entry_SYSCALL_64_after_hwframe+0x44/0xa9

The buggy address belongs to the object at ffff88804d8ab3c0
 which belongs to the cache skbuff_head_cache of size 224
The buggy address is located 0 bytes inside of
 224-byte region [ffff88804d8ab3c0, ffff88804d8ab4a0)
The buggy address belongs to the page:
page:00000000ea8cccfb refcount:1 mapcount:0 mapping:0000000000000000 index:0xffff88804d8abb40 pfn:0x4d8ab
flags: 0xfffe0000000200(slab)
raw: 00fffe0000000200 ffffea0002237ec8 ffffea00029b3388 ffff88821bb66800
raw: ffff88804d8abb40 ffff88804d8ab000 000000010000000b 0000000000000000
page dumped because: kasan: bad access detected

Fixes: ce57785bf91b ("net: qrtr: fix len of skb_put_padto in qrtr_node_enqueue")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: syzbot <syzkaller@googlegroups.com>
Cc: Carl Huang <cjhuang@codeaurora.org>
Cc: Wen Gong <wgong@codeaurora.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 net/qrtr/qrtr.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
index 90c558f89d46565ee5d5845d0ca97c095a8287a8..957aa9263ba4ce10dbb8d94c4e4bbcaef8b6e84b 100644
--- a/net/qrtr/qrtr.c
+++ b/net/qrtr/qrtr.c
@@ -332,8 +332,7 @@ static int qrtr_node_enqueue(struct qrtr_node *node, struct sk_buff *skb,
 {
 	struct qrtr_hdr_v1 *hdr;
 	size_t len = skb->len;
-	int rc = -ENODEV;
-	int confirm_rx;
+	int rc, confirm_rx;
 
 	confirm_rx = qrtr_tx_wait(node, to->sq_node, to->sq_port, type);
 	if (confirm_rx < 0) {
@@ -357,15 +356,17 @@ static int qrtr_node_enqueue(struct qrtr_node *node, struct sk_buff *skb,
 	hdr->size = cpu_to_le32(len);
 	hdr->confirm_rx = !!confirm_rx;
 
-	skb_put_padto(skb, ALIGN(len, 4) + sizeof(*hdr));
-
-	mutex_lock(&node->ep_lock);
-	if (node->ep)
-		rc = node->ep->xmit(node->ep, skb);
-	else
-		kfree_skb(skb);
-	mutex_unlock(&node->ep_lock);
+	rc = skb_put_padto(skb, ALIGN(len, 4) + sizeof(*hdr));
 
+	if (!rc) {
+		mutex_lock(&node->ep_lock);
+		rc = -ENODEV;
+		if (node->ep)
+			rc = node->ep->xmit(node->ep, skb);
+		else
+			kfree_skb(skb);
+		mutex_unlock(&node->ep_lock);
+	}
 	/* Need to ensure that a subsequent message carries the otherwise lost
 	 * confirm_rx flag if we dropped this one */
 	if (rc && confirm_rx)
-- 
2.28.0.526.ge36021eeef-goog


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH net 2/2] net: add __must_check to skb_put_padto()
  2020-09-09  8:27 [PATCH net 0/2] net: skb_put_padto() fixes Eric Dumazet
  2020-09-09  8:27 ` [PATCH net 1/2] net: qrtr: check skb_put_padto() return value Eric Dumazet
@ 2020-09-09  8:27 ` Eric Dumazet
  2020-09-09 18:05 ` [PATCH net 0/2] net: skb_put_padto() fixes David Miller
  2 siblings, 0 replies; 6+ messages in thread
From: Eric Dumazet @ 2020-09-09  8:27 UTC (permalink / raw)
  To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet

skb_put_padto() and __skb_put_padto() callers
must check return values or risk use-after-free.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/linux/skbuff.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index ed9bea924dc3e2cac674da7a4d1f7ed1fd741f9e..04a18e01b362e3f113168e7a21cb9e900394db43 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3223,8 +3223,9 @@ static inline int skb_padto(struct sk_buff *skb, unsigned int len)
  *	is untouched. Otherwise it is extended. Returns zero on
  *	success. The skb is freed on error if @free_on_error is true.
  */
-static inline int __skb_put_padto(struct sk_buff *skb, unsigned int len,
-				  bool free_on_error)
+static inline int __must_check __skb_put_padto(struct sk_buff *skb,
+					       unsigned int len,
+					       bool free_on_error)
 {
 	unsigned int size = skb->len;
 
@@ -3247,7 +3248,7 @@ static inline int __skb_put_padto(struct sk_buff *skb, unsigned int len,
  *	is untouched. Otherwise it is extended. Returns zero on
  *	success. The skb is freed on error.
  */
-static inline int skb_put_padto(struct sk_buff *skb, unsigned int len)
+static inline int __must_check skb_put_padto(struct sk_buff *skb, unsigned int len)
 {
 	return __skb_put_padto(skb, len, true);
 }
-- 
2.28.0.526.ge36021eeef-goog


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH net 1/2] net: qrtr: check skb_put_padto() return value
  2020-09-09  8:27 ` [PATCH net 1/2] net: qrtr: check skb_put_padto() return value Eric Dumazet
@ 2020-09-09 15:30   ` Manivannan Sadhasivam
  2020-09-09 16:41   ` Bjorn Andersson
  1 sibling, 0 replies; 6+ messages in thread
From: Manivannan Sadhasivam @ 2020-09-09 15:30 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: David S . Miller, netdev, Eric Dumazet, syzbot, Carl Huang,
	Wen Gong, Bjorn Andersson

On 0909, Eric Dumazet wrote:
> If skb_put_padto() returns an error, skb has been freed.
> Better not touch it anymore, as reported by syzbot [1]
> 
> Note to qrtr maintainers : this suggests qrtr_sendmsg()
> should adjust sock_alloc_send_skb() second parameter
> to account for the potential added alignment to avoid
> reallocation.
> 
> [1]
> 
> BUG: KASAN: use-after-free in __skb_insert include/linux/skbuff.h:1907 [inline]
> BUG: KASAN: use-after-free in __skb_queue_before include/linux/skbuff.h:2016 [inline]
> BUG: KASAN: use-after-free in __skb_queue_tail include/linux/skbuff.h:2049 [inline]
> BUG: KASAN: use-after-free in skb_queue_tail+0x6b/0x120 net/core/skbuff.c:3146
> Write of size 8 at addr ffff88804d8ab3c0 by task syz-executor.4/4316
> 
> CPU: 1 PID: 4316 Comm: syz-executor.4 Not tainted 5.9.0-rc4-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Call Trace:
>  __dump_stack lib/dump_stack.c:77 [inline]
>  dump_stack+0x1d6/0x29e lib/dump_stack.c:118
>  print_address_description+0x66/0x620 mm/kasan/report.c:383
>  __kasan_report mm/kasan/report.c:513 [inline]
>  kasan_report+0x132/0x1d0 mm/kasan/report.c:530
>  __skb_insert include/linux/skbuff.h:1907 [inline]
>  __skb_queue_before include/linux/skbuff.h:2016 [inline]
>  __skb_queue_tail include/linux/skbuff.h:2049 [inline]
>  skb_queue_tail+0x6b/0x120 net/core/skbuff.c:3146
>  qrtr_tun_send+0x1a/0x40 net/qrtr/tun.c:23
>  qrtr_node_enqueue+0x44f/0xc00 net/qrtr/qrtr.c:364
>  qrtr_bcast_enqueue+0xbe/0x140 net/qrtr/qrtr.c:861
>  qrtr_sendmsg+0x680/0x9c0 net/qrtr/qrtr.c:960
>  sock_sendmsg_nosec net/socket.c:651 [inline]
>  sock_sendmsg net/socket.c:671 [inline]
>  sock_write_iter+0x317/0x470 net/socket.c:998
>  call_write_iter include/linux/fs.h:1882 [inline]
>  new_sync_write fs/read_write.c:503 [inline]
>  vfs_write+0xa96/0xd10 fs/read_write.c:578
>  ksys_write+0x11b/0x220 fs/read_write.c:631
>  do_syscall_64+0x31/0x70 arch/x86/entry/common.c:46
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> RIP: 0033:0x45d5b9
> Code: 5d b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 2b b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00
> RSP: 002b:00007f84b5b81c78 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> RAX: ffffffffffffffda RBX: 0000000000038b40 RCX: 000000000045d5b9
> RDX: 0000000000000055 RSI: 0000000020001240 RDI: 0000000000000003
> RBP: 00007f84b5b81ca0 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 000000000000000f
> R13: 00007ffcbbf86daf R14: 00007f84b5b829c0 R15: 000000000118cf4c
> 
> Allocated by task 4316:
>  kasan_save_stack mm/kasan/common.c:48 [inline]
>  kasan_set_track mm/kasan/common.c:56 [inline]
>  __kasan_kmalloc+0x100/0x130 mm/kasan/common.c:461
>  slab_post_alloc_hook+0x3e/0x290 mm/slab.h:518
>  slab_alloc mm/slab.c:3312 [inline]
>  kmem_cache_alloc+0x1c1/0x2d0 mm/slab.c:3482
>  skb_clone+0x1b2/0x370 net/core/skbuff.c:1449
>  qrtr_bcast_enqueue+0x6d/0x140 net/qrtr/qrtr.c:857
>  qrtr_sendmsg+0x680/0x9c0 net/qrtr/qrtr.c:960
>  sock_sendmsg_nosec net/socket.c:651 [inline]
>  sock_sendmsg net/socket.c:671 [inline]
>  sock_write_iter+0x317/0x470 net/socket.c:998
>  call_write_iter include/linux/fs.h:1882 [inline]
>  new_sync_write fs/read_write.c:503 [inline]
>  vfs_write+0xa96/0xd10 fs/read_write.c:578
>  ksys_write+0x11b/0x220 fs/read_write.c:631
>  do_syscall_64+0x31/0x70 arch/x86/entry/common.c:46
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> Freed by task 4316:
>  kasan_save_stack mm/kasan/common.c:48 [inline]
>  kasan_set_track+0x3d/0x70 mm/kasan/common.c:56
>  kasan_set_free_info+0x17/0x30 mm/kasan/generic.c:355
>  __kasan_slab_free+0xdd/0x110 mm/kasan/common.c:422
>  __cache_free mm/slab.c:3418 [inline]
>  kmem_cache_free+0x82/0xf0 mm/slab.c:3693
>  __skb_pad+0x3f5/0x5a0 net/core/skbuff.c:1823
>  __skb_put_padto include/linux/skbuff.h:3233 [inline]
>  skb_put_padto include/linux/skbuff.h:3252 [inline]
>  qrtr_node_enqueue+0x62f/0xc00 net/qrtr/qrtr.c:360
>  qrtr_bcast_enqueue+0xbe/0x140 net/qrtr/qrtr.c:861
>  qrtr_sendmsg+0x680/0x9c0 net/qrtr/qrtr.c:960
>  sock_sendmsg_nosec net/socket.c:651 [inline]
>  sock_sendmsg net/socket.c:671 [inline]
>  sock_write_iter+0x317/0x470 net/socket.c:998
>  call_write_iter include/linux/fs.h:1882 [inline]
>  new_sync_write fs/read_write.c:503 [inline]
>  vfs_write+0xa96/0xd10 fs/read_write.c:578
>  ksys_write+0x11b/0x220 fs/read_write.c:631
>  do_syscall_64+0x31/0x70 arch/x86/entry/common.c:46
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> The buggy address belongs to the object at ffff88804d8ab3c0
>  which belongs to the cache skbuff_head_cache of size 224
> The buggy address is located 0 bytes inside of
>  224-byte region [ffff88804d8ab3c0, ffff88804d8ab4a0)
> The buggy address belongs to the page:
> page:00000000ea8cccfb refcount:1 mapcount:0 mapping:0000000000000000 index:0xffff88804d8abb40 pfn:0x4d8ab
> flags: 0xfffe0000000200(slab)
> raw: 00fffe0000000200 ffffea0002237ec8 ffffea00029b3388 ffff88821bb66800
> raw: ffff88804d8abb40 ffff88804d8ab000 000000010000000b 0000000000000000
> page dumped because: kasan: bad access detected
> 
> Fixes: ce57785bf91b ("net: qrtr: fix len of skb_put_padto in qrtr_node_enqueue")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> Reported-by: syzbot <syzkaller@googlegroups.com>
> Cc: Carl Huang <cjhuang@codeaurora.org>
> Cc: Wen Gong <wgong@codeaurora.org>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  net/qrtr/qrtr.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
> index 90c558f89d46565ee5d5845d0ca97c095a8287a8..957aa9263ba4ce10dbb8d94c4e4bbcaef8b6e84b 100644
> --- a/net/qrtr/qrtr.c
> +++ b/net/qrtr/qrtr.c
> @@ -332,8 +332,7 @@ static int qrtr_node_enqueue(struct qrtr_node *node, struct sk_buff *skb,
>  {
>  	struct qrtr_hdr_v1 *hdr;
>  	size_t len = skb->len;
> -	int rc = -ENODEV;
> -	int confirm_rx;
> +	int rc, confirm_rx;
>  
>  	confirm_rx = qrtr_tx_wait(node, to->sq_node, to->sq_port, type);
>  	if (confirm_rx < 0) {
> @@ -357,15 +356,17 @@ static int qrtr_node_enqueue(struct qrtr_node *node, struct sk_buff *skb,
>  	hdr->size = cpu_to_le32(len);
>  	hdr->confirm_rx = !!confirm_rx;
>  
> -	skb_put_padto(skb, ALIGN(len, 4) + sizeof(*hdr));
> -
> -	mutex_lock(&node->ep_lock);
> -	if (node->ep)
> -		rc = node->ep->xmit(node->ep, skb);
> -	else
> -		kfree_skb(skb);
> -	mutex_unlock(&node->ep_lock);
> +	rc = skb_put_padto(skb, ALIGN(len, 4) + sizeof(*hdr));
>  
> +	if (!rc) {
> +		mutex_lock(&node->ep_lock);
> +		rc = -ENODEV;
> +		if (node->ep)
> +			rc = node->ep->xmit(node->ep, skb);
> +		else
> +			kfree_skb(skb);
> +		mutex_unlock(&node->ep_lock);
> +	}
>  	/* Need to ensure that a subsequent message carries the otherwise lost
>  	 * confirm_rx flag if we dropped this one */
>  	if (rc && confirm_rx)
> -- 
> 2.28.0.526.ge36021eeef-goog
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH net 1/2] net: qrtr: check skb_put_padto() return value
  2020-09-09  8:27 ` [PATCH net 1/2] net: qrtr: check skb_put_padto() return value Eric Dumazet
  2020-09-09 15:30   ` Manivannan Sadhasivam
@ 2020-09-09 16:41   ` Bjorn Andersson
  1 sibling, 0 replies; 6+ messages in thread
From: Bjorn Andersson @ 2020-09-09 16:41 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: David S . Miller, netdev, Eric Dumazet, syzbot, Carl Huang,
	Wen Gong, Manivannan Sadhasivam

On Wed 09 Sep 03:27 CDT 2020, Eric Dumazet wrote:

> If skb_put_padto() returns an error, skb has been freed.
> Better not touch it anymore, as reported by syzbot [1]
> 
> Note to qrtr maintainers : this suggests qrtr_sendmsg()
> should adjust sock_alloc_send_skb() second parameter
> to account for the potential added alignment to avoid
> reallocation.
> 
> [1]
> 
> BUG: KASAN: use-after-free in __skb_insert include/linux/skbuff.h:1907 [inline]
> BUG: KASAN: use-after-free in __skb_queue_before include/linux/skbuff.h:2016 [inline]
> BUG: KASAN: use-after-free in __skb_queue_tail include/linux/skbuff.h:2049 [inline]
> BUG: KASAN: use-after-free in skb_queue_tail+0x6b/0x120 net/core/skbuff.c:3146
> Write of size 8 at addr ffff88804d8ab3c0 by task syz-executor.4/4316
> 
> CPU: 1 PID: 4316 Comm: syz-executor.4 Not tainted 5.9.0-rc4-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Call Trace:
>  __dump_stack lib/dump_stack.c:77 [inline]
>  dump_stack+0x1d6/0x29e lib/dump_stack.c:118
>  print_address_description+0x66/0x620 mm/kasan/report.c:383
>  __kasan_report mm/kasan/report.c:513 [inline]
>  kasan_report+0x132/0x1d0 mm/kasan/report.c:530
>  __skb_insert include/linux/skbuff.h:1907 [inline]
>  __skb_queue_before include/linux/skbuff.h:2016 [inline]
>  __skb_queue_tail include/linux/skbuff.h:2049 [inline]
>  skb_queue_tail+0x6b/0x120 net/core/skbuff.c:3146
>  qrtr_tun_send+0x1a/0x40 net/qrtr/tun.c:23
>  qrtr_node_enqueue+0x44f/0xc00 net/qrtr/qrtr.c:364
>  qrtr_bcast_enqueue+0xbe/0x140 net/qrtr/qrtr.c:861
>  qrtr_sendmsg+0x680/0x9c0 net/qrtr/qrtr.c:960
>  sock_sendmsg_nosec net/socket.c:651 [inline]
>  sock_sendmsg net/socket.c:671 [inline]
>  sock_write_iter+0x317/0x470 net/socket.c:998
>  call_write_iter include/linux/fs.h:1882 [inline]
>  new_sync_write fs/read_write.c:503 [inline]
>  vfs_write+0xa96/0xd10 fs/read_write.c:578
>  ksys_write+0x11b/0x220 fs/read_write.c:631
>  do_syscall_64+0x31/0x70 arch/x86/entry/common.c:46
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> RIP: 0033:0x45d5b9
> Code: 5d b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 2b b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00
> RSP: 002b:00007f84b5b81c78 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> RAX: ffffffffffffffda RBX: 0000000000038b40 RCX: 000000000045d5b9
> RDX: 0000000000000055 RSI: 0000000020001240 RDI: 0000000000000003
> RBP: 00007f84b5b81ca0 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 000000000000000f
> R13: 00007ffcbbf86daf R14: 00007f84b5b829c0 R15: 000000000118cf4c
> 
> Allocated by task 4316:
>  kasan_save_stack mm/kasan/common.c:48 [inline]
>  kasan_set_track mm/kasan/common.c:56 [inline]
>  __kasan_kmalloc+0x100/0x130 mm/kasan/common.c:461
>  slab_post_alloc_hook+0x3e/0x290 mm/slab.h:518
>  slab_alloc mm/slab.c:3312 [inline]
>  kmem_cache_alloc+0x1c1/0x2d0 mm/slab.c:3482
>  skb_clone+0x1b2/0x370 net/core/skbuff.c:1449
>  qrtr_bcast_enqueue+0x6d/0x140 net/qrtr/qrtr.c:857
>  qrtr_sendmsg+0x680/0x9c0 net/qrtr/qrtr.c:960
>  sock_sendmsg_nosec net/socket.c:651 [inline]
>  sock_sendmsg net/socket.c:671 [inline]
>  sock_write_iter+0x317/0x470 net/socket.c:998
>  call_write_iter include/linux/fs.h:1882 [inline]
>  new_sync_write fs/read_write.c:503 [inline]
>  vfs_write+0xa96/0xd10 fs/read_write.c:578
>  ksys_write+0x11b/0x220 fs/read_write.c:631
>  do_syscall_64+0x31/0x70 arch/x86/entry/common.c:46
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> Freed by task 4316:
>  kasan_save_stack mm/kasan/common.c:48 [inline]
>  kasan_set_track+0x3d/0x70 mm/kasan/common.c:56
>  kasan_set_free_info+0x17/0x30 mm/kasan/generic.c:355
>  __kasan_slab_free+0xdd/0x110 mm/kasan/common.c:422
>  __cache_free mm/slab.c:3418 [inline]
>  kmem_cache_free+0x82/0xf0 mm/slab.c:3693
>  __skb_pad+0x3f5/0x5a0 net/core/skbuff.c:1823
>  __skb_put_padto include/linux/skbuff.h:3233 [inline]
>  skb_put_padto include/linux/skbuff.h:3252 [inline]
>  qrtr_node_enqueue+0x62f/0xc00 net/qrtr/qrtr.c:360
>  qrtr_bcast_enqueue+0xbe/0x140 net/qrtr/qrtr.c:861
>  qrtr_sendmsg+0x680/0x9c0 net/qrtr/qrtr.c:960
>  sock_sendmsg_nosec net/socket.c:651 [inline]
>  sock_sendmsg net/socket.c:671 [inline]
>  sock_write_iter+0x317/0x470 net/socket.c:998
>  call_write_iter include/linux/fs.h:1882 [inline]
>  new_sync_write fs/read_write.c:503 [inline]
>  vfs_write+0xa96/0xd10 fs/read_write.c:578
>  ksys_write+0x11b/0x220 fs/read_write.c:631
>  do_syscall_64+0x31/0x70 arch/x86/entry/common.c:46
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> The buggy address belongs to the object at ffff88804d8ab3c0
>  which belongs to the cache skbuff_head_cache of size 224
> The buggy address is located 0 bytes inside of
>  224-byte region [ffff88804d8ab3c0, ffff88804d8ab4a0)
> The buggy address belongs to the page:
> page:00000000ea8cccfb refcount:1 mapcount:0 mapping:0000000000000000 index:0xffff88804d8abb40 pfn:0x4d8ab
> flags: 0xfffe0000000200(slab)
> raw: 00fffe0000000200 ffffea0002237ec8 ffffea00029b3388 ffff88821bb66800
> raw: ffff88804d8abb40 ffff88804d8ab000 000000010000000b 0000000000000000
> page dumped because: kasan: bad access detected
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Fixes: ce57785bf91b ("net: qrtr: fix len of skb_put_padto in qrtr_node_enqueue")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Cc: Carl Huang <cjhuang@codeaurora.org>
> Cc: Wen Gong <wgong@codeaurora.org>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  net/qrtr/qrtr.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
> index 90c558f89d46565ee5d5845d0ca97c095a8287a8..957aa9263ba4ce10dbb8d94c4e4bbcaef8b6e84b 100644
> --- a/net/qrtr/qrtr.c
> +++ b/net/qrtr/qrtr.c
> @@ -332,8 +332,7 @@ static int qrtr_node_enqueue(struct qrtr_node *node, struct sk_buff *skb,
>  {
>  	struct qrtr_hdr_v1 *hdr;
>  	size_t len = skb->len;
> -	int rc = -ENODEV;
> -	int confirm_rx;
> +	int rc, confirm_rx;
>  
>  	confirm_rx = qrtr_tx_wait(node, to->sq_node, to->sq_port, type);
>  	if (confirm_rx < 0) {
> @@ -357,15 +356,17 @@ static int qrtr_node_enqueue(struct qrtr_node *node, struct sk_buff *skb,
>  	hdr->size = cpu_to_le32(len);
>  	hdr->confirm_rx = !!confirm_rx;
>  
> -	skb_put_padto(skb, ALIGN(len, 4) + sizeof(*hdr));
> -
> -	mutex_lock(&node->ep_lock);
> -	if (node->ep)
> -		rc = node->ep->xmit(node->ep, skb);
> -	else
> -		kfree_skb(skb);
> -	mutex_unlock(&node->ep_lock);
> +	rc = skb_put_padto(skb, ALIGN(len, 4) + sizeof(*hdr));
>  
> +	if (!rc) {
> +		mutex_lock(&node->ep_lock);
> +		rc = -ENODEV;
> +		if (node->ep)
> +			rc = node->ep->xmit(node->ep, skb);
> +		else
> +			kfree_skb(skb);
> +		mutex_unlock(&node->ep_lock);
> +	}
>  	/* Need to ensure that a subsequent message carries the otherwise lost
>  	 * confirm_rx flag if we dropped this one */
>  	if (rc && confirm_rx)
> -- 
> 2.28.0.526.ge36021eeef-goog
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH net 0/2] net: skb_put_padto() fixes
  2020-09-09  8:27 [PATCH net 0/2] net: skb_put_padto() fixes Eric Dumazet
  2020-09-09  8:27 ` [PATCH net 1/2] net: qrtr: check skb_put_padto() return value Eric Dumazet
  2020-09-09  8:27 ` [PATCH net 2/2] net: add __must_check to skb_put_padto() Eric Dumazet
@ 2020-09-09 18:05 ` David Miller
  2 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2020-09-09 18:05 UTC (permalink / raw)
  To: edumazet; +Cc: netdev, eric.dumazet

From: Eric Dumazet <edumazet@google.com>
Date: Wed,  9 Sep 2020 01:27:38 -0700

> sysbot reported a bug in qrtr leading to use-after-free.
> 
> First patch fixes the issue.
> 
> Second patch addes __must_check attribute to avoid similar
> issues in the future.

Series applied and queued up for -stable, thanks!

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-09 18:05 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-09  8:27 [PATCH net 0/2] net: skb_put_padto() fixes Eric Dumazet
2020-09-09  8:27 ` [PATCH net 1/2] net: qrtr: check skb_put_padto() return value Eric Dumazet
2020-09-09 15:30   ` Manivannan Sadhasivam
2020-09-09 16:41   ` Bjorn Andersson
2020-09-09  8:27 ` [PATCH net 2/2] net: add __must_check to skb_put_padto() Eric Dumazet
2020-09-09 18:05 ` [PATCH net 0/2] net: skb_put_padto() fixes David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).