| Message ID | tencent_0BEE86CD3878D26D402DDD6F949484E96E0A@qq.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: read and write sid under lock | expand |
Hi Edward, kernel test robot noticed the following build errors: [auto build test ERROR on pcmoore-selinux/next] [also build test ERROR on linus/master v6.14-rc5 next-20250307] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Edward-Adam-Davis/selinux-read-and-write-sid-under-lock/20250309-130846 base: https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git next patch link: https://lore.kernel.org/r/tencent_0BEE86CD3878D26D402DDD6F949484E96E0A%40qq.com patch subject: [PATCH] selinux: read and write sid under lock config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20250310/202503100821.PtEmEm7K-lkp@intel.com/config) compiler: gcc-11 (Debian 11.3.0-12) 11.3.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250310/202503100821.PtEmEm7K-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202503100821.PtEmEm7K-lkp@intel.com/ All errors (new ones prefixed by >>): security/selinux/hooks.c: In function 'selinux_socket_post_create': >> security/selinux/hooks.c:4718:33: error: 'struct sk_security_struct' has no member named 'lock' 4718 | spin_lock(&sksec->lock); | ^~ security/selinux/hooks.c:4721:35: error: 'struct sk_security_struct' has no member named 'lock' 4721 | spin_unlock(&sksec->lock); | ^~ security/selinux/hooks.c: In function 'selinux_socket_sock_rcv_skb': security/selinux/hooks.c:5198:25: error: 'struct sk_security_struct' has no member named 'lock' 5198 | spin_lock(&sksec->lock); | ^~ security/selinux/hooks.c:5200:27: error: 'struct sk_security_struct' has no member named 'lock' 5200 | spin_unlock(&sksec->lock); | ^~ vim +4718 security/selinux/hooks.c 4695 4696 static int selinux_socket_post_create(struct socket *sock, int family, 4697 int type, int protocol, int kern) 4698 { 4699 const struct task_security_struct *tsec = selinux_cred(current_cred()); 4700 struct inode_security_struct *isec = inode_security_novalidate(SOCK_INODE(sock)); 4701 struct sk_security_struct *sksec; 4702 u16 sclass = socket_type_to_security_class(family, type, protocol); 4703 u32 sid = SECINITSID_KERNEL; 4704 int err = 0; 4705 4706 if (!kern) { 4707 err = socket_sockcreate_sid(tsec, sclass, &sid); 4708 if (err) 4709 return err; 4710 } 4711 4712 isec->sclass = sclass; 4713 isec->sid = sid; 4714 isec->initialized = LABEL_INITIALIZED; 4715 4716 if (sock->sk) { 4717 sksec = selinux_sock(sock->sk); > 4718 spin_lock(&sksec->lock); 4719 sksec->sclass = sclass; 4720 sksec->sid = sid; 4721 spin_unlock(&sksec->lock); 4722 /* Allows detection of the first association on this socket */ 4723 if (sksec->sclass == SECCLASS_SCTP_SOCKET) 4724 sksec->sctp_assoc_state = SCTP_ASSOC_UNSET; 4725 4726 err = selinux_netlbl_socket_post_create(sock->sk, family); 4727 } 4728 4729 return err; 4730 } 4731
Hi Edward, kernel test robot noticed the following build errors: [auto build test ERROR on pcmoore-selinux/next] [also build test ERROR on linus/master v6.14-rc6 next-20250307] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Edward-Adam-Davis/selinux-read-and-write-sid-under-lock/20250309-130846 base: https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git next patch link: https://lore.kernel.org/r/tencent_0BEE86CD3878D26D402DDD6F949484E96E0A%40qq.com patch subject: [PATCH] selinux: read and write sid under lock config: i386-defconfig (https://download.01.org/0day-ci/archive/20250310/202503101039.wURTMnYj-lkp@intel.com/config) compiler: clang version 19.1.7 (https://github.com/llvm/llvm-project cd708029e0b2869e80abe31ddb175f7c35361f90) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250310/202503101039.wURTMnYj-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202503101039.wURTMnYj-lkp@intel.com/ All errors (new ones prefixed by >>): >> security/selinux/hooks.c:4718:21: error: no member named 'lock' in 'struct sk_security_struct' 4718 | spin_lock(&sksec->lock); | ~~~~~ ^ security/selinux/hooks.c:4721:23: error: no member named 'lock' in 'struct sk_security_struct' 4721 | spin_unlock(&sksec->lock); | ~~~~~ ^ security/selinux/hooks.c:5198:20: error: no member named 'lock' in 'struct sk_security_struct' 5198 | spin_lock(&sksec->lock); | ~~~~~ ^ security/selinux/hooks.c:5200:22: error: no member named 'lock' in 'struct sk_security_struct' 5200 | spin_unlock(&sksec->lock); | ~~~~~ ^ 4 errors generated. vim +4718 security/selinux/hooks.c 4695 4696 static int selinux_socket_post_create(struct socket *sock, int family, 4697 int type, int protocol, int kern) 4698 { 4699 const struct task_security_struct *tsec = selinux_cred(current_cred()); 4700 struct inode_security_struct *isec = inode_security_novalidate(SOCK_INODE(sock)); 4701 struct sk_security_struct *sksec; 4702 u16 sclass = socket_type_to_security_class(family, type, protocol); 4703 u32 sid = SECINITSID_KERNEL; 4704 int err = 0; 4705 4706 if (!kern) { 4707 err = socket_sockcreate_sid(tsec, sclass, &sid); 4708 if (err) 4709 return err; 4710 } 4711 4712 isec->sclass = sclass; 4713 isec->sid = sid; 4714 isec->initialized = LABEL_INITIALIZED; 4715 4716 if (sock->sk) { 4717 sksec = selinux_sock(sock->sk); > 4718 spin_lock(&sksec->lock); 4719 sksec->sclass = sclass; 4720 sksec->sid = sid; 4721 spin_unlock(&sksec->lock); 4722 /* Allows detection of the first association on this socket */ 4723 if (sksec->sclass == SECCLASS_SCTP_SOCKET) 4724 sksec->sctp_assoc_state = SCTP_ASSOC_UNSET; 4725 4726 err = selinux_netlbl_socket_post_create(sock->sk, family); 4727 } 4728 4729 return err; 4730 } 4731
On Sat, Mar 8, 2025 at 11:55 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. > > Add a lock 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> > --- > security/selinux/hooks.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 7b867dfec88b..ea5d0273f9d5 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -4677,8 +4677,10 @@ static int selinux_socket_post_create(struct socket *sock, int family, > > if (sock->sk) { > sksec = selinux_sock(sock->sk); > + spin_lock(&sksec->lock); You didn't include the diff that adds this lock field to sk_security_struct, but aside from that, I would suggest something lighter-weight like READ_ONCE/WRITE_ONCE if possible. > sksec->sclass = sclass; > sksec->sid = sid; > + spin_unlock(&sksec->lock); > /* Allows detection of the first association on this socket */ > if (sksec->sclass == SECCLASS_SCTP_SOCKET) > sksec->sctp_assoc_state = SCTP_ASSOC_UNSET; > @@ -5126,7 +5128,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; > struct common_audit_data ad; > struct lsm_network_audit net; > char *addrp; > @@ -5155,6 +5157,9 @@ static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) > if (err) > return err; > > + spin_lock(&sksec->lock); If you retain this as a spinlock, then I think you need the irq-safe version lock/unlock calls in this hook due to some callers. > + sk_sid = sksec->sid; > + spin_unlock(&sksec->lock); > if (peerlbl_active) { > u32 peer_sid; > > -- > 2.43.0 >
On Mon, Mar 10, 2025 at 3:53 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > On Sat, Mar 8, 2025 at 11:55 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. > > > > Add a lock to synchronize the two. ... > > Reported-by: syzbot+00c633585760c05507c3@syzkaller.appspotmail.com > > Closes: https://syzkaller.appspot.com/bug?extid=00c633585760c05507c3 > > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > > --- > > security/selinux/hooks.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index 7b867dfec88b..ea5d0273f9d5 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -4677,8 +4677,10 @@ static int selinux_socket_post_create(struct socket *sock, int family, > > > > if (sock->sk) { > > sksec = selinux_sock(sock->sk); > > + spin_lock(&sksec->lock); > > You didn't include the diff that adds this lock field to > sk_security_struct, but aside from that, I would suggest something > lighter-weight like READ_ONCE/WRITE_ONCE if possible. Yes, please don't add a spinlock to something that is potentially going to be hit on every packet entering the system.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 7b867dfec88b..ea5d0273f9d5 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -4677,8 +4677,10 @@ static int selinux_socket_post_create(struct socket *sock, int family, if (sock->sk) { sksec = selinux_sock(sock->sk); + spin_lock(&sksec->lock); sksec->sclass = sclass; sksec->sid = sid; + spin_unlock(&sksec->lock); /* Allows detection of the first association on this socket */ if (sksec->sclass == SECCLASS_SCTP_SOCKET) sksec->sctp_assoc_state = SCTP_ASSOC_UNSET; @@ -5126,7 +5128,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; struct common_audit_data ad; struct lsm_network_audit net; char *addrp; @@ -5155,6 +5157,9 @@ static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) if (err) return err; + spin_lock(&sksec->lock); + sk_sid = sksec->sid; + spin_unlock(&sksec->lock); if (peerlbl_active) { u32 peer_sid;
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. Add a lock 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> --- security/selinux/hooks.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)