Re: [PATCH 2/5] binderfs: prevent renaming the control dentry

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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...
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux