On Mon, Nov 30, 2020 at 05:55:20PM +0000, Gong, Sishuai wrote:
> 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.
What do you mean by "data race" in this context? If you have one
process setting a file's group id, while another process is trying to
open that same file and that open is depending on the process's group
access vs the file's group id, the open might succeed or fail
depending on whether the chgrp(2) is executed before or after the
open(2).
I could see data race if in some other context of the file open, the
group id is read, and so the group id might be inconsistent during the
course of operating on a particular system call. In which case, we
might need to cache the group id value, or take some kind of lock,
etc.
But if your automated tool can't distinguish whether or not this is
the case, I'll gently suggest that it's not particuarly useful....
Maybe this is something that should be the subject of further
research? The whole point of automated tools, after all, is to save
developers' time. And not asking them to chase down potential
questions like "so is this real or not"?
Cheers,
- Ted
>
> ------------------------------------------
> 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
>
