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?
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. selinux_task_to_inode() is a special
case for /proc inodes. selinux_socket_post_create() and
selinux_socket_accept() should set it before the socket can be
accessed by userspace or via an incoming packet.
>
> > In general, handling of isec->sid needs an overall cleanup but putting
> > that within isec->lock would be a big hammer.
>
> Yes, that would be an absolute last resort.
>
> I agree that our approach towards inode_security_struct in general
> could use some attention, I would encourage anyone who is interested
> in cleaning things up to do so.
>
> > Proper use of READ_ONCE/WRITE_ONCE or smp_load_acquire/store_release
> > may suffice.
>
> My guess is that READ_ONCE()/WRITE_ONCE() should be sufficient as I
> don't believe we are overly concerned about strict synchronization
> between cpus, but I will admit that I'm far from an expert on memory
> ordering issues. If someone believes another approach is needed
> please do speak up.
>
> --
> paul-moore.com
