On Thu, Jan 18, 2024 at 4:52 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
>
> On Thu, Jan 18, 2024 at 3:47 PM Stephen Smalley
> <stephen.smalley.work@xxxxxxxxx> wrote:
> > On Thu, Jan 18, 2024 at 3:19 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > > On Thu, Jan 18, 2024 at 12:02 PM Stephen Smalley
> > > <stephen.smalley.work@xxxxxxxxx> wrote:
> > > >
> > > > On Thu, Jan 18, 2024 at 11:26 AM Gabriel Ryan <gabe@xxxxxxxxxxxxxxx> wrote:
> > > > >
> > > > > We found a race in selinux for kernel v6.6 using a prototype race
> > > > > testing tool based on modified KCSAN we are developing. We are
> > > > > reporting the race because it appears to be a potential bug. The race
> > > > > occurs on isec->sclass and isec->sid, which are set in
> > > > >
> > > > > security/selinux/hooks.c:3329-3330 selinux_inode_post_setxattr
> > > > >
> > > > > isec->sclass = inode_mode_to_security_class(inode->i_mode);
> > > > > isec->sid = newsid;
> > > > >
> > > > > Where isec->lock is held when isec->sclass and isec->sid are set above
> > > > > but not held when they are read in the following 3 locations:
> > > > >
> > > > > security/selinux/hooks.c:1671 inode_has_perm
> > > > > security/selinux/hooks.c:3125 selinux_inode_permission
> > > > > security/selinux/hooks.c:3690 ioctl_has_perm
> > > > >
> > > > >
> > > > > This seems like it could lead to undefined behavior if multiple
> > > > > threads are reading the isec struct and updating it concurrently,
> > > > > (e.g., reading an old isec->sid value but new isec->sclass value).
> > > > >
> > > > > In some other cases in security/selinux/hooks.c, isec->lock is held
> > > > > when isec->sclass and isec->sid are accessed, such as in
> > > > > security/selinux/hooks.c:4942-4945 selinux_socket_accept. Therefore,
> > > > > extending the isec->lock to cover when sclass and sid are read from
> > > > > the isec struct in these three locations might be a suitable fix.
> > > >
> > > > isec->sclass should only really need to be set once when isec is first
> > > > initialized after mode format bits have been set.
> > > > Not sure why it is getting assigned again in post_setxattr.
> > >
> > > There is similar odd behavior in selinux_inode_setsecurity(). Looking
> > > at the other places in hooks.c where we are setting isec->sclass, I'm
> > > wondering if this is a copy-n-paste from one of the other places that
> > > does have a need for it. The pattern of "lock, set the sclass and
> > > SID, mark the inode as initialized, unlock" occurs in at least three
> > > places that I can see in a quick search.
> >
> > git blame indicates that the setting of isec->sclass was added to
> > inode_post_setxattr() and inode_setsecurity() by commit
> > aa9c2669626ca7e5e5ba ("NFS: Client implementation of Labeled-NFS").
> > Not sure why - do NFS inodes get initialized in some manner that skips
> > the usual setting?
>
> I don't know, but I wouldn't be surprised, we've definitely had to do
> some NFS-specific things over the years.
>
> > It's fine to set it in inode_doinit_with_dentry() when initializing
> > the inode security blob for existing inodes and in
> > selinux_inode_init_security() for newly created ones - no other thread
> > can be reading it at that point.
>
> Yes, when I said "odd behavior" I was trying to get at the original
> discussion that it doesn't make much sense there, regardless of the
> safety. I can potentially see a need for it as-is in
> inode_doinit_with_dentry() but I haven't chased down all the call
> chains as that would take a while, and like you say, it should be
> safe.
>
> > selinux_task_to_inode() is a special case for /proc inodes.
>
> Yes, I believe that needs to stay.
>
> > selinux_socket_post_create() and
> > selinux_socket_accept() should set it before the socket can be
> > accessed by userspace or via an incoming packet.
>
> Agreed. (it looks okay there as-is)
Arguably setting it in post_setxattr and _setsecurity is a bug because
it would replace a socket security class with a file security class on
a socket inode if one performed a fsetxattr() on a socket. We don't
really support that anyway since there is no way to propagate it down
to the sock.
Tempting to remove from both and run the NFS tests (tools/nfs.sh) to
check for regressions.
