On Wed, Mar 12, 2025 at 9:23 AM Stephen Smalley <stephen.smalley.work@xxxxxxxxx> wrote: > > On Tue, Mar 11, 2025 at 9:05 PM Edward Adam Davis <eadavis@xxxxxx> wrote: > > > > On Tue, 11 Mar 2025 11:19:49 -0400, Stephen Smalley wrote: > > > > Reported-by: syzbot+00c633585760c05507c3@xxxxxxxxxxxxxxxxxxxxxxxxx > > > > Closes: https://syzkaller.appspot.com/bug?extid=00c633585760c05507c3 > > > > Signed-off-by: Edward Adam Davis <eadavis@xxxxxx> > > > > --- > > > > 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). -- paul-moore.com
