ANDROID: sdcardfs: Don't use OVERRIDE_CRED macro
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>
diff --git a/fs/sdcardfs/file.c b/fs/sdcardfs/file.c
index 15f7a72..8a1e778 100644
--- a/fs/sdcardfs/file.c
+++ b/fs/sdcardfs/file.c
@@ -118,7 +118,11 @@
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);
@@ -127,7 +131,7 @@
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;
}
@@ -149,12 +153,16 @@
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;
}
@@ -241,7 +249,11 @@
}
/* 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);
@@ -271,7 +283,7 @@
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 7a2e23c..324693e 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 @@
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 @@
}
/* 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_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 @@
}
/* 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 @@
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);
@@ -292,7 +222,10 @@
}
/* 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);
@@ -374,13 +307,21 @@
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;
}
}
@@ -393,7 +334,7 @@
out_unlock:
sdcardfs_put_lower_path(dentry, &lower_path);
out_revert:
- REVERT_CRED(saved_cred);
+ revert_fsids(saved_cred);
out_eacces:
return err;
}
@@ -413,7 +354,10 @@
}
/* 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.
@@ -438,44 +382,11 @@
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.
@@ -500,7 +411,10 @@
}
/* 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);
@@ -547,7 +461,7 @@
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;
}
@@ -668,33 +582,7 @@
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)
@@ -769,7 +657,10 @@
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;
@@ -828,7 +719,7 @@
out:
sdcardfs_put_lower_path(dentry, &lower_path);
- REVERT_CRED(saved_cred);
+ revert_fsids(saved_cred);
out_err:
return err;
}
@@ -911,13 +802,6 @@
.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 3395371..5fb9be5 100644
--- a/fs/sdcardfs/lookup.c
+++ b/fs/sdcardfs/lookup.c
@@ -426,7 +426,12 @@
}
/* 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);
@@ -457,7 +462,7 @@
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 3996fc2..845832f 100644
--- a/fs/sdcardfs/sdcardfs.h
+++ b/fs/sdcardfs/sdcardfs.h
@@ -88,31 +88,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
