| Message ID | 20190118145344.11532-3-christian@brauner.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | binderfs: debug galore | expand |
On Fri, Jan 18, 2019 at 03:53:41PM +0100, Christian Brauner wrote: > We don't allow to unlink it since it is crucial for binderfs to be useable > but if we allow to rename it we make the unlink trivial to bypass. So > prevent renaming too and simply treat the control dentry as immutable. > > Take the opportunity and turn the check for the control dentry into a > separate helper is_binderfs_control_device() since it's now used in two > places. > Additionally, replace the custom rename dance we did with call to > simple_rename(). Umm... > +static inline bool is_binderfs_control_device(const struct inode *inode, > + const struct dentry *dentry) > +{ > + return BINDERFS_I(inode)->control_dentry == dentry; > +} What do you need an inode for? static inline struct binderfs_info *BINDERFS_I(const struct inode *inode) { return inode->i_sb->s_fs_info; } so it looks like all you care about is the superblock. Which can be had simply as dentry->d_sb... Besides, what's the point of calling is_binderfs_device() in ->rename()? If your directory methods are given dentries from another filesystem, the kernel is already FUBAR. So your rename should simply do if (is_binderfs_control_device(old_dentry) || is_binderfs_control_device(new_dentry)) return -EPERM; return simple_rename(......); and that's it...
On Fri, Jan 18, 2019 at 10:55:52PM +0000, Al Viro wrote: > On Fri, Jan 18, 2019 at 03:53:41PM +0100, Christian Brauner wrote: > > We don't allow to unlink it since it is crucial for binderfs to be useable > > but if we allow to rename it we make the unlink trivial to bypass. So > > prevent renaming too and simply treat the control dentry as immutable. > > > > Take the opportunity and turn the check for the control dentry into a > > separate helper is_binderfs_control_device() since it's now used in two > > places. > > Additionally, replace the custom rename dance we did with call to > > simple_rename(). > > Umm... > > > +static inline bool is_binderfs_control_device(const struct inode *inode, > > + const struct dentry *dentry) > > +{ > > + return BINDERFS_I(inode)->control_dentry == dentry; > > +} > > What do you need an inode for? BINDERFS_I() is called in is_binderfs_device() which is called in binder.c: static int binder_open(struct inode *nodp, struct file *filp) { <snip> /* binderfs stashes devices in i_private */ if (is_binderfs_device(nodp)) binder_dev = nodp->i_private; else binder_dev = container_of(filp->private_data, struct binder_device, miscdev); <snip> and it was just easier to have it take an inode as arg since that's what binder_open() makes easily available. Just kept it this way for is_binderfs_control_device() too but will switch the latter over now. > > static inline struct binderfs_info *BINDERFS_I(const struct inode *inode) > { > return inode->i_sb->s_fs_info; > } > > so it looks like all you care about is the superblock. Which can be > had simply as dentry->d_sb... Hm yes, I think I'll just do: static inline bool is_binderfs_control_device(const struct dentry *dentry) { struct binderfs_info *info = dentry->d_sb->s_fs_info; return info->control_dentry == dentry; } and pick up what you scratched below. > > Besides, what's the point of calling is_binderfs_device() in ->rename()? > If your directory methods are given dentries from another filesystem, > the kernel is already FUBAR. So your rename should simply do > if (is_binderfs_control_device(old_dentry) || > is_binderfs_control_device(new_dentry)) > return -EPERM; > return simple_rename(......); > and that's it...
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c index 898d847f8505..02c96b5edfa9 100644 --- a/drivers/android/binderfs.c +++ b/drivers/android/binderfs.c @@ -346,34 +346,33 @@ static const struct super_operations binderfs_super_ops = { .statfs = simple_statfs, }; +static inline bool is_binderfs_control_device(const struct inode *inode, + const struct dentry *dentry) +{ + return BINDERFS_I(inode)->control_dentry == dentry; +} + static int binderfs_rename(struct inode *old_dir, struct dentry *old_dentry, struct inode *new_dir, struct dentry *new_dentry, unsigned int flags) { - struct inode *inode = d_inode(old_dentry); - - /* binderfs doesn't support directories. */ - if (d_is_dir(old_dentry)) - return -EPERM; + const struct inode *inode = d_inode(old_dentry); - if (flags & ~RENAME_NOREPLACE) - return -EINVAL; - - if (!simple_empty(new_dentry)) - return -ENOTEMPTY; - - if (d_really_is_positive(new_dentry)) - simple_unlink(new_dir, new_dentry); + if (is_binderfs_device(d_inode(old_dentry))) + inode = d_inode(old_dentry); + else + inode = d_inode(new_dentry); - old_dir->i_ctime = old_dir->i_mtime = new_dir->i_ctime = - new_dir->i_mtime = inode->i_ctime = current_time(old_dir); + if (is_binderfs_control_device(inode, old_dentry) || + is_binderfs_control_device(inode, new_dentry)) + return -EPERM; - return 0; + return simple_rename(old_dir, old_dentry, new_dir, new_dentry, flags); } static int binderfs_unlink(struct inode *dir, struct dentry *dentry) { - if (BINDERFS_I(dir)->control_dentry == dentry) + if (is_binderfs_control_device(dir, dentry)) return -EPERM; return simple_unlink(dir, dentry);
We don't allow to unlink it since it is crucial for binderfs to be useable but if we allow to rename it we make the unlink trivial to bypass. So prevent renaming too and simply treat the control dentry as immutable. Take the opportunity and turn the check for the control dentry into a separate helper is_binderfs_control_device() since it's now used in two places. Additionally, replace the custom rename dance we did with call to simple_rename(). Suggested-by: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Christian Brauner <christian@brauner.io> --- drivers/android/binderfs.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-)