[PATCH v1 net] af_unix: Call sk_diag_fill() under the bucket lock.

[PATCH v1 net] af_unix: Call sk_diag_fill() under the bucket lock.

[Kuniyuki Iwashima]

Wei Chen reported sk->sk_socket can be NULL in sk_user_ns(). [0][1]

It seems that syzbot was dumping an AF_UNIX socket while closing it,
and there is a race below.

  unix_release_sock               unix_diag_handler_dump
  |                               `- unix_diag_get_exact
  |                                  |- unix_lookup_by_ino
  |                                  |  |- spin_lock(&net->unx.table.locks[i])
  |                                  |  |- sock_hold
  |                                  |  `- spin_unlock(&net->unx.table.locks[i])
  |- unix_remove_socket(net, sk)     |     /* after releasing this lock,
  |  /* from here, sk cannot be      |      * there is no guarantee that
  |   * seen in the hash table.      |      * sk is not SOCK_DEAD.
  |   */                             |      */
  |                                  |
  |- unix_state_lock(sk)             |
  |- sock_orphan(sk)                 `- sk_diag_fill
  |  |- sock_set_flag(sk, SOCK_DEAD)    `- sk_diag_dump_uid
  |  `- sk_set_socket(sk, NULL)            `- sk_user_ns
  `- unix_state_unlock(sk)                   `- sk->sk_socket->file->f_cred->user_ns
                                                /* NULL deref here */

After releasing the bucket lock, we cannot guarantee that the found
socket is still alive.  Then, we have to check the SOCK_DEAD flag
under unix_state_lock() and keep holding it unless we access the socket.

In this case, however, we cannot acquire unix_state_lock() in
unix_lookup_by_ino() because we lock it later in sk_diag_dump_peer(),
resulting in deadlock.

Instead, we do not release the bucket lock; then, we can safely access
sk->sk_socket later in sk_user_ns(), and there is no deadlock scenario.
We are already using this strategy in unix_diag_dump().

Note we have to call nlmsg_new() before unix_lookup_by_ino() not to
change the flag from GFP_KERNEL to GFP_ATOMIC.

[0]: https://lore.kernel.org/netdev/CAO4mrfdvyjFpokhNsiwZiP-wpdSD0AStcJwfKcKQdAALQ9_2Qw@mail.gmail.com/
[1]:
BUG: kernel NULL pointer dereference, address: 0000000000000270
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
PGD 12bbce067 P4D 12bbce067 PUD 12bc40067 PMD 0
Oops: 0000 [#1] PREEMPT SMP
CPU: 0 PID: 27942 Comm: syz-executor.0 Not tainted 6.1.0-rc5-next-20221118 #2
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
rel-1.13.0-48-gd9c812dda519-prebuilt.qemu.org 04/01/2014
RIP: 0010:sk_user_ns include/net/sock.h:920 [inline]
RIP: 0010:sk_diag_dump_uid net/unix/diag.c:119 [inline]
RIP: 0010:sk_diag_fill+0x77d/0x890 net/unix/diag.c:170
Code: 89 ef e8 66 d4 2d fd c7 44 24 40 00 00 00 00 49 8d 7c 24 18 e8
54 d7 2d fd 49 8b 5c 24 18 48 8d bb 70 02 00 00 e8 43 d7 2d fd <48> 8b
9b 70 02 00 00 48 8d 7b 10 e8 33 d7 2d fd 48 8b 5b 10 48 8d
RSP: 0018:ffffc90000d67968 EFLAGS: 00010246
RAX: ffff88812badaa48 RBX: 0000000000000000 RCX: ffffffff840d481d
RDX: 0000000000000465 RSI: 0000000000000000 RDI: 0000000000000270
RBP: ffffc90000d679a8 R08: 0000000000000277 R09: 0000000000000000
R10: 0001ffffffffffff R11: 0001c90000d679a8 R12: ffff88812ac03800
R13: ffff88812c87c400 R14: ffff88812ae42210 R15: ffff888103026940
FS:  00007f08b4e6f700(0000) GS:ffff88813bc00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000270 CR3: 000000012c58b000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <TASK>
 unix_diag_get_exact net/unix/diag.c:285 [inline]
 unix_diag_handler_dump+0x3f9/0x500 net/unix/diag.c:317
 __sock_diag_cmd net/core/sock_diag.c:235 [inline]
 sock_diag_rcv_msg+0x237/0x250 net/core/sock_diag.c:266
 netlink_rcv_skb+0x13e/0x250 net/netlink/af_netlink.c:2564
 sock_diag_rcv+0x24/0x40 net/core/sock_diag.c:277
 netlink_unicast_kernel net/netlink/af_netlink.c:1330 [inline]
 netlink_unicast+0x5e9/0x6b0 net/netlink/af_netlink.c:1356
 netlink_sendmsg+0x739/0x860 net/netlink/af_netlink.c:1932
 sock_sendmsg_nosec net/socket.c:714 [inline]
 sock_sendmsg net/socket.c:734 [inline]
 ____sys_sendmsg+0x38f/0x500 net/socket.c:2476
 ___sys_sendmsg net/socket.c:2530 [inline]
 __sys_sendmsg+0x197/0x230 net/socket.c:2559
 __do_sys_sendmsg net/socket.c:2568 [inline]
 __se_sys_sendmsg net/socket.c:2566 [inline]
 __x64_sys_sendmsg+0x42/0x50 net/socket.c:2566
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x2b/0x70 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x4697f9
Code: f7 d8 64 89 02 b8 ff ff ff ff c3 66 0f 1f 44 00 00 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 73 01 c3 48 c7 c1 bc ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f08b4e6ec48 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 000000000077bf80 RCX: 00000000004697f9
RDX: 0000000000000000 RSI: 00000000200001c0 RDI: 0000000000000003
RBP: 00000000004d29e9 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 000000000077bf80
R13: 0000000000000000 R14: 000000000077bf80 R15: 00007ffdb36bc6c0
 </TASK>
Modules linked in:
CR2: 0000000000000270

Fixes: 5d3cae8bc39d ("unix_diag: Dumping exact socket core")
Reported-by: syzbot <syzkaller@googlegroups.com>
Reported-by: Wei Chen <harperchen1110@gmail.com>
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/unix/diag.c | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/net/unix/diag.c b/net/unix/diag.c
index 105f522a89fe..96583cb71cf5 100644
--- a/net/unix/diag.c
+++ b/net/unix/diag.c
@@ -242,8 +242,9 @@ static struct sock *unix_lookup_by_ino(struct net *net, unsigned int ino)
 		spin_lock(&net->unx.table.locks[i]);
 		sk_for_each(sk, &net->unx.table.buckets[i]) {
 			if (ino == sock_i_ino(sk)) {
-				sock_hold(sk);
-				spin_unlock(&net->unx.table.locks[i]);
+				/* sk_diag_fill() must be done under the bucket
+				 * lock not to race with unix_release_sock().
+				 */
 				return sk;
 			}
 		}
@@ -264,15 +265,6 @@ static int unix_diag_get_exact(struct sk_buff *in_skb,
 
 	err = -EINVAL;
 	if (req->udiag_ino == 0)
-		goto out_nosk;
-
-	sk = unix_lookup_by_ino(net, req->udiag_ino);
-	err = -ENOENT;
-	if (sk == NULL)
-		goto out_nosk;
-
-	err = sock_diag_check_cookie(sk, req->udiag_cookie);
-	if (err)
 		goto out;
 
 	extra_len = 256;
@@ -282,8 +274,21 @@ static int unix_diag_get_exact(struct sk_buff *in_skb,
 	if (!rep)
 		goto out;
 
+	/* Acquire a bucket lock on success. */
+	sk = unix_lookup_by_ino(net, req->udiag_ino);
+	err = -ENOENT;
+	if (!sk)
+		goto free;
+
+	err = sock_diag_check_cookie(sk, req->udiag_cookie);
+	if (err)
+		goto unlock;
+
 	err = sk_diag_fill(sk, rep, req, NETLINK_CB(in_skb).portid,
 			   nlh->nlmsg_seq, 0, req->udiag_ino);
+
+	spin_unlock(&net->unx.table.locks[sk->sk_hash]);
+
 	if (err < 0) {
 		nlmsg_free(rep);
 		extra_len += 256;
@@ -292,13 +297,16 @@ static int unix_diag_get_exact(struct sk_buff *in_skb,
 
 		goto again;
 	}
-	err = nlmsg_unicast(net->diag_nlsk, rep, NETLINK_CB(in_skb).portid);
 
+	err = nlmsg_unicast(net->diag_nlsk, rep, NETLINK_CB(in_skb).portid);
 out:
-	if (sk)
-		sock_put(sk);
-out_nosk:
 	return err;
+
+unlock:
+	spin_unlock(&net->unx.table.locks[sk->sk_hash]);
+free:
+	nlmsg_free(rep);
+	goto out;
 }
 
 static int unix_diag_handler_dump(struct sk_buff *skb, struct nlmsghdr *h)
-- 
2.30.2

Re: [PATCH v1 net] af_unix: Call sk_diag_fill() under the bucket lock.

[Paolo Abeni]

On Tue, 2022-11-22 at 12:58 -0800, Kuniyuki Iwashima wrote:
> Wei Chen reported sk->sk_socket can be NULL in sk_user_ns(). [0][1]
> 
> It seems that syzbot was dumping an AF_UNIX socket while closing it,
> and there is a race below.
> 
>   unix_release_sock               unix_diag_handler_dump
>   |                               `- unix_diag_get_exact
>   |                                  |- unix_lookup_by_ino
>   |                                  |  |- spin_lock(&net->unx.table.locks[i])
>   |                                  |  |- sock_hold
>   |                                  |  `- spin_unlock(&net->unx.table.locks[i])
>   |- unix_remove_socket(net, sk)     |     /* after releasing this lock,
>   |  /* from here, sk cannot be      |      * there is no guarantee that
>   |   * seen in the hash table.      |      * sk is not SOCK_DEAD.
>   |   */                             |      */
>   |                                  |
>   |- unix_state_lock(sk)             |
>   |- sock_orphan(sk)                 `- sk_diag_fill
>   |  |- sock_set_flag(sk, SOCK_DEAD)    `- sk_diag_dump_uid
>   |  `- sk_set_socket(sk, NULL)            `- sk_user_ns
>   `- unix_state_unlock(sk)                   `- sk->sk_socket->file->f_cred->user_ns
>                                                 /* NULL deref here */
> 
> After releasing the bucket lock, we cannot guarantee that the found
> socket is still alive.  Then, we have to check the SOCK_DEAD flag
> under unix_state_lock() and keep holding it unless we access the socket.
> 
> In this case, however, we cannot acquire unix_state_lock() in
> unix_lookup_by_ino() because we lock it later in sk_diag_dump_peer(),
> resulting in deadlock.
> 
> Instead, we do not release the bucket lock; then, we can safely access
> sk->sk_socket later in sk_user_ns(), and there is no deadlock scenario.
> We are already using this strategy in unix_diag_dump().
> 
> Note we have to call nlmsg_new() before unix_lookup_by_ino() not to
> change the flag from GFP_KERNEL to GFP_ATOMIC.
> 
> [0]: https://lore.kernel.org/netdev/CAO4mrfdvyjFpokhNsiwZiP-wpdSD0AStcJwfKcKQdAALQ9_2Qw@mail.gmail.com/
> [1]:
> BUG: kernel NULL pointer dereference, address: 0000000000000270
> #PF: supervisor read access in kernel mode
> #PF: error_code(0x0000) - not-present page
> PGD 12bbce067 P4D 12bbce067 PUD 12bc40067 PMD 0
> Oops: 0000 [#1] PREEMPT SMP
> CPU: 0 PID: 27942 Comm: syz-executor.0 Not tainted 6.1.0-rc5-next-20221118 #2
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> rel-1.13.0-48-gd9c812dda519-prebuilt.qemu.org 04/01/2014
> RIP: 0010:sk_user_ns include/net/sock.h:920 [inline]
> RIP: 0010:sk_diag_dump_uid net/unix/diag.c:119 [inline]
> RIP: 0010:sk_diag_fill+0x77d/0x890 net/unix/diag.c:170
> Code: 89 ef e8 66 d4 2d fd c7 44 24 40 00 00 00 00 49 8d 7c 24 18 e8
> 54 d7 2d fd 49 8b 5c 24 18 48 8d bb 70 02 00 00 e8 43 d7 2d fd <48> 8b
> 9b 70 02 00 00 48 8d 7b 10 e8 33 d7 2d fd 48 8b 5b 10 48 8d
> RSP: 0018:ffffc90000d67968 EFLAGS: 00010246
> RAX: ffff88812badaa48 RBX: 0000000000000000 RCX: ffffffff840d481d
> RDX: 0000000000000465 RSI: 0000000000000000 RDI: 0000000000000270
> RBP: ffffc90000d679a8 R08: 0000000000000277 R09: 0000000000000000
> R10: 0001ffffffffffff R11: 0001c90000d679a8 R12: ffff88812ac03800
> R13: ffff88812c87c400 R14: ffff88812ae42210 R15: ffff888103026940
> FS:  00007f08b4e6f700(0000) GS:ffff88813bc00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000270 CR3: 000000012c58b000 CR4: 00000000003506f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>  <TASK>
>  unix_diag_get_exact net/unix/diag.c:285 [inline]
>  unix_diag_handler_dump+0x3f9/0x500 net/unix/diag.c:317
>  __sock_diag_cmd net/core/sock_diag.c:235 [inline]
>  sock_diag_rcv_msg+0x237/0x250 net/core/sock_diag.c:266
>  netlink_rcv_skb+0x13e/0x250 net/netlink/af_netlink.c:2564
>  sock_diag_rcv+0x24/0x40 net/core/sock_diag.c:277
>  netlink_unicast_kernel net/netlink/af_netlink.c:1330 [inline]
>  netlink_unicast+0x5e9/0x6b0 net/netlink/af_netlink.c:1356
>  netlink_sendmsg+0x739/0x860 net/netlink/af_netlink.c:1932
>  sock_sendmsg_nosec net/socket.c:714 [inline]
>  sock_sendmsg net/socket.c:734 [inline]
>  ____sys_sendmsg+0x38f/0x500 net/socket.c:2476
>  ___sys_sendmsg net/socket.c:2530 [inline]
>  __sys_sendmsg+0x197/0x230 net/socket.c:2559
>  __do_sys_sendmsg net/socket.c:2568 [inline]
>  __se_sys_sendmsg net/socket.c:2566 [inline]
>  __x64_sys_sendmsg+0x42/0x50 net/socket.c:2566
>  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>  do_syscall_64+0x2b/0x70 arch/x86/entry/common.c:80
>  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> RIP: 0033:0x4697f9
> Code: f7 d8 64 89 02 b8 ff ff ff ff c3 66 0f 1f 44 00 00 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 73 01 c3 48 c7 c1 bc ff ff ff f7 d8 64 89 01 48
> RSP: 002b:00007f08b4e6ec48 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> RAX: ffffffffffffffda RBX: 000000000077bf80 RCX: 00000000004697f9
> RDX: 0000000000000000 RSI: 00000000200001c0 RDI: 0000000000000003
> RBP: 00000000004d29e9 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 000000000077bf80
> R13: 0000000000000000 R14: 000000000077bf80 R15: 00007ffdb36bc6c0
>  </TASK>
> Modules linked in:
> CR2: 0000000000000270

I *think* that the root cause of the above splat could be
different/simpler. In unix_diag_get_exact():

	
	rep = nlmsg_new(sizeof(struct unix_diag_msg) + extra_len, GFP_KERNEL);
        if (!rep)
                goto out;

	// rep->sk is NULL now.
        err = sk_diag_fill(sk, rep, req, NETLINK_CB(in_skb).portid,
                           nlh->nlmsg_seq, 0, req->udiag_ino);

and sk_diag_fill() will splats deterministically. Note that the user
space don't trigger that code path usually, but it can be reproduced
with a sligthly modified 'ss' version.


Thanks,

Paolo

Re: [PATCH v1 net] af_unix: Call sk_diag_fill() under the bucket lock.

[Wei Chen]

Dear Paolo,

Could you explain the meaning of modified "ss" version to reproduce
the bug? I'd like to learn how to reproduce the bug in the user space
to facilitate the bug fix.

Best,
Wei

On Wed, 23 Nov 2022 at 18:26, Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Tue, 2022-11-22 at 12:58 -0800, Kuniyuki Iwashima wrote:
> > Wei Chen reported sk->sk_socket can be NULL in sk_user_ns(). [0][1]
> >
> > It seems that syzbot was dumping an AF_UNIX socket while closing it,
> > and there is a race below.
> >
> >   unix_release_sock               unix_diag_handler_dump
> >   |                               `- unix_diag_get_exact
> >   |                                  |- unix_lookup_by_ino
> >   |                                  |  |- spin_lock(&net->unx.table.locks[i])
> >   |                                  |  |- sock_hold
> >   |                                  |  `- spin_unlock(&net->unx.table.locks[i])
> >   |- unix_remove_socket(net, sk)     |     /* after releasing this lock,
> >   |  /* from here, sk cannot be      |      * there is no guarantee that
> >   |   * seen in the hash table.      |      * sk is not SOCK_DEAD.
> >   |   */                             |      */
> >   |                                  |
> >   |- unix_state_lock(sk)             |
> >   |- sock_orphan(sk)                 `- sk_diag_fill
> >   |  |- sock_set_flag(sk, SOCK_DEAD)    `- sk_diag_dump_uid
> >   |  `- sk_set_socket(sk, NULL)            `- sk_user_ns
> >   `- unix_state_unlock(sk)                   `- sk->sk_socket->file->f_cred->user_ns
> >                                                 /* NULL deref here */
> >
> > After releasing the bucket lock, we cannot guarantee that the found
> > socket is still alive.  Then, we have to check the SOCK_DEAD flag
> > under unix_state_lock() and keep holding it unless we access the socket.
> >
> > In this case, however, we cannot acquire unix_state_lock() in
> > unix_lookup_by_ino() because we lock it later in sk_diag_dump_peer(),
> > resulting in deadlock.
> >
> > Instead, we do not release the bucket lock; then, we can safely access
> > sk->sk_socket later in sk_user_ns(), and there is no deadlock scenario.
> > We are already using this strategy in unix_diag_dump().
> >
> > Note we have to call nlmsg_new() before unix_lookup_by_ino() not to
> > change the flag from GFP_KERNEL to GFP_ATOMIC.
> >
> > [0]: https://lore.kernel.org/netdev/CAO4mrfdvyjFpokhNsiwZiP-wpdSD0AStcJwfKcKQdAALQ9_2Qw@mail.gmail.com/
> > [1]:
> > BUG: kernel NULL pointer dereference, address: 0000000000000270
> > #PF: supervisor read access in kernel mode
> > #PF: error_code(0x0000) - not-present page
> > PGD 12bbce067 P4D 12bbce067 PUD 12bc40067 PMD 0
> > Oops: 0000 [#1] PREEMPT SMP
> > CPU: 0 PID: 27942 Comm: syz-executor.0 Not tainted 6.1.0-rc5-next-20221118 #2
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> > rel-1.13.0-48-gd9c812dda519-prebuilt.qemu.org 04/01/2014
> > RIP: 0010:sk_user_ns include/net/sock.h:920 [inline]
> > RIP: 0010:sk_diag_dump_uid net/unix/diag.c:119 [inline]
> > RIP: 0010:sk_diag_fill+0x77d/0x890 net/unix/diag.c:170
> > Code: 89 ef e8 66 d4 2d fd c7 44 24 40 00 00 00 00 49 8d 7c 24 18 e8
> > 54 d7 2d fd 49 8b 5c 24 18 48 8d bb 70 02 00 00 e8 43 d7 2d fd <48> 8b
> > 9b 70 02 00 00 48 8d 7b 10 e8 33 d7 2d fd 48 8b 5b 10 48 8d
> > RSP: 0018:ffffc90000d67968 EFLAGS: 00010246
> > RAX: ffff88812badaa48 RBX: 0000000000000000 RCX: ffffffff840d481d
> > RDX: 0000000000000465 RSI: 0000000000000000 RDI: 0000000000000270
> > RBP: ffffc90000d679a8 R08: 0000000000000277 R09: 0000000000000000
> > R10: 0001ffffffffffff R11: 0001c90000d679a8 R12: ffff88812ac03800
> > R13: ffff88812c87c400 R14: ffff88812ae42210 R15: ffff888103026940
> > FS:  00007f08b4e6f700(0000) GS:ffff88813bc00000(0000) knlGS:0000000000000000
> > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 0000000000000270 CR3: 000000012c58b000 CR4: 00000000003506f0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > Call Trace:
> >  <TASK>
> >  unix_diag_get_exact net/unix/diag.c:285 [inline]
> >  unix_diag_handler_dump+0x3f9/0x500 net/unix/diag.c:317
> >  __sock_diag_cmd net/core/sock_diag.c:235 [inline]
> >  sock_diag_rcv_msg+0x237/0x250 net/core/sock_diag.c:266
> >  netlink_rcv_skb+0x13e/0x250 net/netlink/af_netlink.c:2564
> >  sock_diag_rcv+0x24/0x40 net/core/sock_diag.c:277
> >  netlink_unicast_kernel net/netlink/af_netlink.c:1330 [inline]
> >  netlink_unicast+0x5e9/0x6b0 net/netlink/af_netlink.c:1356
> >  netlink_sendmsg+0x739/0x860 net/netlink/af_netlink.c:1932
> >  sock_sendmsg_nosec net/socket.c:714 [inline]
> >  sock_sendmsg net/socket.c:734 [inline]
> >  ____sys_sendmsg+0x38f/0x500 net/socket.c:2476
> >  ___sys_sendmsg net/socket.c:2530 [inline]
> >  __sys_sendmsg+0x197/0x230 net/socket.c:2559
> >  __do_sys_sendmsg net/socket.c:2568 [inline]
> >  __se_sys_sendmsg net/socket.c:2566 [inline]
> >  __x64_sys_sendmsg+0x42/0x50 net/socket.c:2566
> >  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> >  do_syscall_64+0x2b/0x70 arch/x86/entry/common.c:80
> >  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> > RIP: 0033:0x4697f9
> > Code: f7 d8 64 89 02 b8 ff ff ff ff c3 66 0f 1f 44 00 00 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 73 01 c3 48 c7 c1 bc ff ff ff f7 d8 64 89 01 48
> > RSP: 002b:00007f08b4e6ec48 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> > RAX: ffffffffffffffda RBX: 000000000077bf80 RCX: 00000000004697f9
> > RDX: 0000000000000000 RSI: 00000000200001c0 RDI: 0000000000000003
> > RBP: 00000000004d29e9 R08: 0000000000000000 R09: 0000000000000000
> > R10: 0000000000000000 R11: 0000000000000246 R12: 000000000077bf80
> > R13: 0000000000000000 R14: 000000000077bf80 R15: 00007ffdb36bc6c0
> >  </TASK>
> > Modules linked in:
> > CR2: 0000000000000270
>
> I *think* that the root cause of the above splat could be
> different/simpler. In unix_diag_get_exact():
>
>
>         rep = nlmsg_new(sizeof(struct unix_diag_msg) + extra_len, GFP_KERNEL);
>         if (!rep)
>                 goto out;
>
>         // rep->sk is NULL now.
>         err = sk_diag_fill(sk, rep, req, NETLINK_CB(in_skb).portid,
>                            nlh->nlmsg_seq, 0, req->udiag_ino);
>
> and sk_diag_fill() will splats deterministically. Note that the user
> space don't trigger that code path usually, but it can be reproduced
> with a sligthly modified 'ss' version.
>
>
> Thanks,
>
> Paolo
>

Re: [PATCH v1 net] af_unix: Call sk_diag_fill() under the bucket lock.

[Kuniyuki Iwashima]

From:   Wei Chen <harperchen1110@gmail.com>
Date:   Wed, 23 Nov 2022 23:09:53 +0800
> Dear Paolo,
> 
> Could you explain the meaning of modified "ss" version to reproduce
> the bug? I'd like to learn how to reproduce the bug in the user space
> to facilitate the bug fix.

I think it means to drop NLM_F_DUMP and modify args as needed because
ss dumps all sockets, not exactly a single socket.

And please do not top-post :)

> 
> Best,
> Wei
> 
> On Wed, 23 Nov 2022 at 18:26, Paolo Abeni <pabeni@redhat.com> wrote:
> >
> > On Tue, 2022-11-22 at 12:58 -0800, Kuniyuki Iwashima wrote:
> > > Wei Chen reported sk->sk_socket can be NULL in sk_user_ns(). [0][1]
> > >
> > > It seems that syzbot was dumping an AF_UNIX socket while closing it,
> > > and there is a race below.
> > >
> > >   unix_release_sock               unix_diag_handler_dump
> > >   |                               `- unix_diag_get_exact
> > >   |                                  |- unix_lookup_by_ino
> > >   |                                  |  |- spin_lock(&net->unx.table.locks[i])
> > >   |                                  |  |- sock_hold
> > >   |                                  |  `- spin_unlock(&net->unx.table.locks[i])
> > >   |- unix_remove_socket(net, sk)     |     /* after releasing this lock,
> > >   |  /* from here, sk cannot be      |      * there is no guarantee that
> > >   |   * seen in the hash table.      |      * sk is not SOCK_DEAD.
> > >   |   */                             |      */
> > >   |                                  |
> > >   |- unix_state_lock(sk)             |
> > >   |- sock_orphan(sk)                 `- sk_diag_fill
> > >   |  |- sock_set_flag(sk, SOCK_DEAD)    `- sk_diag_dump_uid
> > >   |  `- sk_set_socket(sk, NULL)            `- sk_user_ns
> > >   `- unix_state_unlock(sk)                   `- sk->sk_socket->file->f_cred->user_ns
> > >                                                 /* NULL deref here */
> > >
> > > After releasing the bucket lock, we cannot guarantee that the found
> > > socket is still alive.  Then, we have to check the SOCK_DEAD flag
> > > under unix_state_lock() and keep holding it unless we access the socket.
> > >
> > > In this case, however, we cannot acquire unix_state_lock() in
> > > unix_lookup_by_ino() because we lock it later in sk_diag_dump_peer(),
> > > resulting in deadlock.
> > >
> > > Instead, we do not release the bucket lock; then, we can safely access
> > > sk->sk_socket later in sk_user_ns(), and there is no deadlock scenario.
> > > We are already using this strategy in unix_diag_dump().
> > >
> > > Note we have to call nlmsg_new() before unix_lookup_by_ino() not to
> > > change the flag from GFP_KERNEL to GFP_ATOMIC.
> > >
> > > [0]: https://lore.kernel.org/netdev/CAO4mrfdvyjFpokhNsiwZiP-wpdSD0AStcJwfKcKQdAALQ9_2Qw@mail.gmail.com/
> > > [1]:
> > > BUG: kernel NULL pointer dereference, address: 0000000000000270
> > > #PF: supervisor read access in kernel mode
> > > #PF: error_code(0x0000) - not-present page
> > > PGD 12bbce067 P4D 12bbce067 PUD 12bc40067 PMD 0
> > > Oops: 0000 [#1] PREEMPT SMP
> > > CPU: 0 PID: 27942 Comm: syz-executor.0 Not tainted 6.1.0-rc5-next-20221118 #2
> > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> > > rel-1.13.0-48-gd9c812dda519-prebuilt.qemu.org 04/01/2014
> > > RIP: 0010:sk_user_ns include/net/sock.h:920 [inline]
> > > RIP: 0010:sk_diag_dump_uid net/unix/diag.c:119 [inline]
> > > RIP: 0010:sk_diag_fill+0x77d/0x890 net/unix/diag.c:170
> > > Code: 89 ef e8 66 d4 2d fd c7 44 24 40 00 00 00 00 49 8d 7c 24 18 e8
> > > 54 d7 2d fd 49 8b 5c 24 18 48 8d bb 70 02 00 00 e8 43 d7 2d fd <48> 8b
> > > 9b 70 02 00 00 48 8d 7b 10 e8 33 d7 2d fd 48 8b 5b 10 48 8d
> > > RSP: 0018:ffffc90000d67968 EFLAGS: 00010246
> > > RAX: ffff88812badaa48 RBX: 0000000000000000 RCX: ffffffff840d481d
> > > RDX: 0000000000000465 RSI: 0000000000000000 RDI: 0000000000000270
> > > RBP: ffffc90000d679a8 R08: 0000000000000277 R09: 0000000000000000
> > > R10: 0001ffffffffffff R11: 0001c90000d679a8 R12: ffff88812ac03800
> > > R13: ffff88812c87c400 R14: ffff88812ae42210 R15: ffff888103026940
> > > FS:  00007f08b4e6f700(0000) GS:ffff88813bc00000(0000) knlGS:0000000000000000
> > > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > CR2: 0000000000000270 CR3: 000000012c58b000 CR4: 00000000003506f0
> > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > Call Trace:
> > >  <TASK>
> > >  unix_diag_get_exact net/unix/diag.c:285 [inline]
> > >  unix_diag_handler_dump+0x3f9/0x500 net/unix/diag.c:317
> > >  __sock_diag_cmd net/core/sock_diag.c:235 [inline]
> > >  sock_diag_rcv_msg+0x237/0x250 net/core/sock_diag.c:266
> > >  netlink_rcv_skb+0x13e/0x250 net/netlink/af_netlink.c:2564
> > >  sock_diag_rcv+0x24/0x40 net/core/sock_diag.c:277
> > >  netlink_unicast_kernel net/netlink/af_netlink.c:1330 [inline]
> > >  netlink_unicast+0x5e9/0x6b0 net/netlink/af_netlink.c:1356
> > >  netlink_sendmsg+0x739/0x860 net/netlink/af_netlink.c:1932
> > >  sock_sendmsg_nosec net/socket.c:714 [inline]
> > >  sock_sendmsg net/socket.c:734 [inline]
> > >  ____sys_sendmsg+0x38f/0x500 net/socket.c:2476
> > >  ___sys_sendmsg net/socket.c:2530 [inline]
> > >  __sys_sendmsg+0x197/0x230 net/socket.c:2559
> > >  __do_sys_sendmsg net/socket.c:2568 [inline]
> > >  __se_sys_sendmsg net/socket.c:2566 [inline]
> > >  __x64_sys_sendmsg+0x42/0x50 net/socket.c:2566
> > >  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> > >  do_syscall_64+0x2b/0x70 arch/x86/entry/common.c:80
> > >  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> > > RIP: 0033:0x4697f9
> > > Code: f7 d8 64 89 02 b8 ff ff ff ff c3 66 0f 1f 44 00 00 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 73 01 c3 48 c7 c1 bc ff ff ff f7 d8 64 89 01 48
> > > RSP: 002b:00007f08b4e6ec48 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> > > RAX: ffffffffffffffda RBX: 000000000077bf80 RCX: 00000000004697f9
> > > RDX: 0000000000000000 RSI: 00000000200001c0 RDI: 0000000000000003
> > > RBP: 00000000004d29e9 R08: 0000000000000000 R09: 0000000000000000
> > > R10: 0000000000000000 R11: 0000000000000246 R12: 000000000077bf80
> > > R13: 0000000000000000 R14: 000000000077bf80 R15: 00007ffdb36bc6c0
> > >  </TASK>
> > > Modules linked in:
> > > CR2: 0000000000000270
> >
> > I *think* that the root cause of the above splat could be
> > different/simpler. In unix_diag_get_exact():
> >
> >
> >         rep = nlmsg_new(sizeof(struct unix_diag_msg) + extra_len, GFP_KERNEL);
> >         if (!rep)
> >                 goto out;
> >
> >         // rep->sk is NULL now.
> >         err = sk_diag_fill(sk, rep, req, NETLINK_CB(in_skb).portid,
> >                            nlh->nlmsg_seq, 0, req->udiag_ino);
> >
> > and sk_diag_fill() will splats deterministically. Note that the user
> > space don't trigger that code path usually, but it can be reproduced
> > with a sligthly modified 'ss' version.

Ah, I misunderstood that the found sk is passed to sk_user_ns(), but it's
skb->sk.

I'll check again.

Thank you, Paolo!


P.S.  I'm leaving for Japan today and will be bit slow this and next week
for vacation.

Re: [PATCH v1 net] af_unix: Call sk_diag_fill() under the bucket lock.

[Paolo Abeni]

On Wed, 2022-11-23 at 07:22 -0800, Kuniyuki Iwashima wrote:
> From:   Wei Chen <harperchen1110@gmail.com>
> Date:   Wed, 23 Nov 2022 23:09:53 +0800
> > Dear Paolo,
> > 
> > Could you explain the meaning of modified "ss" version to reproduce
> > the bug? I'd like to learn how to reproduce the bug in the user space
> > to facilitate the bug fix.
> 
> I think it means to drop NLM_F_DUMP and modify args as needed because
> ss dumps all sockets, not exactly a single socket.

Exactly! Additionally 'ss' must fill udiag_ino and udiag_cookie with
values matching a live unix socket. And before that you have to add
more code to allow 'ss' dumping such values (or fetch them with some
bpf/perf probe).

> 
> Ah, I misunderstood that the found sk is passed to sk_user_ns(), but it's
> skb->sk.

I did not double check the race you outlined in this patch. That could
still possibly be a valid/existing one.

> P.S.  I'm leaving for Japan today and will be bit slow this and next week
> for vacation.

Have a nice trip ;)

/P

Re: [PATCH v1 net] af_unix: Call sk_diag_fill() under the bucket lock.

[Wei Chen]

Dear Linux developers,

My step tracing over Linux found the following C program would trigger
the reported crash. I hope it is helpful for bug fix.

#include <errno.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>
#include <sys/socket.h>
#include <sys/un.h>
#include <linux/netlink.h>
#include <linux/rtnetlink.h>
#include <linux/sock_diag.h>
#include <linux/unix_diag.h>
#include <linux/stat.h>
#include <sys/types.h>
#include <sys/stat.h>

int main(void) {
    int fd1 = socket(AF_UNIX, SOCK_STREAM, 0);
    struct stat file_stat;
    fstat(fd1, &file_stat);
    int fd2 = socket(AF_NETLINK, SOCK_RAW, NETLINK_SOCK_DIAG);

    struct sockaddr_nl nladdr = {
        .nl_family = AF_NETLINK
    };
    struct {
        struct nlmsghdr nlh;
        struct unix_diag_req udr;
    } req = {
        .nlh = {
            .nlmsg_len = sizeof(req),
            .nlmsg_type = SOCK_DIAG_BY_FAMILY,
            .nlmsg_flags = NLM_F_REQUEST
        },
        .udr = {
            .sdiag_family = AF_UNIX,
            .udiag_states = -1,
            .udiag_ino = file_stat.st_ino,
            .udiag_show = 0x40
        }
    };
    struct iovec iov = {
        .iov_base = &req,
        .iov_len = sizeof(req)
    };
    struct msghdr msg = {
        .msg_name = &nladdr,
        .msg_namelen = sizeof(nladdr),
        .msg_iov = &iov,
        .msg_iovlen = 1
    };

    sendmsg(fd2, &msg, 0);
    return 0;
}

Best,
Wei

On Wed, 23 Nov 2022 at 23:38, Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Wed, 2022-11-23 at 07:22 -0800, Kuniyuki Iwashima wrote:
> > From:   Wei Chen <harperchen1110@gmail.com>
> > Date:   Wed, 23 Nov 2022 23:09:53 +0800
> > > Dear Paolo,
> > >
> > > Could you explain the meaning of modified "ss" version to reproduce
> > > the bug? I'd like to learn how to reproduce the bug in the user space
> > > to facilitate the bug fix.
> >
> > I think it means to drop NLM_F_DUMP and modify args as needed because
> > ss dumps all sockets, not exactly a single socket.
>
> Exactly! Additionally 'ss' must fill udiag_ino and udiag_cookie with
> values matching a live unix socket. And before that you have to add
> more code to allow 'ss' dumping such values (or fetch them with some
> bpf/perf probe).
>
> >
> > Ah, I misunderstood that the found sk is passed to sk_user_ns(), but it's
> > skb->sk.
>
> I did not double check the race you outlined in this patch. That could
> still possibly be a valid/existing one.
>
> > P.S.  I'm leaving for Japan today and will be bit slow this and next week
> > for vacation.
>
> Have a nice trip ;)
>
> /P
>

Re: [PATCH v1 net] af_unix: Call sk_diag_fill() under the bucket lock.

[Kuniyuki Iwashima]

From:   Wei Chen <harperchen1110@gmail.com>
Date:   Thu, 24 Nov 2022 17:37:04 +0800
> Dear Linux developers,
> 
> My step tracing over Linux found the following C program would trigger
> the reported crash. I hope it is helpful for bug fix.

Thank you, Wei.

I guess you commented out the sock_diag_check_cookie() validation ?
Otherwise you would have to set req.udr.udiag_cookie.

For the record, my repro:

---8<---
#include <errno.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>
#include <sys/socket.h>
#include <sys/stat.h>
#include <sys/un.h>
#include <linux/netlink.h>
#include <linux/rtnetlink.h>
#include <linux/sock_diag.h>
#include <linux/unix_diag.h>

void main(void)
{
	struct sockaddr_nl nladdr = {
		.nl_family = AF_NETLINK
	};
	struct {
		struct nlmsghdr nlh;
		struct unix_diag_req udr;
	} req = {
		.nlh = {
			.nlmsg_len = sizeof(req),
			.nlmsg_type = SOCK_DIAG_BY_FAMILY,
			.nlmsg_flags = NLM_F_REQUEST
		},
		.udr = {
			.sdiag_family = AF_UNIX,
			.udiag_show = UDIAG_SHOW_UID,
		}
	};
	struct iovec iov = {
		.iov_base = &req,
		.iov_len = sizeof(req)
	};
	struct msghdr msg = {
		.msg_name = &nladdr,
		.msg_namelen = sizeof(nladdr),
		.msg_iov = &iov,
		.msg_iovlen = 1
	};
	int netlink_fd, unix_fd, ret;
	struct stat file_stat;
	socklen_t optlen;
	__u64 cookie;

	unix_fd = socket(AF_UNIX, SOCK_STREAM, 0);
	fstat(unix_fd, &file_stat);
	optlen = sizeof(cookie);
	getsockopt(unix_fd, SOL_SOCKET, SO_COOKIE, &cookie, &optlen);

	req.udr.udiag_ino = file_stat.st_ino;
	req.udr.udiag_cookie[0] = (__u32)cookie;
	req.udr.udiag_cookie[1] = (__u32)(cookie >> 32);

	netlink_fd = socket(AF_NETLINK, SOCK_RAW, NETLINK_SOCK_DIAG);
	sendmsg(netlink_fd, &msg, 0);

	close(netlink_fd);
	close(unix_fd);
}
---8<---


Interestingly, I only see the NULL deref with this patch applied.
Anyway, I'll post a fix later :)

---8<---
diff --git a/net/unix/diag.c b/net/unix/diag.c
index 105f522a89fe..f1c8f565af77 100644
--- a/net/unix/diag.c
+++ b/net/unix/diag.c
@@ -117,6 +117,7 @@ static int sk_diag_show_rqlen(struct sock *sk, struct sk_buff *nlskb)
 static int sk_diag_dump_uid(struct sock *sk, struct sk_buff *nlskb)
 {
 	uid_t uid = from_kuid_munged(sk_user_ns(nlskb->sk), sock_i_uid(sk));
+	printk(KERN_ERR "sk_diag_dump_uid: sk: %px\tskb->sk: %px, %px\n", sk, nlskb->sk, sk_user_ns(nlskb->sk));
 	return nla_put(nlskb, UNIX_DIAG_UID, sizeof(uid_t), &uid);
 }
 
---8<---


> 
> #include <errno.h>
> #include <stdio.h>
> #include <string.h>
> #include <unistd.h>
> #include <sys/socket.h>
> #include <sys/un.h>
> #include <linux/netlink.h>
> #include <linux/rtnetlink.h>
> #include <linux/sock_diag.h>
> #include <linux/unix_diag.h>
> #include <linux/stat.h>
> #include <sys/types.h>
> #include <sys/stat.h>
> 
> int main(void) {
>     int fd1 = socket(AF_UNIX, SOCK_STREAM, 0);
>     struct stat file_stat;
>     fstat(fd1, &file_stat);
>     int fd2 = socket(AF_NETLINK, SOCK_RAW, NETLINK_SOCK_DIAG);
> 
>     struct sockaddr_nl nladdr = {
>         .nl_family = AF_NETLINK
>     };
>     struct {
>         struct nlmsghdr nlh;
>         struct unix_diag_req udr;
>     } req = {
>         .nlh = {
>             .nlmsg_len = sizeof(req),
>             .nlmsg_type = SOCK_DIAG_BY_FAMILY,
>             .nlmsg_flags = NLM_F_REQUEST
>         },
>         .udr = {
>             .sdiag_family = AF_UNIX,
>             .udiag_states = -1,
>             .udiag_ino = file_stat.st_ino,
>             .udiag_show = 0x40
>         }
>     };
>     struct iovec iov = {
>         .iov_base = &req,
>         .iov_len = sizeof(req)
>     };
>     struct msghdr msg = {
>         .msg_name = &nladdr,
>         .msg_namelen = sizeof(nladdr),
>         .msg_iov = &iov,
>         .msg_iovlen = 1
>     };
> 
>     sendmsg(fd2, &msg, 0);
>     return 0;
> }
> 
> Best,
> Wei
> 
> On Wed, 23 Nov 2022 at 23:38, Paolo Abeni <pabeni@redhat.com> wrote:
> >
> > On Wed, 2022-11-23 at 07:22 -0800, Kuniyuki Iwashima wrote:
> > > From:   Wei Chen <harperchen1110@gmail.com>
> > > Date:   Wed, 23 Nov 2022 23:09:53 +0800
> > > > Dear Paolo,
> > > >
> > > > Could you explain the meaning of modified "ss" version to reproduce
> > > > the bug? I'd like to learn how to reproduce the bug in the user space
> > > > to facilitate the bug fix.
> > >
> > > I think it means to drop NLM_F_DUMP and modify args as needed because
> > > ss dumps all sockets, not exactly a single socket.
> >
> > Exactly! Additionally 'ss' must fill udiag_ino and udiag_cookie with
> > values matching a live unix socket. And before that you have to add
> > more code to allow 'ss' dumping such values (or fetch them with some
> > bpf/perf probe).
> >
> > >
> > > Ah, I misunderstood that the found sk is passed to sk_user_ns(), but it's
> > > skb->sk.
> >
> > I did not double check the race you outlined in this patch. That could
> > still possibly be a valid/existing one.
> >
> > > P.S.  I'm leaving for Japan today and will be bit slow this and next week
> > > for vacation.
> >
> > Have a nice trip ;)
> >
> > /P