From 082eabab5a202211764398cbfde4c5fd680cd4e2 Mon Sep 17 00:00:00 2001 From: Jaegeuk Kim Date: Thu, 1 Oct 2020 00:53:59 -0700 Subject: [PATCH] sdcardfs: fix stale permission error This patch fixes a bug showing EACCES of open() which is caused by missing inode and dentry object. Bug: 142101178 Signed-off-by: Jaegeuk Kim Change-Id: I4cab9a3554fad9e2c58a0840fc968257b796e156 --- fs/sdcardfs/derived_perm.c | 39 ++++++++++++++++---------------------- fs/sdcardfs/inode.c | 23 ++++++++++++++++------ fs/sdcardfs/lookup.c | 38 +++++++++++++++++++++++-------------- fs/sdcardfs/sdcardfs.h | 14 +++++++++----- fs/sdcardfs/super.c | 1 + 5 files changed, 67 insertions(+), 48 deletions(-) diff --git a/fs/sdcardfs/derived_perm.c b/fs/sdcardfs/derived_perm.c index 78a669c8a4d6..e6e59e36f8d9 100644 --- a/fs/sdcardfs/derived_perm.c +++ b/fs/sdcardfs/derived_perm.c @@ -51,11 +51,11 @@ void setup_derived_state(struct inode *inode, perm_t perm, userid_t userid, /* While renaming, there is a point where we want the path from dentry, * but the name from newdentry */ -void get_derived_permission_new(struct dentry *parent, struct dentry *dentry, +void get_derived_permission_new(struct inode *parent, struct inode *inode, const struct qstr *name) { - struct sdcardfs_inode_info *info = SDCARDFS_I(d_inode(dentry)); - struct sdcardfs_inode_info *parent_info = SDCARDFS_I(d_inode(parent)); + struct sdcardfs_inode_info *info = SDCARDFS_I(inode); + struct sdcardfs_inode_info *parent_info = SDCARDFS_I(parent); struct sdcardfs_inode_data *parent_data = parent_info->data; appid_t appid; unsigned long user_num; @@ -75,10 +75,10 @@ void get_derived_permission_new(struct dentry *parent, struct dentry *dentry, * of using the inode permissions. */ - inherit_derived_state(d_inode(parent), d_inode(dentry)); + inherit_derived_state(parent, inode); /* Files don't get special labels */ - if (!S_ISDIR(d_inode(dentry)->i_mode)) { + if (!S_ISDIR(inode->i_mode)) { set_top(info, parent_info); return; } @@ -145,9 +145,9 @@ void get_derived_permission_new(struct dentry *parent, struct dentry *dentry, } } -void get_derived_permission(struct dentry *parent, struct dentry *dentry) +void get_derived_permission(struct inode *parent, struct dentry *dentry) { - get_derived_permission_new(parent, dentry, &dentry->d_name); + get_derived_permission_new(parent, d_inode(dentry), &dentry->d_name); } static appid_t get_type(const char *name) @@ -306,9 +306,11 @@ static void __fixup_perms_recursive(struct dentry *dentry, struct limit_search * if (needs_fixup(info->data->perm)) { list_for_each_entry(child, &dentry->d_subdirs, d_child) { spin_lock_nested(&child->d_lock, depth + 1); - if (!(limit->flags & BY_NAME) || qstr_case_eq(&child->d_name, &limit->name)) { + if (!(limit->flags & BY_NAME) || + qstr_case_eq(&child->d_name, &limit->name)) { if (d_inode(child)) { - get_derived_permission(dentry, child); + get_derived_permission(d_inode(dentry), + child); fixup_tmp_permissions(d_inode(child)); spin_unlock(&child->d_lock); break; @@ -330,25 +332,16 @@ void fixup_perms_recursive(struct dentry *dentry, struct limit_search *limit) } /* main function for updating derived permission */ -inline void update_derived_permission_lock(struct dentry *dentry) +inline void update_derived_permission_lock(struct inode *dir, + struct inode *inode, + struct dentry *dentry) { - struct dentry *parent; - - if (!dentry || !d_inode(dentry)) { - pr_err("sdcardfs: %s: invalid dentry\n", __func__); - return; - } /* FIXME: * 1. need to check whether the dentry is updated or not * 2. remove the root dentry update */ - if (!IS_ROOT(dentry)) { - parent = dget_parent(dentry); - if (parent) { - get_derived_permission(parent, dentry); - dput(parent); - } - } + if (!IS_ROOT(dentry)) + get_derived_permission(dir, dentry); fixup_tmp_permissions(d_inode(dentry)); } diff --git a/fs/sdcardfs/inode.c b/fs/sdcardfs/inode.c index 7c08ffe4158a..9fb18e4e58a1 100644 --- a/fs/sdcardfs/inode.c +++ b/fs/sdcardfs/inode.c @@ -105,7 +105,7 @@ static int sdcardfs_create(struct inode *dir, struct dentry *dentry, if (err) goto out; - err = sdcardfs_interpose(dentry, dir->i_sb, &lower_path, + err = sdcardfs_interpose(dir, dentry, dir->i_sb, &lower_path, SDCARDFS_I(dir)->data->userid); if (err) goto out; @@ -287,7 +287,8 @@ static int sdcardfs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode make_nomedia_in_obb = 1; } - err = sdcardfs_interpose(dentry, dir->i_sb, &lower_path, pd->userid); + err = sdcardfs_interpose(dir, dentry, dir->i_sb, + &lower_path, pd->userid); if (err) { unlock_dir(lower_parent_dentry); goto out; @@ -455,7 +456,9 @@ static int sdcardfs_rename(struct inode *old_dir, struct dentry *old_dentry, sdcardfs_copy_and_fix_attrs(old_dir, d_inode(lower_old_dir_dentry)); fsstack_copy_inode_size(old_dir, d_inode(lower_old_dir_dentry)); } - get_derived_permission_new(new_dentry->d_parent, old_dentry, &new_dentry->d_name); + get_derived_permission_new(d_inode(new_dentry->d_parent), + d_inode(old_dentry), + &new_dentry->d_name); fixup_tmp_permissions(d_inode(old_dentry)); fixup_lower_ownership(old_dentry, new_dentry->d_name.name); d_invalidate(old_dentry); /* Can't fixup ownership recursively :( */ @@ -556,12 +559,10 @@ static int sdcardfs_permission(struct vfsmount *mnt, struct inode *inode, int ma { int err; struct inode tmp; - struct sdcardfs_inode_data *top = top_data_get(SDCARDFS_I(inode)); + struct sdcardfs_inode_data *top; if (IS_ERR(mnt)) return PTR_ERR(mnt); - if (!top) - return -EINVAL; /* * Permission check on sdcardfs inode. @@ -575,11 +576,21 @@ static int sdcardfs_permission(struct vfsmount *mnt, struct inode *inode, int ma * locks must be dealt with to avoid undefined behavior. */ copy_attrs(&tmp, inode); + + mutex_lock(&SDCARDFS_I(inode)->top_mutex); + top = top_data_get(SDCARDFS_I(inode)); + if (!top) { + mutex_unlock(&SDCARDFS_I(inode)->top_mutex); + return -EINVAL; + } + tmp.i_uid = make_kuid(&init_user_ns, top->d_uid); tmp.i_gid = make_kgid(&init_user_ns, get_gid(mnt, inode->i_sb, top)); tmp.i_mode = (inode->i_mode & S_IFMT) | get_mode(mnt, SDCARDFS_I(inode), top); data_put(top); + mutex_unlock(&SDCARDFS_I(inode)->top_mutex); + tmp.i_sb = inode->i_sb; if (IS_POSIXACL(inode)) pr_warn("%s: This may be undefined behavior...\n", __func__); diff --git a/fs/sdcardfs/lookup.c b/fs/sdcardfs/lookup.c index c55dc9f5cd52..8178d24828aa 100644 --- a/fs/sdcardfs/lookup.c +++ b/fs/sdcardfs/lookup.c @@ -165,10 +165,11 @@ struct inode *sdcardfs_iget(struct super_block *sb, struct inode *lower_inode, u * Helper interpose routine, called directly by ->lookup to handle * spliced dentries. */ -static struct dentry *__sdcardfs_interpose(struct dentry *dentry, - struct super_block *sb, - struct path *lower_path, - userid_t id) +static struct dentry *__sdcardfs_interpose(struct inode *dir, + struct dentry *dentry, + struct super_block *sb, + struct path *lower_path, + userid_t id) { struct inode *inode; struct inode *lower_inode; @@ -196,10 +197,15 @@ static struct dentry *__sdcardfs_interpose(struct dentry *dentry, goto out; } + mutex_lock(&SDCARDFS_I(inode)->top_mutex); + ret_dentry = d_splice_alias(inode, dentry); dentry = ret_dentry ?: dentry; + if (!IS_ERR(dentry)) - update_derived_permission_lock(dentry); + update_derived_permission_lock(dir, inode, dentry); + + mutex_unlock(&SDCARDFS_I(inode)->top_mutex); out: return ret_dentry; } @@ -212,12 +218,13 @@ out: * @sb: sdcardfs's super_block * @lower_path: the lower path (caller does path_get/put) */ -int sdcardfs_interpose(struct dentry *dentry, struct super_block *sb, - struct path *lower_path, userid_t id) +int sdcardfs_interpose(struct inode *dir, struct dentry *dentry, + struct super_block *sb, + struct path *lower_path, userid_t id) { struct dentry *ret_dentry; - ret_dentry = __sdcardfs_interpose(dentry, sb, lower_path, id); + ret_dentry = __sdcardfs_interpose(dir, dentry, sb, lower_path, id); return PTR_ERR(ret_dentry); } @@ -249,8 +256,11 @@ static int sdcardfs_name_match(struct dir_context *ctx, const char *name, * Returns: NULL (ok), ERR_PTR if an error occurred. * Fills in lower_parent_path with on success. */ -static struct dentry *__sdcardfs_lookup(struct dentry *dentry, - unsigned int flags, struct path *lower_parent_path, userid_t id) +static struct dentry *__sdcardfs_lookup(struct inode *dir, + struct dentry *dentry, + unsigned int flags, + struct path *lower_parent_path, + userid_t id) { int err = 0; struct vfsmount *lower_dir_mnt; @@ -347,8 +357,8 @@ put_name: } sdcardfs_set_lower_path(dentry, &lower_path); - ret_dentry = - __sdcardfs_interpose(dentry, dentry->d_sb, &lower_path, id); + ret_dentry = __sdcardfs_interpose(dir, dentry, + dentry->d_sb, &lower_path, id); if (IS_ERR(ret_dentry)) { err = PTR_ERR(ret_dentry); /* path_put underlying path on error */ @@ -450,7 +460,7 @@ struct dentry *sdcardfs_lookup(struct inode *dir, struct dentry *dentry, goto out; } - ret = __sdcardfs_lookup(dentry, flags, &lower_parent_path, + ret = __sdcardfs_lookup(dir, dentry, flags, &lower_parent_path, SDCARDFS_I(dir)->data->userid); if (IS_ERR(ret)) goto out; @@ -460,7 +470,7 @@ struct dentry *sdcardfs_lookup(struct inode *dir, struct dentry *dentry, fsstack_copy_attr_times(d_inode(dentry), sdcardfs_lower_inode(d_inode(dentry))); /* get derived permission */ - get_derived_permission(parent, dentry); + get_derived_permission(dir, dentry); fixup_tmp_permissions(d_inode(dentry)); fixup_lower_ownership(dentry, dentry->d_name.name); } diff --git a/fs/sdcardfs/sdcardfs.h b/fs/sdcardfs/sdcardfs.h index 6219771ed71c..236cad94eaf0 100644 --- a/fs/sdcardfs/sdcardfs.h +++ b/fs/sdcardfs/sdcardfs.h @@ -148,8 +148,9 @@ extern struct dentry *sdcardfs_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags); extern struct inode *sdcardfs_iget(struct super_block *sb, struct inode *lower_inode, userid_t id); -extern int sdcardfs_interpose(struct dentry *dentry, struct super_block *sb, - struct path *lower_path, userid_t id); +extern int sdcardfs_interpose(struct inode *dir, struct dentry *dentry, + struct super_block *sb, + struct path *lower_path, userid_t id); /* file private data */ struct sdcardfs_file_info { @@ -177,6 +178,7 @@ struct sdcardfs_inode_info { /* top folder for ownership */ spinlock_t top_lock; + struct mutex top_mutex; struct sdcardfs_inode_data *top_data; struct inode vfs_inode; @@ -505,11 +507,13 @@ struct limit_search { extern void setup_derived_state(struct inode *inode, perm_t perm, userid_t userid, uid_t uid); -extern void get_derived_permission(struct dentry *parent, struct dentry *dentry); -extern void get_derived_permission_new(struct dentry *parent, struct dentry *dentry, const struct qstr *name); +extern void get_derived_permission(struct inode *parent, struct dentry *dentry); +extern void get_derived_permission_new(struct inode *parent, + struct inode *inode, const struct qstr *name); extern void fixup_perms_recursive(struct dentry *dentry, struct limit_search *limit); -extern void update_derived_permission_lock(struct dentry *dentry); +extern void update_derived_permission_lock(struct inode *dir, + struct inode *inode, struct dentry *dentry); void fixup_lower_ownership(struct dentry *dentry, const char *name); extern int need_graft_path(struct dentry *dentry); extern int is_base_obbpath(struct dentry *dentry); diff --git a/fs/sdcardfs/super.c b/fs/sdcardfs/super.c index 7693b0e8efef..a5347e7c7b01 100644 --- a/fs/sdcardfs/super.c +++ b/fs/sdcardfs/super.c @@ -218,6 +218,7 @@ static struct inode *sdcardfs_alloc_inode(struct super_block *sb) i->top_data = d; spin_lock_init(&i->top_lock); kref_get(&d->refcount); + mutex_init(&i->top_mutex); i->vfs_inode.i_version = 1; return &i->vfs_inode;