Skip to content

Commit

Permalink
ANDROID: sdcardfs: Don't use OVERRIDE_CRED macro
Browse files Browse the repository at this point in the history 
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>
Signed-off-by: khusika <khusikadhamar@gmail.com>
  • Loading branch information
@drosen-google @khusika
drosen-google authored and khusika committed Oct 13, 2018
1 parent 24eccf1 commit aa91db1
Show file tree
Hide file tree
Showing 4 changed files with 66 additions and 190 deletions.
24 changes: 18 additions & 6 deletions fs/sdcardfs/file.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,11 @@ static long sdcardfs_unlocked_ioctl(struct file *file, unsigned int cmd,
goto out;

/* save current_cred and override it */
OVERRIDE_CRED(sbi, saved_cred, SDCARDFS_I(file_inode(file)));
saved_cred = override_fsids(sbi, SDCARDFS_I(file_inode(file))->data);
if (!saved_cred) {
err = -ENOMEM;
goto out;
}

if (lower_file->f_op->unlocked_ioctl)
err = lower_file->f_op->unlocked_ioctl(lower_file, cmd, arg);
Expand All @@ -127,7 +131,7 @@ static long sdcardfs_unlocked_ioctl(struct file *file, unsigned int cmd,
if (!err)
sdcardfs_copy_and_fix_attrs(file_inode(file),
file_inode(lower_file));
REVERT_CRED(saved_cred);
revert_fsids(saved_cred);
out:
return err;
}
Expand All @@ -149,12 +153,16 @@ static long sdcardfs_compat_ioctl(struct file *file, unsigned int cmd,
goto out;

/* save current_cred and override it */
OVERRIDE_CRED(sbi, saved_cred, SDCARDFS_I(file_inode(file)));
saved_cred = override_fsids(sbi, SDCARDFS_I(file_inode(file))->data);
if (!saved_cred) {
err = -ENOMEM;
goto out;
}

if (lower_file->f_op->compat_ioctl)
err = lower_file->f_op->compat_ioctl(lower_file, cmd, arg);

REVERT_CRED(saved_cred);
revert_fsids(saved_cred);
out:
return err;
}
Expand Down Expand Up @@ -241,7 +249,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);
Expand Down Expand Up @@ -271,7 +283,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;
Expand Down
198 changes: 41 additions & 157 deletions fs/sdcardfs/inode.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -115,53 +116,11 @@ static int sdcardfs_create(struct inode *dir, struct dentry *dentry,
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;
Expand All @@ -178,7 +137,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;
Expand Down Expand Up @@ -209,43 +171,11 @@ static int sdcardfs_unlink(struct inode *dir, struct dentry *dentry)
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);
Expand Down Expand Up @@ -287,7 +217,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 */
parent_dentry = dget_parent(dentry);
Expand Down Expand Up @@ -366,13 +299,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;
}
}
Expand All @@ -382,7 +323,7 @@ static int sdcardfs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode
out_unlock:
sdcardfs_put_lower_path(dentry, &lower_path);
out_revert:
REVERT_CRED(saved_cred);
revert_fsids(saved_cred);
out_eacces:
return err;
}
Expand All @@ -402,7 +343,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.
Expand All @@ -427,44 +371,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.
Expand All @@ -489,7 +400,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);
Expand Down Expand Up @@ -536,7 +450,7 @@ static int sdcardfs_rename(struct inode *old_dir, struct dentry *old_dentry,
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;
}
Expand Down Expand Up @@ -657,33 +571,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)
Expand Down Expand Up @@ -758,7 +646,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;
Expand Down Expand Up @@ -817,7 +708,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;
}
Expand Down Expand Up @@ -900,13 +791,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 = {
Expand Down
Loading

0 comments on commit aa91db1

Please sign in to comment.