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)
--
paul-moore.com
