configfs: make directories inherit uid/gid from creator
diff mbox series

Message ID 20210123205516.2738060-1-zenczykowski@gmail.com
State New, archived
Headers show
Series
  • configfs: make directories inherit uid/gid from creator
Related show

Commit Message

Maciej Żenczykowski Jan. 23, 2021, 8:55 p.m. UTC
From: Maciej Żenczykowski <maze@google.com>

Currently a non-root process can create directories, but cannot
create stuff in the directories it creates.

Example (before this patch):
  phone:/ $ id
  uid=1000(system) gid=1000(system) groups=1000(system),... context=u:r:su:s0

  phone:/ $ cd /config/usb_gadget/g1/configs/

  phone:/config/usb_gadget/g1/configs $ ls -lZ
  drwxr-xr-x 3 system system u:object_r:configfs:s0  0 2020-12-28 06:03 b.1

  phone:/config/usb_gadget/g1/configs $ mkdir b.2

  phone:/config/usb_gadget/g1/configs $ ls -lZ
  drwxr-xr-x 3 system system u:object_r:configfs:s0  0 2020-12-28 06:03 b.1
  drwxr-xr-x 3 root   root   u:object_r:configfs:s0  0 2020-12-28 06:51 b.2

  phone:/config/usb_gadget/g1/configs $ chown system:system b.2
  chown: 'b.2' to 'system:system': Operation not permitted

  phone:/config/usb_gadget/g1/configs $ ls -lZ b.1
  -rw-r--r-- 1 system system u:object_r:configfs:s0  4096 2020-12-28 05:23 MaxPower
  -rw-r--r-- 1 system system u:object_r:configfs:s0  4096 2020-12-28 05:23 bmAttributes
  lrwxrwxrwx 1 root   root   u:object_r:configfs:s0     0 2020-12-28 05:23 function0 -> ../../../../usb_gadget/g1/functions/ffs.adb
  drwxr-xr-x 2 system system u:object_r:configfs:s0     0 2020-12-28 05:23 strings

  phone:/config/usb_gadget/g1/configs $ ln -s ../../../../usb_gadget/g1/functions/ffs.adb b.2/function0
  ln: cannot create symbolic link from '../../../../usb_gadget/g1/functions/ffs.adb' to 'b.2/function0': Permission denied

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Joel Becker <jlbec@evilplan.org>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Maciej Żenczykowski <maze@google.com>
Change-Id: Ia907b2def940197b44aa87b337d37c5dde9c5b91
---
 fs/configfs/dir.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Greg KH Jan. 27, 2021, 1:47 p.m. UTC | #1
On Sat, Jan 23, 2021 at 12:55:16PM -0800, Maciej Żenczykowski wrote:
> From: Maciej Żenczykowski <maze@google.com>
> 
> Currently a non-root process can create directories, but cannot
> create stuff in the directories it creates.

Isn't that on purpose?

> 
> Example (before this patch):
>   phone:/ $ id
>   uid=1000(system) gid=1000(system) groups=1000(system),... context=u:r:su:s0
> 
>   phone:/ $ cd /config/usb_gadget/g1/configs/
> 
>   phone:/config/usb_gadget/g1/configs $ ls -lZ
>   drwxr-xr-x 3 system system u:object_r:configfs:s0  0 2020-12-28 06:03 b.1
> 
>   phone:/config/usb_gadget/g1/configs $ mkdir b.2
> 
>   phone:/config/usb_gadget/g1/configs $ ls -lZ
>   drwxr-xr-x 3 system system u:object_r:configfs:s0  0 2020-12-28 06:03 b.1
>   drwxr-xr-x 3 root   root   u:object_r:configfs:s0  0 2020-12-28 06:51 b.2
> 
>   phone:/config/usb_gadget/g1/configs $ chown system:system b.2
>   chown: 'b.2' to 'system:system': Operation not permitted
> 
>   phone:/config/usb_gadget/g1/configs $ ls -lZ b.1
>   -rw-r--r-- 1 system system u:object_r:configfs:s0  4096 2020-12-28 05:23 MaxPower
>   -rw-r--r-- 1 system system u:object_r:configfs:s0  4096 2020-12-28 05:23 bmAttributes
>   lrwxrwxrwx 1 root   root   u:object_r:configfs:s0     0 2020-12-28 05:23 function0 -> ../../../../usb_gadget/g1/functions/ffs.adb
>   drwxr-xr-x 2 system system u:object_r:configfs:s0     0 2020-12-28 05:23 strings
> 
>   phone:/config/usb_gadget/g1/configs $ ln -s ../../../../usb_gadget/g1/functions/ffs.adb b.2/function0
>   ln: cannot create symbolic link from '../../../../usb_gadget/g1/functions/ffs.adb' to 'b.2/function0': Permission denied
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Joel Becker <jlbec@evilplan.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Maciej Żenczykowski <maze@google.com>
> Change-Id: Ia907b2def940197b44aa87b337d37c5dde9c5b91

No need for "Change-Id:" :)

> ---
>  fs/configfs/dir.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index b839dd1b459f..04f18402ef7c 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -1410,6 +1410,21 @@ static int configfs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode
>  	else
>  		ret = configfs_attach_item(parent_item, item, dentry, frag);
>  
> +	/* inherit uid/gid from process creating the directory */
> +	if (!uid_eq(current_fsuid(), GLOBAL_ROOT_UID) ||
> +	    !gid_eq(current_fsgid(), GLOBAL_ROOT_GID)) {
> +		struct inode * inode = d_inode(dentry);
> +		struct iattr ia = {
> +			.ia_uid = current_fsuid(),
> +			.ia_gid = current_fsgid(),
> +			.ia_valid = ATTR_UID | ATTR_GID,
> +		};
> +		inode->i_uid = ia.ia_uid;
> +		inode->i_gid = ia.ia_gid;
> +		/* (note: the above manual assignments skip the permission checks) */
> +		(void)configfs_setattr(dentry, &ia);

No need for (void), here, right?

And this feels like a big user-visible change, what is going to break
with this?

thanks,

greg k-h
Maciej Żenczykowski Jan. 27, 2021, 7:56 p.m. UTC | #2
> > Currently a non-root process can create directories, but cannot
> > create stuff in the directories it creates.
>
> Isn't that on purpose?

Is it?  What's the use case of being able to create empty directories
you can't use?
Why allow their creation in the first place then?

> > +             (void)configfs_setattr(dentry, &ia);
>
> No need for (void), here, right?

Just being clear we're ignoring any potential error return.

> And this feels like a big user-visible change, what is going to break
> with this?

That I don't know, but it's unlikely to break anything, since it's virtually
impossible to use as it is now.  If non-root creates the directory, it's
currently root-owned, so can only be used by root (or something with
appropriate caps).
Something with capabilities will be able to use it even if it is no
longer owned by root.

Patch
diff mbox series

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index b839dd1b459f..04f18402ef7c 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -1410,6 +1410,21 @@  static int configfs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode
 	else
 		ret = configfs_attach_item(parent_item, item, dentry, frag);
 
+	/* inherit uid/gid from process creating the directory */
+	if (!uid_eq(current_fsuid(), GLOBAL_ROOT_UID) ||
+	    !gid_eq(current_fsgid(), GLOBAL_ROOT_GID)) {
+		struct inode * inode = d_inode(dentry);
+		struct iattr ia = {
+			.ia_uid = current_fsuid(),
+			.ia_gid = current_fsgid(),
+			.ia_valid = ATTR_UID | ATTR_GID,
+		};
+		inode->i_uid = ia.ia_uid;
+		inode->i_gid = ia.ia_gid;
+		/* (note: the above manual assignments skip the permission checks) */
+		(void)configfs_setattr(dentry, &ia);
+	}
+
 	spin_lock(&configfs_dirent_lock);
 	sd->s_type &= ~CONFIGFS_USET_IN_MKDIR;
 	if (!ret)