netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net] sk_msg: Always cancel strp work before freeing the psock
@ 2019-01-28  9:13 Jakub Sitnicki
  2019-01-28 20:35 ` Song Liu
  2019-01-28 23:24 ` Daniel Borkmann
  0 siblings, 2 replies; 3+ messages in thread
From: Jakub Sitnicki @ 2019-01-28  9:13 UTC (permalink / raw)
  To: netdev; +Cc: John Fastabend, Daniel Borkmann, Marek Majkowski

Despite having stopped the parser, we still need to deinitialize it by
calling strp_done so that it cancels its work. Otherwise the worker
thread can run after we have freed the parser, and attempt to access its
workqueue resulting in a use-after-free:

==================================================================
BUG: KASAN: use-after-free in pwq_activate_delayed_work+0x1b/0x1d0
Read of size 8 at addr ffff888069975240 by task kworker/u2:2/93

CPU: 0 PID: 93 Comm: kworker/u2:2 Not tainted 5.0.0-rc2-00335-g28f9d1a3d4fe-dirty #14
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-2.fc27 04/01/2014
Workqueue:            (null) (kstrp)
Call Trace:
 print_address_description+0x6e/0x2b0
 ? pwq_activate_delayed_work+0x1b/0x1d0
 kasan_report+0xfd/0x177
 ? pwq_activate_delayed_work+0x1b/0x1d0
 ? pwq_activate_delayed_work+0x1b/0x1d0
 pwq_activate_delayed_work+0x1b/0x1d0
 ? process_one_work+0x4aa/0x660
 pwq_dec_nr_in_flight+0x9b/0x100
 worker_thread+0x82/0x680
 ? process_one_work+0x660/0x660
 kthread+0x1b9/0x1e0
 ? __kthread_create_on_node+0x250/0x250
 ret_from_fork+0x1f/0x30

Allocated by task 111:
 sk_psock_init+0x3c/0x1b0
 sock_map_link.isra.2+0x103/0x4b0
 sock_map_update_common+0x94/0x270
 sock_map_update_elem+0x145/0x160
 __se_sys_bpf+0x152e/0x1e10
 do_syscall_64+0xb2/0x3e0
 entry_SYSCALL_64_after_hwframe+0x44/0xa9

Freed by task 112:
 kfree+0x7f/0x140
 process_one_work+0x40b/0x660
 worker_thread+0x82/0x680
 kthread+0x1b9/0x1e0
 ret_from_fork+0x1f/0x30

The buggy address belongs to the object at ffff888069975180
 which belongs to the cache kmalloc-512 of size 512
The buggy address is located 192 bytes inside of
 512-byte region [ffff888069975180, ffff888069975380)
The buggy address belongs to the page:
page:ffffea0001a65d00 count:1 mapcount:0 mapping:ffff88806d401280 index:0x0 compound_mapcount: 0
flags: 0x4000000000010200(slab|head)
raw: 4000000000010200 dead000000000100 dead000000000200 ffff88806d401280
raw: 0000000000000000 00000000800c000c 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 ffff888069975100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff888069975180: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>ffff888069975200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
                                           ^
 ffff888069975280: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff888069975300: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================

Reported-by: Marek Majkowski <marek@cloudflare.com>
Link: https://lore.kernel.org/netdev/CAJPywTLwgXNEZ2dZVoa=udiZmtrWJ0q5SuBW64aYs0Y1khXX3A@mail.gmail.com
Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 net/core/skmsg.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index d6d5c20d7044..8c826603bf36 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -545,8 +545,7 @@ static void sk_psock_destroy_deferred(struct work_struct *gc)
 	struct sk_psock *psock = container_of(gc, struct sk_psock, gc);
 
 	/* No sk_callback_lock since already detached. */
-	if (psock->parser.enabled)
-		strp_done(&psock->parser.strp);
+	strp_done(&psock->parser.strp);
 
 	cancel_work_sync(&psock->work);
 
-- 
2.17.2


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH net] sk_msg: Always cancel strp work before freeing the psock
  2019-01-28  9:13 [PATCH net] sk_msg: Always cancel strp work before freeing the psock Jakub Sitnicki
@ 2019-01-28 20:35 ` Song Liu
  2019-01-28 23:24 ` Daniel Borkmann
  1 sibling, 0 replies; 3+ messages in thread
From: Song Liu @ 2019-01-28 20:35 UTC (permalink / raw)
  To: Jakub Sitnicki
  Cc: Networking, John Fastabend, Daniel Borkmann, Marek Majkowski

On Mon, Jan 28, 2019 at 1:15 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>
> Despite having stopped the parser, we still need to deinitialize it by
> calling strp_done so that it cancels its work. Otherwise the worker
> thread can run after we have freed the parser, and attempt to access its
> workqueue resulting in a use-after-free:
>
> ==================================================================
> BUG: KASAN: use-after-free in pwq_activate_delayed_work+0x1b/0x1d0
> Read of size 8 at addr ffff888069975240 by task kworker/u2:2/93
>
> CPU: 0 PID: 93 Comm: kworker/u2:2 Not tainted 5.0.0-rc2-00335-g28f9d1a3d4fe-dirty #14
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-2.fc27 04/01/2014
> Workqueue:            (null) (kstrp)
> Call Trace:
>  print_address_description+0x6e/0x2b0
>  ? pwq_activate_delayed_work+0x1b/0x1d0
>  kasan_report+0xfd/0x177
>  ? pwq_activate_delayed_work+0x1b/0x1d0
>  ? pwq_activate_delayed_work+0x1b/0x1d0
>  pwq_activate_delayed_work+0x1b/0x1d0
>  ? process_one_work+0x4aa/0x660
>  pwq_dec_nr_in_flight+0x9b/0x100
>  worker_thread+0x82/0x680
>  ? process_one_work+0x660/0x660
>  kthread+0x1b9/0x1e0
>  ? __kthread_create_on_node+0x250/0x250
>  ret_from_fork+0x1f/0x30
>
> Allocated by task 111:
>  sk_psock_init+0x3c/0x1b0
>  sock_map_link.isra.2+0x103/0x4b0
>  sock_map_update_common+0x94/0x270
>  sock_map_update_elem+0x145/0x160
>  __se_sys_bpf+0x152e/0x1e10
>  do_syscall_64+0xb2/0x3e0
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> Freed by task 112:
>  kfree+0x7f/0x140
>  process_one_work+0x40b/0x660
>  worker_thread+0x82/0x680
>  kthread+0x1b9/0x1e0
>  ret_from_fork+0x1f/0x30
>
> The buggy address belongs to the object at ffff888069975180
>  which belongs to the cache kmalloc-512 of size 512
> The buggy address is located 192 bytes inside of
>  512-byte region [ffff888069975180, ffff888069975380)
> The buggy address belongs to the page:
> page:ffffea0001a65d00 count:1 mapcount:0 mapping:ffff88806d401280 index:0x0 compound_mapcount: 0
> flags: 0x4000000000010200(slab|head)
> raw: 4000000000010200 dead000000000100 dead000000000200 ffff88806d401280
> raw: 0000000000000000 00000000800c000c 00000001ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
>  ffff888069975100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>  ffff888069975180: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> >ffff888069975200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>                                            ^
>  ffff888069975280: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>  ffff888069975300: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ==================================================================
>
> Reported-by: Marek Majkowski <marek@cloudflare.com>
> Link: https://lore.kernel.org/netdev/CAJPywTLwgXNEZ2dZVoa=udiZmtrWJ0q5SuBW64aYs0Y1khXX3A@mail.gmail.com
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>

Acked-by: Song Liu <songliubraving@fb.com>

> ---
>  net/core/skmsg.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index d6d5c20d7044..8c826603bf36 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -545,8 +545,7 @@ static void sk_psock_destroy_deferred(struct work_struct *gc)
>         struct sk_psock *psock = container_of(gc, struct sk_psock, gc);
>
>         /* No sk_callback_lock since already detached. */
> -       if (psock->parser.enabled)
> -               strp_done(&psock->parser.strp);
> +       strp_done(&psock->parser.strp);
>
>         cancel_work_sync(&psock->work);
>
> --
> 2.17.2
>

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH net] sk_msg: Always cancel strp work before freeing the psock
  2019-01-28  9:13 [PATCH net] sk_msg: Always cancel strp work before freeing the psock Jakub Sitnicki
  2019-01-28 20:35 ` Song Liu
@ 2019-01-28 23:24 ` Daniel Borkmann
  1 sibling, 0 replies; 3+ messages in thread
From: Daniel Borkmann @ 2019-01-28 23:24 UTC (permalink / raw)
  To: Jakub Sitnicki, netdev; +Cc: John Fastabend, Marek Majkowski

On 01/28/2019 10:13 AM, Jakub Sitnicki wrote:
> Despite having stopped the parser, we still need to deinitialize it by
> calling strp_done so that it cancels its work. Otherwise the worker
> thread can run after we have freed the parser, and attempt to access its
> workqueue resulting in a use-after-free:
> 
[...]
> Reported-by: Marek Majkowski <marek@cloudflare.com>
> Link: https://lore.kernel.org/netdev/CAJPywTLwgXNEZ2dZVoa=udiZmtrWJ0q5SuBW64aYs0Y1khXX3A@mail.gmail.com
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>

Applied, thanks!

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2019-01-28 23:24 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-28  9:13 [PATCH net] sk_msg: Always cancel strp work before freeing the psock Jakub Sitnicki
2019-01-28 20:35 ` Song Liu
2019-01-28 23:24 ` Daniel Borkmann

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).