[RAMEN9610-10347] Subject: [PATCH] ANDROID: sdcardfs: Don't use OVERRIDE_CRED macro
authorJaeHun Jung <jh0801.jung@samsung.com>
Thu, 3 Jan 2019 05:02:25 +0000 (14:02 +0900)
committerhskang <hs1218.kang@samsung.com>
Fri, 4 Jan 2019 03:57:46 +0000 (12:57 +0900)
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>
fs/sdcardfs/file.c
fs/sdcardfs/inode.c
fs/sdcardfs/lookup.c
fs/sdcardfs/sdcardfs.h

index 1461254f301dcf81192dab8044fa36f93b8a6546..271c4c4cb760f829af12cd810943e9580e4a2177 100644 (file)
@@ -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);
@@ -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;
 }
@@ -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;
 }
@@ -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);
@@ -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;
index f28db0afa596a3fbeaef16f8e5a724be1cb7b550..4dd681e0d59d948374883a7e34ed8318d4537e23 100644 (file)
@@ -23,7 +23,6 @@
 #include <linux/ratelimit.h>
 #include <linux/sched/task.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)
 {
@@ -51,7 +50,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;
@@ -79,7 +77,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;
@@ -121,53 +122,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(d_inode(old_dentry));
-       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, d_inode(lower_dir_dentry),
-                      lower_new_dentry, NULL);
-       if (err || !d_inode(lower_new_dentry))
-               goto out;
-
-       err = sdcardfs_interpose(new_dentry, dir->i_sb, &lower_new_path);
-       if (err)
-               goto out;
-       fsstack_copy_attr_times(dir, d_inode(lower_new_dentry));
-       fsstack_copy_inode_size(dir, d_inode(lower_new_dentry));
-       set_nlink(d_inode(old_dentry),
-                 sdcardfs_lower_inode(d_inode(old_dentry))->i_nlink);
-       i_size_write(d_inode(new_dentry), 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;
@@ -184,7 +143,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;
@@ -215,43 +177,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(d_inode(lower_parent_dentry), 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, d_inode(lower_parent_dentry));
-
-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);
@@ -293,7 +223,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);
@@ -375,13 +308,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(d_inode(dentry)));
+               revert_fsids(saved_cred);
+               saved_cred = override_fsids(sbi,
+                                       SDCARDFS_I(d_inode(dentry))->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;
                }
        }
@@ -394,7 +335,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;
 }
@@ -414,7 +355,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.
@@ -439,44 +383,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(d_inode(lower_parent_dentry), 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, d_inode(lower_parent_dentry));
-
-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.
@@ -505,7 +416,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);
@@ -552,7 +466,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;
 }
@@ -672,33 +586,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)
@@ -776,7 +664,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;
@@ -835,7 +726,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;
 }
@@ -919,13 +810,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 = {
index 2ff3051618820847de4e20cf136e466bfc5f6abf..73179ce2591fdb2704691ea25701b76efb265a18 100644 (file)
@@ -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;
index 826afb5c7e88360a42f7dff18879851b76440f9a..ec2290a16d3cbf0e5989c4cf9f48f32a992ffb48 100644 (file)
                (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