| Message ID | 20210123205516.2738060-1-zenczykowski@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series |
|
| Related | show |
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
> > 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.
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)