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