On 06/20/2013 01:39 PM, Dwight Engen wrote: > On Thu, 20 Jun 2013 11:27:04 -0400 > Brian Foster <bfoster@xxxxxxxxxx> wrote: > >> On 06/20/2013 09:54 AM, Dwight Engen wrote: >>> On Thu, 20 Jun 2013 10:13:41 +1000 >>> Dave Chinner <david@xxxxxxxxxxxxx> 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. >> 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. xfs_icache_free_eofblocks() and below are internal interfaces where these checks are probably not relevant. I actually have code lying around that creates an internal structure for that code, similar but separate from the xfs_eofblocks structure. > if (!capable(CAP_SYS_ADMIN) && > !uid_eq(VFS_I(ip)->i_uid, current_fsuid()) && > !in_group_p(VFS_I(ip)->i_gid)) > return 0; > This is a little confusing (and pardon me, I'm a bit new to the namespace work). What might be a bit more clear is to do the capability checks against the EOFBLOCKS command flags in xfs_file_ioctl() and return an appropriate error if permission is not granted for the requested type of scan (i.e., a regular user doing a global or non-id matching scan). Then restrict the changes in xfs_icache_free_eofblocks() to just dealing with the namespace conversions. 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. I suspect adding the namespace conversion stuff wouldn't break the typical user/group quota case, but 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. Brian > This has the drawback that the caller won't know if they supplied a > uid/gid in eofblocks that won't actually get cleared, so maybe we > want to validate a uid/gid in eofblocks after its copy_from_user()ed > in instead? Also, I'm not sure if this is the same as "under his quota" > and how it plays with project quotas. > >> Brian _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
