diff options
author | Daniel Rosenberg <drosen@google.com> | 2018-07-19 18:08:35 -0700 |
---|---|---|
committer | John Dias <joaodias@google.com> | 2018-08-10 15:33:41 -0700 |
commit | c6347674bd53e029039956befc62fa8e871a60b7 (patch) | |
tree | ad4554c63685d3929fcb8e3228f830f0063db4d1 | |
parent | 11132a4504594ec8fc70cd44a6867abb0de7d048 (diff) | |
download | tegra-c6347674bd53e029039956befc62fa8e871a60b7.tar.gz |
ANDROID: sdcardfs: Don't use OVERRIDE_CRED macroandroid-8.1.0_r0.115android-8.1.0_r0.113
The macro hides some control flow, making it easier
to run into bugs.
bug: 111642636
Change-Id: I37ec207c277d97c4e7f1e8381bc9ae743ad78435
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Daniel Rosenberg <drosen@google.com>
-rw-r--r-- | fs/sdcardfs/file.c | 8 | ||||
-rw-r--r-- | fs/sdcardfs/inode.c | 198 | ||||
-rw-r--r-- | fs/sdcardfs/lookup.c | 9 | ||||
-rw-r--r-- | fs/sdcardfs/sdcardfs.h | 25 |
4 files changed, 54 insertions, 186 deletions
diff --git a/fs/sdcardfs/file.c b/fs/sdcardfs/file.c index cd69eea6df3b..46d3696939df 100644 --- a/fs/sdcardfs/file.c +++ b/fs/sdcardfs/file.c @@ -223,7 +223,11 @@ static int sdcardfs_open(struct inode *inode, struct file *file) } /* save current_cred and override it */ - OVERRIDE_CRED(sbi, saved_cred, SDCARDFS_I(inode)); + saved_cred = override_fsids(sbi, SDCARDFS_I(inode)->data); + if (!saved_cred) { + err = -ENOMEM; + goto out_err; + } file->private_data = kzalloc(sizeof(struct sdcardfs_file_info), GFP_KERNEL); @@ -254,7 +258,7 @@ static int sdcardfs_open(struct inode *inode, struct file *file) sdcardfs_copy_and_fix_attrs(inode, sdcardfs_lower_inode(inode)); out_revert_cred: - REVERT_CRED(saved_cred); + revert_fsids(saved_cred); out_err: dput(parent); return err; diff --git a/fs/sdcardfs/inode.c b/fs/sdcardfs/inode.c index 6f599b2418d5..7bed0a7b04b4 100644 --- a/fs/sdcardfs/inode.c +++ b/fs/sdcardfs/inode.c @@ -22,7 +22,6 @@ #include <linux/fs_struct.h> #include <linux/ratelimit.h> -/* Do not directly use this function. Use OVERRIDE_CRED() instead. */ const struct cred *override_fsids(struct sdcardfs_sb_info *sbi, struct sdcardfs_inode_data *data) { @@ -50,7 +49,6 @@ const struct cred *override_fsids(struct sdcardfs_sb_info *sbi, return old_cred; } -/* Do not directly use this function, use REVERT_CRED() instead. */ void revert_fsids(const struct cred *old_cred) { const struct cred *cur_cred; @@ -78,7 +76,10 @@ static int sdcardfs_create(struct inode *dir, struct dentry *dentry, } /* save current_cred and override it */ - OVERRIDE_CRED(SDCARDFS_SB(dir->i_sb), saved_cred, SDCARDFS_I(dir)); + saved_cred = override_fsids(SDCARDFS_SB(dir->i_sb), + SDCARDFS_I(dir)->data); + if (!saved_cred) + return -ENOMEM; sdcardfs_get_lower_path(dentry, &lower_path); lower_dentry = lower_path.dentry; @@ -120,53 +121,11 @@ out: out_unlock: unlock_dir(lower_parent_dentry); sdcardfs_put_lower_path(dentry, &lower_path); - REVERT_CRED(saved_cred); + revert_fsids(saved_cred); out_eacces: return err; } -#if 0 -static int sdcardfs_link(struct dentry *old_dentry, struct inode *dir, - struct dentry *new_dentry) -{ - struct dentry *lower_old_dentry; - struct dentry *lower_new_dentry; - struct dentry *lower_dir_dentry; - u64 file_size_save; - int err; - struct path lower_old_path, lower_new_path; - - OVERRIDE_CRED(SDCARDFS_SB(dir->i_sb)); - - file_size_save = i_size_read(old_dentry->d_inode); - sdcardfs_get_lower_path(old_dentry, &lower_old_path); - sdcardfs_get_lower_path(new_dentry, &lower_new_path); - lower_old_dentry = lower_old_path.dentry; - lower_new_dentry = lower_new_path.dentry; - lower_dir_dentry = lock_parent(lower_new_dentry); - - err = vfs_link(lower_old_dentry, lower_dir_dentry->d_inode, - lower_new_dentry, NULL); - if (err || !lower_new_dentry->d_inode) - goto out; - - err = sdcardfs_interpose(new_dentry, dir->i_sb, &lower_new_path); - if (err) - goto out; - fsstack_copy_attr_times(dir, lower_new_dentry->d_inode); - fsstack_copy_inode_size(dir, lower_new_dentry->d_inode); - set_nlink(old_dentry->d_inode, - sdcardfs_lower_inode(old_dentry->d_inode)->i_nlink); - i_size_write(new_dentry->d_inode, file_size_save); -out: - unlock_dir(lower_dir_dentry); - sdcardfs_put_lower_path(old_dentry, &lower_old_path); - sdcardfs_put_lower_path(new_dentry, &lower_new_path); - REVERT_CRED(); - return err; -} -#endif - static int sdcardfs_unlink(struct inode *dir, struct dentry *dentry) { int err; @@ -183,7 +142,10 @@ static int sdcardfs_unlink(struct inode *dir, struct dentry *dentry) } /* save current_cred and override it */ - OVERRIDE_CRED(SDCARDFS_SB(dir->i_sb), saved_cred, SDCARDFS_I(dir)); + saved_cred = override_fsids(SDCARDFS_SB(dir->i_sb), + SDCARDFS_I(dir)->data); + if (!saved_cred) + return -ENOMEM; sdcardfs_get_lower_path(dentry, &lower_path); lower_dentry = lower_path.dentry; @@ -214,43 +176,11 @@ out: unlock_dir(lower_dir_dentry); dput(lower_dentry); sdcardfs_put_lower_path(dentry, &lower_path); - REVERT_CRED(saved_cred); + revert_fsids(saved_cred); out_eacces: return err; } -#if 0 -static int sdcardfs_symlink(struct inode *dir, struct dentry *dentry, - const char *symname) -{ - int err; - struct dentry *lower_dentry; - struct dentry *lower_parent_dentry = NULL; - struct path lower_path; - - OVERRIDE_CRED(SDCARDFS_SB(dir->i_sb)); - - sdcardfs_get_lower_path(dentry, &lower_path); - lower_dentry = lower_path.dentry; - lower_parent_dentry = lock_parent(lower_dentry); - - err = vfs_symlink(lower_parent_dentry->d_inode, lower_dentry, symname); - if (err) - goto out; - err = sdcardfs_interpose(dentry, dir->i_sb, &lower_path); - if (err) - goto out; - fsstack_copy_attr_times(dir, sdcardfs_lower_inode(dir)); - fsstack_copy_inode_size(dir, lower_parent_dentry->d_inode); - -out: - unlock_dir(lower_parent_dentry); - sdcardfs_put_lower_path(dentry, &lower_path); - REVERT_CRED(); - return err; -} -#endif - static int touch(char *abs_path, mode_t mode) { struct file *filp = filp_open(abs_path, O_RDWR|O_CREAT|O_EXCL|O_NOFOLLOW, mode); @@ -291,7 +221,10 @@ static int sdcardfs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode } /* save current_cred and override it */ - OVERRIDE_CRED(SDCARDFS_SB(dir->i_sb), saved_cred, SDCARDFS_I(dir)); + saved_cred = override_fsids(SDCARDFS_SB(dir->i_sb), + SDCARDFS_I(dir)->data); + if (!saved_cred) + return -ENOMEM; /* check disk space */ if (!check_min_free_space(dentry, 0, 1)) { @@ -370,13 +303,21 @@ static int sdcardfs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode if (make_nomedia_in_obb || ((pd->perm == PERM_ANDROID) && (qstr_case_eq(&dentry->d_name, &q_data)))) { - REVERT_CRED(saved_cred); - OVERRIDE_CRED(SDCARDFS_SB(dir->i_sb), saved_cred, SDCARDFS_I(dentry->d_inode)); + revert_fsids(saved_cred); + saved_cred = override_fsids(sbi, + SDCARDFS_I(dentry->d_inode)->data); + if (!saved_cred) { + pr_err("sdcardfs: failed to set up .nomedia in %s: %d\n", + lower_path.dentry->d_name.name, + -ENOMEM); + goto out; + } set_fs_pwd(current->fs, &lower_path); touch_err = touch(".nomedia", 0664); if (touch_err) { pr_err("sdcardfs: failed to create .nomedia in %s: %d\n", - lower_path.dentry->d_name.name, touch_err); + lower_path.dentry->d_name.name, + touch_err); goto out; } } @@ -389,7 +330,7 @@ out: out_unlock: sdcardfs_put_lower_path(dentry, &lower_path); out_revert: - REVERT_CRED(saved_cred); + revert_fsids(saved_cred); out_eacces: return err; } @@ -409,7 +350,10 @@ static int sdcardfs_rmdir(struct inode *dir, struct dentry *dentry) } /* save current_cred and override it */ - OVERRIDE_CRED(SDCARDFS_SB(dir->i_sb), saved_cred, SDCARDFS_I(dir)); + saved_cred = override_fsids(SDCARDFS_SB(dir->i_sb), + SDCARDFS_I(dir)->data); + if (!saved_cred) + return -ENOMEM; /* sdcardfs_get_real_lower(): in case of remove an user's obb dentry * the dentry on the original path should be deleted. @@ -434,44 +378,11 @@ static int sdcardfs_rmdir(struct inode *dir, struct dentry *dentry) out: unlock_dir(lower_dir_dentry); sdcardfs_put_real_lower(dentry, &lower_path); - REVERT_CRED(saved_cred); + revert_fsids(saved_cred); out_eacces: return err; } -#if 0 -static int sdcardfs_mknod(struct inode *dir, struct dentry *dentry, umode_t mode, - dev_t dev) -{ - int err; - struct dentry *lower_dentry; - struct dentry *lower_parent_dentry = NULL; - struct path lower_path; - - OVERRIDE_CRED(SDCARDFS_SB(dir->i_sb)); - - sdcardfs_get_lower_path(dentry, &lower_path); - lower_dentry = lower_path.dentry; - lower_parent_dentry = lock_parent(lower_dentry); - - err = vfs_mknod(lower_parent_dentry->d_inode, lower_dentry, mode, dev); - if (err) - goto out; - - err = sdcardfs_interpose(dentry, dir->i_sb, &lower_path); - if (err) - goto out; - fsstack_copy_attr_times(dir, sdcardfs_lower_inode(dir)); - fsstack_copy_inode_size(dir, lower_parent_dentry->d_inode); - -out: - unlock_dir(lower_parent_dentry); - sdcardfs_put_lower_path(dentry, &lower_path); - REVERT_CRED(); - return err; -} -#endif - /* * The locking rules in sdcardfs_rename are complex. We could use a simpler * superblock-level name-space lock for renames and copy-ups. @@ -496,7 +407,10 @@ static int sdcardfs_rename(struct inode *old_dir, struct dentry *old_dentry, } /* save current_cred and override it */ - OVERRIDE_CRED(SDCARDFS_SB(old_dir->i_sb), saved_cred, SDCARDFS_I(new_dir)); + saved_cred = override_fsids(SDCARDFS_SB(old_dir->i_sb), + SDCARDFS_I(new_dir)->data); + if (!saved_cred) + return -ENOMEM; sdcardfs_get_real_lower(old_dentry, &lower_old_path); sdcardfs_get_lower_path(new_dentry, &lower_new_path); @@ -543,7 +457,7 @@ out: dput(lower_new_dir_dentry); sdcardfs_put_real_lower(old_dentry, &lower_old_path); sdcardfs_put_lower_path(new_dentry, &lower_new_path); - REVERT_CRED(saved_cred); + revert_fsids(saved_cred); out_eacces: return err; } @@ -662,33 +576,7 @@ static int sdcardfs_permission(struct vfsmount *mnt, struct inode *inode, int ma if (IS_POSIXACL(inode)) pr_warn("%s: This may be undefined behavior...\n", __func__); err = generic_permission(&tmp, mask); - /* XXX - * Original sdcardfs code calls inode_permission(lower_inode,.. ) - * for checking inode permission. But doing such things here seems - * duplicated work, because the functions called after this func, - * such as vfs_create, vfs_unlink, vfs_rename, and etc, - * does exactly same thing, i.e., they calls inode_permission(). - * So we just let they do the things. - * If there are any security hole, just uncomment following if block. - */ -#if 0 - if (!err) { - /* - * Permission check on lower_inode(=EXT4). - * we check it with AID_MEDIA_RW permission - */ - struct inode *lower_inode; - - OVERRIDE_CRED(SDCARDFS_SB(inode->sb)); - - lower_inode = sdcardfs_lower_inode(inode); - err = inode_permission(lower_inode, mask); - - REVERT_CRED(); - } -#endif return err; - } static int sdcardfs_setattr_wrn(struct dentry *dentry, struct iattr *ia) @@ -763,7 +651,10 @@ static int sdcardfs_setattr(struct vfsmount *mnt, struct dentry *dentry, struct goto out_err; /* save current_cred and override it */ - OVERRIDE_CRED(SDCARDFS_SB(dentry->d_sb), saved_cred, SDCARDFS_I(inode)); + saved_cred = override_fsids(SDCARDFS_SB(dentry->d_sb), + SDCARDFS_I(inode)->data); + if (!saved_cred) + return -ENOMEM; sdcardfs_get_lower_path(dentry, &lower_path); lower_dentry = lower_path.dentry; @@ -822,7 +713,7 @@ static int sdcardfs_setattr(struct vfsmount *mnt, struct dentry *dentry, struct out: sdcardfs_put_lower_path(dentry, &lower_path); - REVERT_CRED(saved_cred); + revert_fsids(saved_cred); out_err: return err; } @@ -905,13 +796,6 @@ const struct inode_operations sdcardfs_dir_iops = { .setattr = sdcardfs_setattr_wrn, .setattr2 = sdcardfs_setattr, .getattr = sdcardfs_getattr, - /* XXX Following operations are implemented, - * but FUSE(sdcard) or FAT does not support them - * These methods are *NOT* perfectly tested. - .symlink = sdcardfs_symlink, - .link = sdcardfs_link, - .mknod = sdcardfs_mknod, - */ }; const struct inode_operations sdcardfs_main_iops = { diff --git a/fs/sdcardfs/lookup.c b/fs/sdcardfs/lookup.c index 369b94e77184..fd6c66c107b6 100644 --- a/fs/sdcardfs/lookup.c +++ b/fs/sdcardfs/lookup.c @@ -427,7 +427,12 @@ struct dentry *sdcardfs_lookup(struct inode *dir, struct dentry *dentry, } /* save current_cred and override it */ - OVERRIDE_CRED_PTR(SDCARDFS_SB(dir->i_sb), saved_cred, SDCARDFS_I(dir)); + saved_cred = override_fsids(SDCARDFS_SB(dir->i_sb), + SDCARDFS_I(dir)->data); + if (!saved_cred) { + ret = ERR_PTR(-ENOMEM); + goto out_err; + } sdcardfs_get_lower_path(parent, &lower_parent_path); @@ -458,7 +463,7 @@ struct dentry *sdcardfs_lookup(struct inode *dir, struct dentry *dentry, out: sdcardfs_put_lower_path(parent, &lower_parent_path); - REVERT_CRED(saved_cred); + revert_fsids(saved_cred); out_err: dput(parent); return ret; diff --git a/fs/sdcardfs/sdcardfs.h b/fs/sdcardfs/sdcardfs.h index 2488a94f4b0e..7862c8b563cd 100644 --- a/fs/sdcardfs/sdcardfs.h +++ b/fs/sdcardfs/sdcardfs.h @@ -87,31 +87,6 @@ (x)->i_mode = ((x)->i_mode & S_IFMT) | 0775;\ } while (0) -/* OVERRIDE_CRED() and REVERT_CRED() - * OVERRIDE_CRED() - * backup original task->cred - * and modifies task->cred->fsuid/fsgid to specified value. - * REVERT_CRED() - * restore original task->cred->fsuid/fsgid. - * These two macro should be used in pair, and OVERRIDE_CRED() should be - * placed at the beginning of a function, right after variable declaration. - */ -#define OVERRIDE_CRED(sdcardfs_sbi, saved_cred, info) \ - do { \ - saved_cred = override_fsids(sdcardfs_sbi, info->data); \ - if (!saved_cred) \ - return -ENOMEM; \ - } while (0) - -#define OVERRIDE_CRED_PTR(sdcardfs_sbi, saved_cred, info) \ - do { \ - saved_cred = override_fsids(sdcardfs_sbi, info->data); \ - if (!saved_cred) \ - return ERR_PTR(-ENOMEM); \ - } while (0) - -#define REVERT_CRED(saved_cred) revert_fsids(saved_cred) - /* Android 5.0 support */ /* Permission mode for a specific node. Controls how file permissions |