Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754345AbdLWMiy (ORCPT ); Sat, 23 Dec 2017 07:38:54 -0500 Received: from mail-yb0-f196.google.com ([209.85.213.196]:33816 "EHLO mail-yb0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752674AbdLWMiu (ORCPT ); Sat, 23 Dec 2017 07:38:50 -0500 X-Google-Smtp-Source: ACJfBotR/ND4RcClhauqbBzbm7I9DZEk0ihixp0nG4vNhOhdJu4X+PtP+r9IUm2Rp1x3n+yCLAfvaJJURQqq70LGua4= MIME-Version: 1.0 In-Reply-To: <20171223032606.GD6837@mail.hallyn.com> References: <20171223032606.GD6837@mail.hallyn.com> From: Dongsu Park Date: Sat, 23 Dec 2017 13:38:45 +0100 Message-ID: Subject: Re: [PATCH 04/11] fs: Don't remove suid for CAP_FSETID for userns root To: "Serge E. Hallyn" Cc: LKML , Linux Containers , Alban Crequy , "Eric W . Biederman" , Miklos Szeredi , Seth Forshee , Sargun Dhillon , linux-fsdevel@vger.kernel.org, Alexander Viro Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2490 Lines: 72 Hi, On Sat, Dec 23, 2017 at 4:26 AM, Serge E. Hallyn wrote: > 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' ? Makes sense. Will do. >> 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 Ah. I see. I suppose it was changed probably for the privileged_wrt_inode_uidgid() called by capable_wrt_inode_uidgid(). But as you pointed out, that checks against current_user_ns, which is wrong. I would just create another wrapper like capable_userns_wrt_inode_uidgid(), which takes an additional parameter of (struct user_namespace *), to be able to check for both ns_capable() and privileged_wrt_inode_uidgid(). Thanks, Dongsu >> 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