Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757296AbdLWD0L (ORCPT ); Fri, 22 Dec 2017 22:26:11 -0500 Received: from h2.hallyn.com ([78.46.35.8]:53770 "EHLO h2.hallyn.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756885AbdLWD0I (ORCPT ); Fri, 22 Dec 2017 22:26:08 -0500 Date: Fri, 22 Dec 2017 21:26:06 -0600 From: "Serge E. Hallyn" 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 , Serge Hallyn Subject: Re: [PATCH 04/11] fs: Don't remove suid for CAP_FSETID for userns root Message-ID: <20171223032606.GD6837@mail.hallyn.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1837 Lines: 57 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@vger.kernel.org > Cc: linux-kernel@vger.kernel.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