Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 29d3d43a authored by Daniel Rosenberg's avatar Daniel Rosenberg Committed by Todd Kjos
Browse files

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: default avatarJann Horn <jannh@google.com>
Signed-off-by: default avatarDaniel Rosenberg <drosen@google.com>
parent ff9973a5

fs/sdcardfs/file.c

+18 −6
Original line number Diff line number Diff line
@@ -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;

fs/sdcardfs/inode.c

+41 −157
Original line number Diff line number Diff line
@@ -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;
@@ -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(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;
@@ -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;
@@ -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(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);
@@ -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);
@@ -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(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;

		}

	}
@@ -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;

}
@@ -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.
@@ -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(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.
@@ -493,7 +404,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);
@@ -540,7 +454,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;

}
@@ -660,33 +574,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)
@@ -764,7 +652,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;
@@ -823,7 +714,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;

}
@@ -907,13 +798,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 = {

fs/sdcardfs/lookup.c

+7 −2
Original line number Diff line number Diff line
@@ -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;

fs/sdcardfs/sdcardfs.h

+0 −25
Original line number Diff line number Diff line
@@ -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