Quoting Marian Marinov ([email protected]): > On 04/30/2014 01:02 AM, Serge Hallyn wrote: > >Quoting Marian Marinov ([email protected]): > >>On 04/29/2014 09:52 PM, Serge Hallyn wrote: > >>>Quoting Theodore Ts'o ([email protected]): > >>>>On Tue, Apr 29, 2014 at 04:49:14PM +0300, Marian Marinov wrote: > >>>>> > >>>>>I'm proposing a fix to this, by replacing the > >>>>>capable(CAP_LINUX_IMMUTABLE) > >>>>>check with ns_capable(current_cred()->user_ns, CAP_LINUX_IMMUTABLE). > >>>> > >>>>Um, wouldn't it be better to simply fix the capable() function? > >>>> > >>>>/** > >>>> * capable - Determine if the current task has a superior capability in > >>>> effect > >>>> * @cap: The capability to be tested for > >>>> * > >>>> * Return true if the current task has the given superior capability > >>>> currently > >>>> * available for use, false if not. > >>>> * > >>>> * This sets PF_SUPERPRIV on the task if the capability is available on > >>>> the > >>>> * assumption that it's about to be used. > >>>> */ > >>>>bool capable(int cap) > >>>>{ > >>>> return ns_capable(&init_user_ns, cap); > >>>>} > >>>>EXPORT_SYMBOL(capable); > >>>> > >>>>The documentation states that it is for "the current task", and I > >>>>can't imagine any use case, where user namespaces are in effect, where > >>>>using init_user_ns would ever make sense. > >>> > >>>the init_user_ns represents the user_ns owning the object, not the > >>>subject. > >>> > >>>The patch by Marian is wrong. Anyone can do 'clone(CLONE_NEWUSER)', > >>>setuid(0), execve, and end up satisfying > >>>'ns_capable(current_cred()->userns, > >>>CAP_SYS_IMMUTABLE)' by definition. > >>> > >>>So NACK to that particular patch. I'm not sure, but IIUC it should be > >>>safe to check against the userns owning the inode? > >>> > >> > >>So what you are proposing is to replace 'ns_capable(current_cred()->userns, > >>CAP_SYS_IMMUTABLE)' with > >>'inode_capable(inode, CAP_SYS_IMMUTABLE)' ? > >> > >>I agree that this is more sane. > > > >Right, and I think the two operations you're looking at seem sane > >to allow. > > If you are ok with this patch, I will fix all file systems and send patches.
Sounds good, thanks. > Signed-off-by: Marian Marinov <[email protected]> Acked-by: Serge E. Hallyn <[email protected]> > --- > fs/ext4/ioctl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c > index d011b69..9418634 100644 > --- a/fs/ext4/ioctl.c > +++ b/fs/ext4/ioctl.c > @@ -265,7 +265,7 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, > unsigned long arg) > * This test looks nicer. Thanks to Pauline Middelink > */ > if ((flags ^ oldflags) & (EXT4_APPEND_FL | > EXT4_IMMUTABLE_FL)) { > - if (!capable(CAP_LINUX_IMMUTABLE)) > + if (!inode_capable(inode, CAP_LINUX_IMMUTABLE)) > goto flags_out; > } > > --- > 1.8.4 > > Marian > > > > > >thanks, > >-serge > > > > > -- > Marian Marinov > Founder & CEO of 1H Ltd. > Jabber/GTalk: [email protected] > ICQ: 7556201 > Mobile: +359 886 660 270 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
