Hi, On Sat, Dec 23, 2017 at 4:26 AM, Serge E. Hallyn <se...@hallyn.com> wrote: > On Fri, Dec 22, 2017 at 03:32:28PM +0100, Dongsu Park wrote: >> From: Seth Forshee <seth.fors...@canonical.com> >> >> 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-fsde...@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org >> Cc: Alexander Viro <v...@zeniv.linux.org.uk> >> Cc: Serge Hallyn <se...@hallyn.com> >> Signed-off-by: Seth Forshee <seth.fors...@canonical.com> >> Signed-off-by: Dongsu Park <don...@kinvolk.io> >> --- >> 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
