On Fri, Dec 22, 2017 at 03:32:28PM +0100, Dongsu Park wrote: > From: Seth Forshee <seth.forshee@xxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxx > Cc: linux-kernel@xxxxxxxxxxxxxxx > Cc: Alexander Viro <viro@xxxxxxxxxxxxxxxxxx> > Cc: Serge Hallyn <serge@xxxxxxxxxx> > Signed-off-by: Seth Forshee <seth.forshee@xxxxxxxxxxxxx> > Signed-off-by: Dongsu Park <dongsu@xxxxxxxxxx> > --- > 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
