[PATCH 0/5] binderfs: debug galore

[PATCH 0/5] binderfs: debug galore

[Christian Brauner]

Hey everyone,

Al gave me a really helpful review of binderfs and pointed out a range
of bugs. The most obvious and serious ones have fortunately already been
taken care of by patches sitting in Greg's char-misc-linus tree. The
others are hopefully all covered in this patchset.

Thanks!
Christian

Christian Brauner (5):
  binderfs: remove outdated comment
  binderfs: prevent renaming the control dentry
  binderfs: rework binderfs_fill_super()
  binderfs: kill_litter_super() before cleanup
  binderfs: drop lock in binderfs_binder_ctl_create

 drivers/android/binderfs.c | 79 +++++++++++++-------------------------
 1 file changed, 27 insertions(+), 52 deletions(-)

-- 
2.19.1

[PATCH 1/5] binderfs: remove outdated comment

[Christian Brauner]

The comment stems from an early version of that patchset and is just
confusing now.

Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Christian Brauner <christian@brauner.io>
---
 drivers/android/binderfs.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index e4ff4c3fa371..898d847f8505 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -373,10 +373,6 @@ static int binderfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 
 static int binderfs_unlink(struct inode *dir, struct dentry *dentry)
 {
-	/*
-	 * The control dentry is only ever touched during mount so checking it
-	 * here should not require us to take lock.
-	 */
 	if (BINDERFS_I(dir)->control_dentry == dentry)
 		return -EPERM;
 
-- 
2.19.1

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

[Christian Brauner]

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(-)

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);
-- 
2.19.1

[PATCH 3/5] binderfs: rework binderfs_fill_super()

[Christian Brauner]

Al pointed out that on binderfs_fill_super() error
deactivate_locked_super() will call binderfs_kill_super() so all of the
freeing and putting we currently do in binderfs_fill_super() is unnecessary
and buggy. Let's simply return errors and let binderfs_fill_super() take
care of cleaning up on error.

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Christian Brauner <christian@brauner.io>
---
 drivers/android/binderfs.c | 35 +++++++++--------------------------
 1 file changed, 9 insertions(+), 26 deletions(-)

diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 02c96b5edfa9..c0fa495ee994 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -468,8 +468,8 @@ static const struct inode_operations binderfs_dir_inode_operations = {
 
 static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
 {
+	int ret;
 	struct binderfs_info *info;
-	int ret = -ENOMEM;
 	struct inode *inode = NULL;
 	struct ipc_namespace *ipc_ns = current->nsproxy->ipc_ns;
 
@@ -495,13 +495,14 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_op = &binderfs_super_ops;
 	sb->s_time_gran = 1;
 
-	info = kzalloc(sizeof(struct binderfs_info), GFP_KERNEL);
-	if (!info)
-		goto err_without_dentry;
+	sb->s_fs_info = kzalloc(sizeof(struct binderfs_info), GFP_KERNEL);
+	if (!sb->s_fs_info)
+		return -ENOMEM;
+	info = sb->s_fs_info;
 
 	ret = binderfs_parse_mount_opts(data, &info->mount_opts);
 	if (ret)
-		goto err_without_dentry;
+		return ret;
 
 	info->ipc_ns = ipc_ns;
 	info->root_gid = make_kgid(sb->s_user_ns, 0);
@@ -511,12 +512,9 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
 	if (!uid_valid(info->root_uid))
 		info->root_uid = GLOBAL_ROOT_UID;
 
-	sb->s_fs_info = info;
-
-	ret = -ENOMEM;
 	inode = new_inode(sb);
 	if (!inode)
-		goto err_without_dentry;
+		return -ENOMEM;
 
 	inode->i_ino = FIRST_INODE;
 	inode->i_fop = &simple_dir_operations;
@@ -527,24 +525,9 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
 
 	sb->s_root = d_make_root(inode);
 	if (!sb->s_root)
-		goto err_without_dentry;
-
-	ret = binderfs_binder_ctl_create(sb);
-	if (ret)
-		goto err_with_dentry;
-
-	return 0;
-
-err_with_dentry:
-	dput(sb->s_root);
-	sb->s_root = NULL;
-
-err_without_dentry:
-	put_ipc_ns(ipc_ns);
-	iput(inode);
-	kfree(info);
+		return -ENOMEM;
 
-	return ret;
+	return binderfs_binder_ctl_create(sb);
 }
 
 static struct dentry *binderfs_mount(struct file_system_type *fs_type,
-- 
2.19.1

[PATCH 4/5] binderfs: kill_litter_super() before cleanup

[Christian Brauner]

Al pointed out that first calling kill_litter_super() before cleaning up
info is more correct since destroying info doesn't depend on the state of
the dentries and inodes. That the opposite remains true is not guaranteed.

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Christian Brauner <christian@brauner.io>
---
 drivers/android/binderfs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index c0fa495ee994..c5feb9ffce0f 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -541,11 +541,12 @@ static void binderfs_kill_super(struct super_block *sb)
 {
 	struct binderfs_info *info = sb->s_fs_info;
 
+	kill_litter_super(sb);
+
 	if (info && info->ipc_ns)
 		put_ipc_ns(info->ipc_ns);
 
 	kfree(info);
-	kill_litter_super(sb);
 }
 
 static struct file_system_type binder_fs_type = {
-- 
2.19.1

[PATCH 5/5] binderfs: drop lock in binderfs_binder_ctl_create

[Christian Brauner]

The binderfs_binder_ctl_create() call is a no-op on subsequent calls and
the first call is done before we unlock the suberblock. Hence, there is no
need to take inode_lock() in there. Let's remove it.

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Christian Brauner <christian@brauner.io>
---
Note, that fs/devptfs/inode.c:mknod_ptmx() is currently holding
inode_lock() too under the exact same circumstances. Seems that we can drop
it from there too.
---
 drivers/android/binderfs.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index c5feb9ffce0f..db09274a1b75 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -408,8 +408,6 @@ static int binderfs_binder_ctl_create(struct super_block *sb)
 	if (!device)
 		return -ENOMEM;
 
-	inode_lock(d_inode(root));
-
 	/* If we have already created a binder-control node, return. */
 	if (info->control_dentry) {
 		ret = 0;
@@ -448,12 +446,10 @@ static int binderfs_binder_ctl_create(struct super_block *sb)
 	inode->i_private = device;
 	info->control_dentry = dentry;
 	d_add(dentry, inode);
-	inode_unlock(d_inode(root));
 
 	return 0;
 
 out:
-	inode_unlock(d_inode(root));
 	kfree(device);
 	iput(inode);
 
-- 
2.19.1

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

[Al Viro]

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...

Re: [PATCH 3/5] binderfs: rework binderfs_fill_super()

[Al Viro]

On Fri, Jan 18, 2019 at 03:53:42PM +0100, Christian Brauner wrote:
>  static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
>  {
> +	int ret;
>  	struct binderfs_info *info;
> -	int ret = -ENOMEM;
>  	struct inode *inode = NULL;
>  	struct ipc_namespace *ipc_ns = current->nsproxy->ipc_ns;
>  
> @@ -495,13 +495,14 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
>  	sb->s_op = &binderfs_super_ops;
>  	sb->s_time_gran = 1;
>  
> -	info = kzalloc(sizeof(struct binderfs_info), GFP_KERNEL);
> -	if (!info)
> -		goto err_without_dentry;
> +	sb->s_fs_info = kzalloc(sizeof(struct binderfs_info), GFP_KERNEL);
> +	if (!sb->s_fs_info)
> +		return -ENOMEM;
> +	info = sb->s_fs_info;

... and that's when you should grab ipcns reference and stick it into
info->ipc_ns, to match the logics in binderfs_kill_super().

Otherwise the failure above

>  	ret = binderfs_parse_mount_opts(data, &info->mount_opts);
>  	if (ret)
> -		goto err_without_dentry;
> +		return ret;

... or here leaves you with an ipcns leak.

Destructor does
	if ->s_fs_info is non-NULL
		release ->s_fs_info->ipc_ns
		free ->s_fs_info
so constructor should not leave object in a state when ipcns is already
grabbed, but not stored in ->s_fs_info->ipc_ns (including the case of
allocation failure leaving it with NULL ->s_fs_info).

Re: [PATCH 0/5] binderfs: debug galore

[Al Viro]

On Fri, Jan 18, 2019 at 03:53:39PM +0100, Christian Brauner wrote:
> Hey everyone,
> 
> Al gave me a really helpful review of binderfs and pointed out a range
> of bugs. The most obvious and serious ones have fortunately already been
> taken care of by patches sitting in Greg's char-misc-linus tree. The
> others are hopefully all covered in this patchset.

BTW, binderfs_binder_device_create() looks rather odd - it would be easier
to do this:
        inode_lock(d_inode(root));
	/* look it up */
        dentry = lookup_one_len(name, root, strlen(name));
	if (IS_ERR(dentry)) {
		/* some kind of error (ENOMEM, permissions) - report */
		inode_unlock(d_inode(root));
		ret = PTR_ERR(dentry);
		goto err;
	}
	if (d_really_is_positive(dentry)) {
		/* already exists */
		dput(dentry);
		inode_unlock(d_inode(root));
		ret = -EEXIST;
		goto err;
	}
	inode->i_private = device;
... and from that point on - as in your variant.  Another thing in there:
        name = kmalloc(name_len, GFP_KERNEL);
        if (!name)
                goto err;

        strscpy(name, req->name, name_len);
is an odd way to go; more straightforward would be
	req->name[BINDERFS_MAX_NAME] = '\0';	/* NUL-terminate */
	name = kmemdup(req->name, sizeof(req->name), GEP_KERNEL);
	if (!name)
		....

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

[Christian Brauner]

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...

Re: [PATCH 3/5] binderfs: rework binderfs_fill_super()

[Christian Brauner]

On Fri, Jan 18, 2019 at 11:03:54PM +0000, Al Viro wrote:
> On Fri, Jan 18, 2019 at 03:53:42PM +0100, Christian Brauner wrote:
> >  static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
> >  {
> > +	int ret;
> >  	struct binderfs_info *info;
> > -	int ret = -ENOMEM;
> >  	struct inode *inode = NULL;
> >  	struct ipc_namespace *ipc_ns = current->nsproxy->ipc_ns;
> >  
> > @@ -495,13 +495,14 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
> >  	sb->s_op = &binderfs_super_ops;
> >  	sb->s_time_gran = 1;
> >  
> > -	info = kzalloc(sizeof(struct binderfs_info), GFP_KERNEL);
> > -	if (!info)
> > -		goto err_without_dentry;
> > +	sb->s_fs_info = kzalloc(sizeof(struct binderfs_info), GFP_KERNEL);
> > +	if (!sb->s_fs_info)
> > +		return -ENOMEM;
> > +	info = sb->s_fs_info;
> 
> ... and that's when you should grab ipcns reference and stick it into
> info->ipc_ns, to match the logics in binderfs_kill_super().
> 
> Otherwise the failure above
> 
> >  	ret = binderfs_parse_mount_opts(data, &info->mount_opts);
> >  	if (ret)
> > -		goto err_without_dentry;
> > +		return ret;
> 
> ... or here leaves you with an ipcns leak.
> 
> Destructor does
> 	if ->s_fs_info is non-NULL
> 		release ->s_fs_info->ipc_ns
> 		free ->s_fs_info
> so constructor should not leave object in a state when ipcns is already
> grabbed, but not stored in ->s_fs_info->ipc_ns (including the case of
> allocation failure leaving it with NULL ->s_fs_info).

Yeah, total brainfart on my side. I shouldn't code in airports
apparently... Fixed.

Re: [PATCH 0/5] binderfs: debug galore

[Christian Brauner]

On Fri, Jan 18, 2019 at 11:26:34PM +0000, Al Viro wrote:
> On Fri, Jan 18, 2019 at 03:53:39PM +0100, Christian Brauner wrote:
> > Hey everyone,
> > 
> > Al gave me a really helpful review of binderfs and pointed out a range
> > of bugs. The most obvious and serious ones have fortunately already been
> > taken care of by patches sitting in Greg's char-misc-linus tree. The
> > others are hopefully all covered in this patchset.
> 
> BTW, binderfs_binder_device_create() looks rather odd - it would be easier
> to do this:
>         inode_lock(d_inode(root));
> 	/* look it up */
>         dentry = lookup_one_len(name, root, strlen(name));

It didn't seem obvious that that's what you should use to lookup
dentries instead of d_lookup(). Especially since d_lookup() points out
that it takes the rename lock which seemed crucial to me so that we
don't end up in a situation where we race against another dentry being
renamed to the name we're currently trying to used.
Thanks for the pointer!

> 	if (IS_ERR(dentry)) {
> 		/* some kind of error (ENOMEM, permissions) - report */
> 		inode_unlock(d_inode(root));
> 		ret = PTR_ERR(dentry);
> 		goto err;
> 	}
> 	if (d_really_is_positive(dentry)) {
> 		/* already exists */
> 		dput(dentry);
> 		inode_unlock(d_inode(root));
> 		ret = -EEXIST;
> 		goto err;
> 	}
> 	inode->i_private = device;
> ... and from that point on - as in your variant.  Another thing in there:

Right, just read through the code for lookup_one_len() it seems to
allocate a new dentry if it can't find a hashed match for the old name.
That's surprising. The name of the function does not really give that
impression. :) (Would probably be better if lookup_or_alloc_one_len() or
something. But that's not important now.)

>         name = kmalloc(name_len, GFP_KERNEL);
>         if (!name)
>                 goto err;
> 
>         strscpy(name, req->name, name_len);
> is an odd way to go; more straightforward would be
> 	req->name[BINDERFS_MAX_NAME] = '\0';	/* NUL-terminate */
> 	name = kmemdup(req->name, sizeof(req->name), GEP_KERNEL);
> 	if (!name)
> 		....

Using kmemdup() now.