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. In general, handling of isec->sid needs an overall cleanup but putting that within isec->lock would be a big hammer. Proper use of READ_ONCE/WRITE_ONCE or smp_load_acquire/store_release may suffice.
