Race: data race between ext4_setattr() and acl_permission_check()

["Gong, Sishuai" <sishuai@xxxxxxxxxx>]

Hi,

We found a data race in linux kernel 5.3.11 that we are able to reproduce in x86 under specific interleavings. Currently, we are not sure about the consequence of this race so we would like to confirm with the community if this can be a harmful bug.

------------------------------------------
Writer site

 /tmp/tmp.B7zb7od2zE-5.3.11/extract/linux-5.3.11/fs/ext4/inode.c:5599
       5579              error = PTR_ERR(handle);
       5580              goto err_out;
       5581          }
       5582
       5583          /* dquot_transfer() calls back ext4_get_inode_usage() which
       5584           * counts xattr inode references.
       5585           */
       5586          down_read(&EXT4_I(inode)->xattr_sem);
       5587          error = dquot_transfer(inode, attr);
       5588          up_read(&EXT4_I(inode)->xattr_sem);
       5589
       5590          if (error) {
       5591              ext4_journal_stop(handle);
       5592              return error;
       5593          }
       5594          /* Update corresponding info in inode so that everything is in
       5595           * one transaction */
       5596          if (attr->ia_valid & ATTR_UID)
       5597              inode->i_uid = attr->ia_uid;
       5598          if (attr->ia_valid & ATTR_GID)
 ==>   5599              inode->i_gid = attr->ia_gid;
       5600          error = ext4_mark_inode_dirty(handle, inode);
       5601          ext4_journal_stop(handle);
       5602      }
       5603
       5604      if (attr->ia_valid & ATTR_SIZE) {
       5605          handle_t *handle;
       5606          loff_t oldsize = inode->i_size;
       5607          int shrink = (attr->ia_size < inode->i_size);
       5608
       5609          if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) {
       5610              struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
       5611
       5612              if (attr->ia_size > sbi->s_bitmap_maxbytes)
       5613                  return -EFBIG;
       5614          }
       5615          if (!S_ISREG(inode->i_mode))
       5616              return -EINVAL;
       5617
       5618          if (IS_I_VERSION(inode) && attr->ia_size != inode->i_size)
       5619              inode_inc_iversion(inode);

------------------------------------------
Reader site

/tmp/tmp.B7zb7od2zE-5.3.11/extract/linux-5.3.11/fs/namei.c:306
        286
        287      return -EAGAIN;
        288  }
        289
        290  /*
        291   * This does the basic permission checking
        292   */
        293  static int acl_permission_check(struct inode *inode, int mask)
        294  {
        295      unsigned int mode = inode->i_mode;
        296
        297      if (likely(uid_eq(current_fsuid(), inode->i_uid)))
        298          mode >>= 6;
        299      else {
        300          if (IS_POSIXACL(inode) && (mode & S_IRWXG)) {
        301              int error = check_acl(inode, mask);
        302              if (error != -EAGAIN)
        303                  return error;
        304          }
        305
 ==>    306          if (in_group_p(inode->i_gid))
        307              mode >>= 3;
        308      }
        309
        310      /*
        311       * If the DACs are ok we don't need any capability check.
        312       */
        313      if ((mask & ~mode & (MAY_READ | MAY_WRITE | MAY_EXEC)) == 0)
        314          return 0;
        315      return -EACCES;
        316  }
        317
------------------------------------------
Writer calling trace

- do_fchownat
-- chown_common
--- notify_change

------------------------------------------
Reader calling trace

- do_execve
-- __do_execve_file
--- do_open_execat
---- do_filp_open
----- path_openat
------ link_path_walk
------- inode_permission
-------- generic_permission



Thanks,
Sishuai