From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Serge E. Hallyn" Subject: Re: [PATCH 04/11] fs: Don't remove suid for CAP_FSETID for userns root Date: Fri, 22 Dec 2017 21:26:06 -0600 Message-ID: <20171223032606.GD6837__46265.0698995732$1513999477$gmane$org@mail.hallyn.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Dongsu Park Cc: Miklos Szeredi , containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Seth Forshee , Alban Crequy , "Eric W . Biederman" , Sargun Dhillon , linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Alexander Viro List-Id: containers.vger.kernel.org On Fri, Dec 22, 2017 at 03:32:28PM +0100, Dongsu Park wrote: > From: Seth Forshee > > Expand the check in should_remove_suid() to keep privileges for I realize this description came from Seth, but reading it now, 'Expand' seems wrong. Expanding a check brings to my mind making it stricter, not looser. How about 'Relax the check' ? > CAP_FSETID in s_user_ns rather than init_user_ns. > > Patch v4 is available: https://patchwork.kernel.org/patch/8944621/ > > --EWB Changed from ns_capable(sb->s_user_ns, ) to capable_wrt_inode_uidgid Why exactly? This is wrong, because capable_wrt_inode_uidgid() does a check against current_user_ns, not the inode->i_sb->s_user_ns > > Cc: linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > Cc: Alexander Viro > Cc: Serge Hallyn > Signed-off-by: Seth Forshee > Signed-off-by: Dongsu Park > --- > fs/inode.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/fs/inode.c b/fs/inode.c > index fd401028..6459a437 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -1749,7 +1749,8 @@ EXPORT_SYMBOL(touch_atime); > */ > int should_remove_suid(struct dentry *dentry) > { > - umode_t mode = d_inode(dentry)->i_mode; > + struct inode *inode = d_inode(dentry); > + umode_t mode = inode->i_mode; > int kill = 0; > > /* suid always must be killed */ > @@ -1763,7 +1764,8 @@ int should_remove_suid(struct dentry *dentry) > if (unlikely((mode & S_ISGID) && (mode & S_IXGRP))) > kill |= ATTR_KILL_SGID; > > - if (unlikely(kill && !capable(CAP_FSETID) && S_ISREG(mode))) > + if (unlikely(kill && !capable_wrt_inode_uidgid(inode, CAP_FSETID) && > + S_ISREG(mode))) > return kill; > > return 0; > -- > 2.13.6