Race in fs/posix_acl.c posix_acl_update_mode and related checks on inode->i_mode in kernel v6.6

Race in fs/posix_acl.c posix_acl_update_mode and related checks on inode->i_mode in kernel v6.6

[Gabriel Ryan]

We found races in the fs subsystem in kernel v6.6 using a race testing
tool we are developing based on modified KCSAN. We are reporting the
races because they appear to be a potential bug. The races occur on
inode->i_mode, which is updated in

fs/posix_acl.c:722 posix_acl_update_mode

and can race with reads in the following locations:

security/selinux/hooks.c:3087 selinux_inode_permission
include/linux/fsnotify.h:65 fsnotify_parent
include/linux/device_cgroup.h:24 devcgroup_inode_permission
fs/open.c:923,931 do_dentry_open
fs/namei.c:342 acl_permission_check
fs/namei.c:3242 may_open


In cases where multiple threads are updating and accessing a single
inode simultaneously, it seems like this could potentially lead to
undefined behavior, if for example an access check is passed based on
one i_mode setting, and then the inode->imode is modified by another
thread.

Best,
Gabe

Re: Race in fs/posix_acl.c posix_acl_update_mode and related checks on inode->i_mode in kernel v6.6

[Christian Brauner]

On Thu, Jan 18, 2024 at 11:30:29AM -0500, Gabriel Ryan wrote:
> We found races in the fs subsystem in kernel v6.6 using a race testing
> tool we are developing based on modified KCSAN. We are reporting the
> races because they appear to be a potential bug. The races occur on
> inode->i_mode, which is updated in
> 
> fs/posix_acl.c:722 posix_acl_update_mode
> 
> and can race with reads in the following locations:
> 
> security/selinux/hooks.c:3087 selinux_inode_permission
> include/linux/fsnotify.h:65 fsnotify_parent
> include/linux/device_cgroup.h:24 devcgroup_inode_permission
> fs/open.c:923,931 do_dentry_open
> fs/namei.c:342 acl_permission_check
> fs/namei.c:3242 may_open
> 
> 
> In cases where multiple threads are updating and accessing a single
> inode simultaneously, it seems like this could potentially lead to
> undefined behavior, if for example an access check is passed based on
> one i_mode setting, and then the inode->imode is modified by another
> thread.

Uhm, you need to provide more details than that. This is too vague to be
meaningful especially without any traces. IIRC, posix_acl_update_mode()
is called:

* when a new inode is created before it has been added to the
  d+i-cache. Unless of course, there is a filesystem that updates the
  mode of the inode _after_ the inode has been added to the d+icache.

* The other codepath would be setting an ACL directly via ->set_acl()
  through one of the setxattr() system calls. But even in that case the
  race would be semantically benign. IOW, if we passed the permission
  check on inode->i_mode and it's changed afterwads we don't care. We
  accept such races.

  So the issue you're referencing is what exactly? That the update to
  inode->i_mode can be done through a pointer when passing
  &inode->i_mode directly to posix_acl_update_mode() in which case you
  worry about torn reads/writes? So that would be fixed with a
  WRITE_ONCE() in posix_acl_update_mode()?

Re: Race in fs/posix_acl.c posix_acl_update_mode and related checks on inode->i_mode in kernel v6.6

[Christian Brauner]

On Fri, Jan 19, 2024 at 10:24:59AM -0500, Gabriel Ryan wrote:
> Thank you for your response Christian. I reported the race because it
> appeared potentially harmful, but based on your response it sounds like the
> race is almost certainly benign, and any potential issues would be
> addressed with WRITE_ONCE() as you suggest.

Ok. What I'm asking is whether you can confirm that the thing that your
tool discovered is indeed a WRITE_ONCE() issue. Is there any trace and
any more details?