| Message ID | tencent_5E4FD70DE825E68D96927A5DE95D90991807@qq.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [V2] selinux: access sid under READ/WRITE_ONCE | expand |
On Mon, Mar 10, 2025 at 8:03 PM Edward Adam Davis <eadavis@qq.com> wrote: > > syzbot reported a data-race in selinux_socket_post_create / > selinux_socket_sock_rcv_skb. [1] > > When creating the socket path and receiving the network data packet path, > effective data access protection is not performed when reading and writing > the sid, resulting in a race condition. > > Use READ_ONCE/WRITE_ONCE to synchronize the two. > > [1] > BUG: KCSAN: data-race in selinux_socket_post_create / selinux_socket_sock_rcv_skb > > write to 0xffff88811b989e30 of 4 bytes by task 3803 on cpu 0: > selinux_socket_post_create+0x1b5/0x2a0 security/selinux/hooks.c:4681 > security_socket_post_create+0x5b/0xa0 security/security.c:4577 > __sock_create+0x35b/0x5a0 net/socket.c:1571 > sock_create net/socket.c:1606 [inline] > __sys_socket_create net/socket.c:1643 [inline] > __sys_socket+0xae/0x240 net/socket.c:1690 > __do_sys_socket net/socket.c:1704 [inline] > __se_sys_socket net/socket.c:1702 [inline] > __x64_sys_socket+0x3f/0x50 net/socket.c:1702 > x64_sys_call+0x2cf2/0x2dc0 arch/x86/include/generated/asm/syscalls_64.h:42 > do_syscall_x64 arch/x86/entry/common.c:52 [inline] > do_syscall_64+0xc9/0x1c0 arch/x86/entry/common.c:83 > entry_SYSCALL_64_after_hwframe+0x77/0x7f > > read to 0xffff88811b989e30 of 4 bytes by task 3805 on cpu 1: > selinux_socket_sock_rcv_skb+0x72/0x6a0 security/selinux/hooks.c:5129 > security_sock_rcv_skb+0x3d/0x80 security/security.c:4781 > sk_filter_trim_cap+0xca/0x3c0 net/core/filter.c:151 > sk_filter include/linux/filter.h:1062 [inline] > sock_queue_rcv_skb_reason+0x28/0xc0 net/core/sock.c:527 > sock_queue_rcv_skb include/net/sock.h:2403 [inline] > packet_rcv_spkt+0x2f7/0x3b0 net/packet/af_packet.c:1967 > deliver_skb net/core/dev.c:2449 [inline] > __netif_receive_skb_core+0x48f/0x2350 net/core/dev.c:5737 > __netif_receive_skb_list_core+0x115/0x520 net/core/dev.c:5968 > __netif_receive_skb_list net/core/dev.c:6035 [inline] > netif_receive_skb_list_internal+0x4e4/0x660 net/core/dev.c:6126 > netif_receive_skb_list+0x31/0x230 net/core/dev.c:6178 > xdp_recv_frames net/bpf/test_run.c:280 [inline] > xdp_test_run_batch net/bpf/test_run.c:361 [inline] > bpf_test_run_xdp_live+0xe10/0x1040 net/bpf/test_run.c:390 > bpf_prog_test_run_xdp+0x51d/0x8b0 net/bpf/test_run.c:1316 > bpf_prog_test_run+0x20f/0x3a0 kernel/bpf/syscall.c:4407 > __sys_bpf+0x400/0x7a0 kernel/bpf/syscall.c:5813 > __do_sys_bpf kernel/bpf/syscall.c:5902 [inline] > __se_sys_bpf kernel/bpf/syscall.c:5900 [inline] > __x64_sys_bpf+0x43/0x50 kernel/bpf/syscall.c:5900 > x64_sys_call+0x2914/0x2dc0 arch/x86/include/generated/asm/syscalls_64.h:322 > do_syscall_x64 arch/x86/entry/common.c:52 [inline] > do_syscall_64+0xc9/0x1c0 arch/x86/entry/common.c:83 > entry_SYSCALL_64_after_hwframe+0x77/0x7f > > value changed: 0x00000003 -> 0x00000087 > > Reported-by: syzbot+00c633585760c05507c3@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=00c633585760c05507c3 > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > --- > V1 -> V2: replace with READ_ONCE/WRITE_ONCE Thanks for the patch. Did you audit all other users of sksec->sid to see if they too should be wrapped with READ_ONCE()/WRITE_ONCE() or are already safe? > > security/selinux/hooks.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 7b867dfec88b..77d2953eaa4d 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -4678,7 +4678,7 @@ static int selinux_socket_post_create(struct socket *sock, int family, > if (sock->sk) { > sksec = selinux_sock(sock->sk); > sksec->sclass = sclass; > - sksec->sid = sid; > + WRITE_ONCE(sksec->sid, sid); > /* Allows detection of the first association on this socket */ > if (sksec->sclass == SECCLASS_SCTP_SOCKET) > sksec->sctp_assoc_state = SCTP_ASSOC_UNSET; > @@ -5126,7 +5126,7 @@ static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) > int err, peerlbl_active, secmark_active; > struct sk_security_struct *sksec = selinux_sock(sk); > u16 family = sk->sk_family; > - u32 sk_sid = sksec->sid; > + u32 sk_sid = READ_ONCE(sksec->sid); > struct common_audit_data ad; > struct lsm_network_audit net; > char *addrp; > -- > 2.43.0 >
On Tue, 11 Mar 2025 11:19:49 -0400, Stephen Smalley wrote: > > Reported-by: syzbot+00c633585760c05507c3@syzkaller.appspotmail.com > > Closes: https://syzkaller.appspot.com/bug?extid=00c633585760c05507c3 > > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > > --- > > V1 -> V2: replace with READ_ONCE/WRITE_ONCE > > Thanks for the patch. Did you audit all other users of sksec->sid to > see if they too should be wrapped with READ_ONCE()/WRITE_ONCE() or are > already safe? This fix is only for the issues reported by syzbot+00c633585760c05507c3. I have confirmed that there are many contexts related to "sksec->sid" (at least 29). I am not familiar with selinux, and it is unnecessary to do excessive fixes before syzbot reports other issues. Maybe the subject of my patch is not appropriate, and it may be more appropriate to adjust it to "selinux: fix data race in socket create and receive skb". BR, Edward
On Tue, Mar 11, 2025 at 9:05 PM Edward Adam Davis <eadavis@qq.com> wrote: > > On Tue, 11 Mar 2025 11:19:49 -0400, Stephen Smalley wrote: > > > Reported-by: syzbot+00c633585760c05507c3@syzkaller.appspotmail.com > > > Closes: https://syzkaller.appspot.com/bug?extid=00c633585760c05507c3 > > > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > > > --- > > > V1 -> V2: replace with READ_ONCE/WRITE_ONCE > > > > Thanks for the patch. Did you audit all other users of sksec->sid to > > see if they too should be wrapped with READ_ONCE()/WRITE_ONCE() or are > > already safe? > This fix is only for the issues reported by syzbot+00c633585760c05507c3. > I have confirmed that there are many contexts related to "sksec->sid" (at > least 29). I am not familiar with selinux, and it is unnecessary to do > excessive fixes before syzbot reports other issues. > > Maybe the subject of my patch is not appropriate, and it may be more > appropriate to adjust it to "selinux: fix data race in socket create and > receive skb". We don't want to just silence warnings but rather identify and fix root causes, and do so comprehensively. Looking more closely at the syzbot report, it appears that a sock that initially has SID 3 (aka SECINITSID_UNLABELED) is being assigned a specific SID via socket_post_create at a point where it might already be receiving packets. That seems like it requires a more general fix to ensure that the sock is correctly labeled before it can start receiving packets.
On Wed, Mar 12, 2025 at 9:23 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Tue, Mar 11, 2025 at 9:05 PM Edward Adam Davis <eadavis@qq.com> wrote: > > > > On Tue, 11 Mar 2025 11:19:49 -0400, Stephen Smalley wrote: > > > > Reported-by: syzbot+00c633585760c05507c3@syzkaller.appspotmail.com > > > > Closes: https://syzkaller.appspot.com/bug?extid=00c633585760c05507c3 > > > > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > > > > --- > > > > V1 -> V2: replace with READ_ONCE/WRITE_ONCE > > > > > > Thanks for the patch. Did you audit all other users of sksec->sid to > > > see if they too should be wrapped with READ_ONCE()/WRITE_ONCE() or are > > > already safe? > > This fix is only for the issues reported by syzbot+00c633585760c05507c3. > > I have confirmed that there are many contexts related to "sksec->sid" (at > > least 29). I am not familiar with selinux, and it is unnecessary to do > > excessive fixes before syzbot reports other issues. > > > > Maybe the subject of my patch is not appropriate, and it may be more > > appropriate to adjust it to "selinux: fix data race in socket create and > > receive skb". > > We don't want to just silence warnings but rather identify and fix > root causes, and do so comprehensively. > Looking more closely at the syzbot report, it appears that a sock that > initially has SID 3 (aka SECINITSID_UNLABELED) is being assigned a > specific SID via socket_post_create at a point where it might already > be receiving packets. > That seems like it requires a more general fix to ensure that the sock > is correctly labeled before it can start receiving packets. There is a window in __sock_create() where the socket is created and "alive" in the network stack, but before security_socket_post_create() is called to fully initialize/label the socket (look between pf->create() and the LSM call in __sock_create()). Looking quickly, I *think* it may be as simple as doing the read/write_once() accessors for the sksec->sid, but I didn't dig into the NetLabel and XFRM aspects of the code paths. I suspect they are okay due to how they work, but I'm not going to be surprised if there is an issue lurking there. We could possibly solve this generically by introducing a sksec->initialized field, similar to inodes, although we would have no way to properly initialize the sksec in selinux_socket_sock_rcv_skb() if we hit the uninitialized case so we would need to decide how to handle that. I worry that dropping packets in that case could negatively impact stream connections that need to go through a multi-step handshake process. Maybe we could capture the creator's label in selinux_sk_alloc_security(), at least in some cases (?), but this would need a lot of investigation to see if that works properly in all the cases (I worry it doesn't).
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 7b867dfec88b..77d2953eaa4d 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -4678,7 +4678,7 @@ static int selinux_socket_post_create(struct socket *sock, int family, if (sock->sk) { sksec = selinux_sock(sock->sk); sksec->sclass = sclass; - sksec->sid = sid; + WRITE_ONCE(sksec->sid, sid); /* Allows detection of the first association on this socket */ if (sksec->sclass == SECCLASS_SCTP_SOCKET) sksec->sctp_assoc_state = SCTP_ASSOC_UNSET; @@ -5126,7 +5126,7 @@ static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) int err, peerlbl_active, secmark_active; struct sk_security_struct *sksec = selinux_sock(sk); u16 family = sk->sk_family; - u32 sk_sid = sksec->sid; + u32 sk_sid = READ_ONCE(sksec->sid); struct common_audit_data ad; struct lsm_network_audit net; char *addrp;
syzbot reported a data-race in selinux_socket_post_create / selinux_socket_sock_rcv_skb. [1] When creating the socket path and receiving the network data packet path, effective data access protection is not performed when reading and writing the sid, resulting in a race condition. Use READ_ONCE/WRITE_ONCE to synchronize the two. [1] BUG: KCSAN: data-race in selinux_socket_post_create / selinux_socket_sock_rcv_skb write to 0xffff88811b989e30 of 4 bytes by task 3803 on cpu 0: selinux_socket_post_create+0x1b5/0x2a0 security/selinux/hooks.c:4681 security_socket_post_create+0x5b/0xa0 security/security.c:4577 __sock_create+0x35b/0x5a0 net/socket.c:1571 sock_create net/socket.c:1606 [inline] __sys_socket_create net/socket.c:1643 [inline] __sys_socket+0xae/0x240 net/socket.c:1690 __do_sys_socket net/socket.c:1704 [inline] __se_sys_socket net/socket.c:1702 [inline] __x64_sys_socket+0x3f/0x50 net/socket.c:1702 x64_sys_call+0x2cf2/0x2dc0 arch/x86/include/generated/asm/syscalls_64.h:42 do_syscall_x64 arch/x86/entry/common.c:52 [inline] do_syscall_64+0xc9/0x1c0 arch/x86/entry/common.c:83 entry_SYSCALL_64_after_hwframe+0x77/0x7f read to 0xffff88811b989e30 of 4 bytes by task 3805 on cpu 1: selinux_socket_sock_rcv_skb+0x72/0x6a0 security/selinux/hooks.c:5129 security_sock_rcv_skb+0x3d/0x80 security/security.c:4781 sk_filter_trim_cap+0xca/0x3c0 net/core/filter.c:151 sk_filter include/linux/filter.h:1062 [inline] sock_queue_rcv_skb_reason+0x28/0xc0 net/core/sock.c:527 sock_queue_rcv_skb include/net/sock.h:2403 [inline] packet_rcv_spkt+0x2f7/0x3b0 net/packet/af_packet.c:1967 deliver_skb net/core/dev.c:2449 [inline] __netif_receive_skb_core+0x48f/0x2350 net/core/dev.c:5737 __netif_receive_skb_list_core+0x115/0x520 net/core/dev.c:5968 __netif_receive_skb_list net/core/dev.c:6035 [inline] netif_receive_skb_list_internal+0x4e4/0x660 net/core/dev.c:6126 netif_receive_skb_list+0x31/0x230 net/core/dev.c:6178 xdp_recv_frames net/bpf/test_run.c:280 [inline] xdp_test_run_batch net/bpf/test_run.c:361 [inline] bpf_test_run_xdp_live+0xe10/0x1040 net/bpf/test_run.c:390 bpf_prog_test_run_xdp+0x51d/0x8b0 net/bpf/test_run.c:1316 bpf_prog_test_run+0x20f/0x3a0 kernel/bpf/syscall.c:4407 __sys_bpf+0x400/0x7a0 kernel/bpf/syscall.c:5813 __do_sys_bpf kernel/bpf/syscall.c:5902 [inline] __se_sys_bpf kernel/bpf/syscall.c:5900 [inline] __x64_sys_bpf+0x43/0x50 kernel/bpf/syscall.c:5900 x64_sys_call+0x2914/0x2dc0 arch/x86/include/generated/asm/syscalls_64.h:322 do_syscall_x64 arch/x86/entry/common.c:52 [inline] do_syscall_64+0xc9/0x1c0 arch/x86/entry/common.c:83 entry_SYSCALL_64_after_hwframe+0x77/0x7f value changed: 0x00000003 -> 0x00000087 Reported-by: syzbot+00c633585760c05507c3@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=00c633585760c05507c3 Signed-off-by: Edward Adam Davis <eadavis@qq.com> --- V1 -> V2: replace with READ_ONCE/WRITE_ONCE security/selinux/hooks.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)