[syzbot] [selinux?] KCSAN: data-race in selinux_socket_post_create / selinux_socket_sock_rcv_skb

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 0f52fd4f67c6 Merge tag 'bcachefs-2025-03-06' of git://evil..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=11a5ca64580000
kernel config: https://syzkaller.appspot.com/x/.config?x=523b0e2f15224775
dashboard link: https://syzkaller.appspot.com/bug?extid=00c633585760c05507c3
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

Unfortunately, I don't have any reproducer for this issue yet.

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/eb0d7b540c67/disk-0f52fd4f.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/51c261332ad9/vmlinux-0f52fd4f.xz
kernel image: https://storage.googleapis.com/syzbot-assets/38914a4790c8/bzImage-0f52fd4f.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+00c633...@syzkaller.appspotmail.com

==================================================================
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 Kernel Concurrency Sanitizer on:
CPU: 1 UID: 0 PID: 3805 Comm: syz.4.118 Not tainted 6.14.0-rc5-syzkaller-00109-g0f52fd4f67c6 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/12/2025
==================================================================
syz.4.118 (3805) used greatest stack depth: 10328 bytes left


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

[Edward Adam Davis]

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]

Reported-by: syzbot+00c633...@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=00c633585760c05507c3
Signed-off-by: Edward Adam Davis <ead...@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);
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;

--
2.43.0

[kernel test robot]

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...@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...@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 <l...@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503100821...@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

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[kernel test robot]

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...@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...@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 <l...@intel.com>

| Closes: https://lore.kernel.org/oe-kbuild-all/202503101039...@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.

[Stephen Smalley]

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.

[Paul Moore]

On Mon, Mar 10, 2025 at 3:53 PM Stephen Smalley
<stephen.sm...@gmail.com> wrote:
> On Sat, Mar 8, 2025 at 11:55 PM Edward Adam Davis <ead...@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+00c633...@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=00c633585760c05507c3
> > Signed-off-by: Edward Adam Davis <ead...@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.

--
paul-moore.com

[Edward Adam Davis]

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.

Reported-by: syzbot+00c633...@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=00c633585760c05507c3
Signed-off-by: Edward Adam Davis <ead...@qq.com>
---

V1 -> V2: replace with READ_ONCE/WRITE_ONCE

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

[Stephen Smalley]

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?

[Edward Adam Davis]

On Tue, 11 Mar 2025 11:19:49 -0400, Stephen Smalley wrote:
> > Reported-by: syzbot+00c633...@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=00c633585760c05507c3
> > Signed-off-by: Edward Adam Davis <ead...@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

[Stephen Smalley]

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.

[Paul Moore]

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

--
paul-moore.com

[syzbot]

Auto-closing this bug as obsolete.
Crashes did not happen for a while, no reproducer and no activity.