Quoting Marian Marinov (mm@xxxxxx):
> On 04/30/2014 01:02 AM, Serge Hallyn wrote:
> >Quoting Marian Marinov (mm@xxxxxx):
> >>On 04/29/2014 09:52 PM, Serge Hallyn wrote:
> >>>Quoting Theodore Ts'o (tytso@xxxxxxx):
> >>>>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 <mm@xxxxxxxx>
Acked-by: Serge E. Hallyn <serge.hallyn@xxxxxxxxxx>
> ---
> 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: hackman@xxxxxxxxxx
> ICQ: 7556201
> Mobile: +359 886 660 270
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/containers
