Re: [PATCH] userns: Convert xfs to use kuid/kgid where appropriate

[Dave Chinner <david@fromorbit.com>]

On Thu, Jun 20, 2013 at 03:12:16PM -0400, Brian Foster wrote:
> On 06/20/2013 01:39 PM, Dwight Engen wrote:
> > On Thu, 20 Jun 2013 11:27:04 -0400
> > Brian Foster <bfoster@redhat.com> wrote:
> > 
> >> On 06/20/2013 09:54 AM, Dwight Engen wrote:
> >>> On Thu, 20 Jun 2013 10:13:41 +1000
> >>> Dave Chinner <david@fromorbit.com> wrote:
> >>>
> >>>> On Wed, Jun 19, 2013 at 11:09:48AM -0400, Dwight Engen wrote:
> ...
> >>
> >> Hi Dwight,
> >>
> >> If I understand correctly, the proposition is to turn
> >> XFS_EOF_FREE_EOFBLOCKS into administrator only functionality and run
> >> ns conversions on the inode uid/gid and associated eofb values for
> >> the ID filtering functionality.
> > 
> > Hi Brian, yeah that was the proposal :) I think there are really two
> > issues here. One is that the uid_t/gid_t may come from a userns so we
> > should be aware of that. Currently the ids passed in are used for
> > *filtering* so a malicious user can't do anything more than they
> > already can by not passing ids at all, but we should fix this so only
> > the intended files are affected. Second is that currently the ioctl
> > allows an unprivileged user to affect another user (as Eric pointed
> > out):
> > 
> >> I am little dubious about XFS_IOC_FREE_EOFBLOCKS allowing any
> >> user to affect any other user.  Your changes just seem to make
> >> it guaranteed that when called from a user namespace the wrong
> >> user will be affected.
> > 
> > I don't think the nsown_capability() I proposed is enough to take care
> > of this. Do you agree that if the caller is going to affect other
> > users, they should be CAP_SYS_ADMIN (or maybe CAP_FOWNER) in
> > init_user_ns?
> > 
> 
> Yeah, that's what I was getting at below by restricting "global" scans
> to admin privilege.

Project quota scans are global scans, so user-based initiation
through ioctls they should always be restricted to CAP_SYS_ADMIN.

> >> The latter sounds reasonable to me, though I'm not so sure about the
> >> CAP_SYS_ADMIN bit. For example, I think we'd expect a regular user to
> >> be able to run an eofblocks scan against files covered under his
> >> quota.
> >>
> >> Perhaps the right thing to do here is to restrict global (and project
> >> quota?) scans to CAP_SYS_ADMIN and uid/gid based scans to processes
> >> with the appropriate permissions (i.e., CAP_SYS_ADMIN, matching
> >> uid/gid or CAP_FOWNER). Thoughts?
> > 
> > That sounds good to me. Maybe for a regular user the appropriate
> > permission check (at the top of xfs_inode_free_eofblocks()) could be
> > something like:
> > 
> 
> I think the various capability/permission checks should be in the ioctl
> code.

Yes, the cap/perm checks should be done before anything else in
the ioctl.

> This would still allow use cases such as the pending code I have that
> invokes an eofblocks scan on write() failure due to EDQUOT/ENOSPC in the
> case of project or user/group quotas.

Right, we have to ensure this can occur without namespace
restriction, because ENOSPC is not something that is bound by user
namespaces.

> I suspect adding the namespace
> conversion stuff wouldn't break the typical user/group quota case, but

For EDQUOT, no, but for a global ENOSPC scan I think it could cause
problems.

> we'd still require the ability to run a project quota scan from a
> particular user context.  I think the combined check you have
> above would break that.

Yup, that still needs to work, as does the background scanner which
should not be subject to any restrictions at all ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs