From mboxrd@z Thu Jan 1 00:00:00 1970 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752617AbeAETYO (ORCPT + 1 other); Fri, 5 Jan 2018 14:24:14 -0500 Received: from mx2.suse.de ([195.135.220.15]:48689 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752121AbeAETYM (ORCPT ); Fri, 5 Jan 2018 14:24:12 -0500 Date: Fri, 5 Jan 2018 20:24:07 +0100 From: "Luis R. Rodriguez" To: Dongsu Park Cc: linux-kernel@vger.kernel.org, containers@lists.linux-foundation.org, Alban Crequy , "Eric W . Biederman" , Miklos Szeredi , Seth Forshee , Sargun Dhillon , linux-fsdevel@vger.kernel.org, Alexander Viro , "Luis R. Rodriguez" , Kees Cook Subject: Re: [PATCH 03/11] fs: Allow superblock owner to change ownership of inodes Message-ID: <20180105192407.GF22430@wotan.suse.de> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On Fri, Dec 22, 2017 at 03:32:27PM +0100, Dongsu Park wrote: > diff --git a/fs/attr.c b/fs/attr.c > index 12ffdb6f..bf8e94f3 100644 > --- a/fs/attr.c > +++ b/fs/attr.c > @@ -18,6 +18,30 @@ > #include > #include > > +static bool chown_ok(const struct inode *inode, kuid_t uid) > +{ > + if (uid_eq(current_fsuid(), inode->i_uid) && > + uid_eq(uid, inode->i_uid)) > + return true; > + if (capable_wrt_inode_uidgid(inode, CAP_CHOWN)) > + return true; > + if (ns_capable(inode->i_sb->s_user_ns, CAP_CHOWN)) > + return true; > + return false; > +} > + > +static bool chgrp_ok(const struct inode *inode, kgid_t gid) > +{ > + if (uid_eq(current_fsuid(), inode->i_uid) && > + (in_group_p(gid) || gid_eq(gid, inode->i_gid))) > + return true; > + if (capable_wrt_inode_uidgid(inode, CAP_CHOWN)) > + return true; > + if (ns_capable(inode->i_sb->s_user_ns, CAP_CHOWN)) > + return true; > + return false; > +} > + > /** > * setattr_prepare - check if attribute changes to a dentry are allowed > * @dentry: dentry to check > @@ -52,17 +76,11 @@ int setattr_prepare(struct dentry *dentry, struct iattr *attr) > goto kill_priv; > > /* Make sure a caller can chown. */ > - if ((ia_valid & ATTR_UID) && > - (!uid_eq(current_fsuid(), inode->i_uid) || > - !uid_eq(attr->ia_uid, inode->i_uid)) && > - !capable_wrt_inode_uidgid(inode, CAP_CHOWN)) > + if ((ia_valid & ATTR_UID) && !chown_ok(inode, attr->ia_uid)) > return -EPERM; I think this patch would read much better and easier to review if it was split up by first adding the helpers, and then extending them afterwards. > > /* Make sure caller can chgrp. */ > - if ((ia_valid & ATTR_GID) && > - (!uid_eq(current_fsuid(), inode->i_uid) || > - (!in_group_p(attr->ia_gid) && !gid_eq(attr->ia_gid, inode->i_gid))) && > - !capable_wrt_inode_uidgid(inode, CAP_CHOWN)) > + if ((ia_valid & ATTR_GID) && !chgrp_ok(inode, attr->ia_gid)) > return -EPERM; > > /* Make sure a caller can chmod. */ > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 31934cb9..9d50ec92 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -665,10 +665,17 @@ int proc_setattr(struct dentry *dentry, struct iattr *attr) > { > int error; > struct inode *inode = d_inode(dentry); > + struct user_namespace *s_user_ns; > > if (attr->ia_valid & ATTR_MODE) > return -EPERM; > > + /* Don't let anyone mess with weird proc files */ > + s_user_ns = inode->i_sb->s_user_ns; > + if (!kuid_has_mapping(s_user_ns, inode->i_uid) || > + !kgid_has_mapping(s_user_ns, inode->i_gid)) > + return -EPERM; > + > error = setattr_prepare(dentry, attr); > if (error) > return error; Are we sure proc is the only special one? How was it observed first that this was require for proc? Has anyone tried fuzzing by trying this op with a slew of other filesystems on all files? Luis