On 07/24/2013 12:22 PM, Dwight Engen wrote:
> On Wed, 24 Jul 2013 10:27:43 -0400
> Brian Foster <bfoster@xxxxxxxxxx> wrote:
>
>> On 07/24/2013 12:53 AM, Dwight Engen wrote:
>>> We need to check that userspace callers can only truncate
>>> preallocated blocks from files they have write access to to prevent
>>> them from prematurley reclaiming blocks from another user. The
>>> internal reclaimer will not specify the XFS_EOF_FLAGS_PERM_CHECK
>>> flag, but userspace callers should.
>>>
>>> Add check for read-only filesystem to free eofblocks ioctl.
>>>
>>> Signed-off-by: Dwight Engen <dwight.engen@xxxxxxxxxx>
>>> ---
>>> fs/xfs/xfs_fs.h | 1 +
>>> fs/xfs/xfs_icache.c | 4 ++++
>>> fs/xfs/xfs_ioctl.c | 4 ++++
>>> 3 files changed, 9 insertions(+)
>>>
>>> diff --git a/fs/xfs/xfs_fs.h b/fs/xfs/xfs_fs.h
>>> index 7eb4a5e..aee4b12 100644
>>> --- a/fs/xfs/xfs_fs.h
>>> +++ b/fs/xfs/xfs_fs.h
>>> @@ -361,6 +361,7 @@ struct xfs_fs_eofblocks {
>>> #define XFS_EOF_FLAGS_GID (1 << 2) /* filter by gid
>>> */ #define XFS_EOF_FLAGS_PRID (1 << 3) /* filter by
>>> project id */ #define XFS_EOF_FLAGS_MINFILESIZE (1 << 4) /*
>>> filter by min file size */ +#define XFS_EOF_FLAGS_PERM_CHECK
>>> (1 << 5) /* check can write inode */ #define
>>> XFS_EOF_FLAGS_VALID \ (XFS_EOF_FLAGS_SYNC | \
>>> XFS_EOF_FLAGS_UID | \
>>
>> We're not updating the VALID definition, which means the ioctl() would
>> fail if the caller sets this flag. I find that a little confusing
>> since we're effectively enforcing it. Given that the new flag would be
>> exported, it might be a better idea to add it to the valid definition
>> even though we don't require the caller to set it.
>>
>> An alternative might be to duplicate the set of flags in xfs_icache.h
>> and not export this one at all, but I don't know it's really worth
>> that.
>
> I didn't put it in VALID because its really an internal flag, and we
> don't want userspace to think that we will honor them specifying it
> or not. ie. its not a valid bit for them to turn on. I agree it would be
> best not to export it though, how about if we move the definition to
> xfs_icache.h with a guard against someone accidentally adding a new
> duplicate bit in xfs_fs.h, like this:
>
> #define XFS_EOF_FLAGS_PERM_CHECK (1 << 5) /* check can write inode */
> #if XFS_EOF_FLAGS_PERM_CHECK & XFS_EOF_FLAGS_VALID
> #error "Internal XFS_EOF_FLAGS_PERM_CHECK duplicated bit from XFS_EOF_FLAGS_VALID"
> #endif
>
> Maybe since this is internal we should also start at 1<<31 to allow
> room for exported flags to grow?
Fair enough. It sounds reasonable to me to start a separate, internal
only set of flags. I'm not sure if you're suggesting to only use the
msb, or start at the msb in decreasing fashion.
I was going to suggest reserving the last byte for internal flags or
perhaps use a u64 for the internal eofb flags and reserve bits 32-63 for
internal use (or just create another variable). I also think a slight
name change is useful to differentiate the internal flags. Perhaps
XFS_KEOF_*?
Brian
>
>>> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
>>> index ed35584..823f2c0 100644
>>> --- a/fs/xfs/xfs_icache.c
>>> +++ b/fs/xfs/xfs_icache.c
>>> @@ -1247,6 +1247,10 @@ xfs_inode_free_eofblocks(
>>> if (!xfs_inode_match_id(ip, eofb))
>>> return 0;
>>>
>>> + if ((eofb->eof_flags & XFS_EOF_FLAGS_PERM_CHECK) &&
>>> + inode_permission(VFS_I(ip), MAY_WRITE))
>>> + return 0;
>>> +
>>> /* skip the inode if the file size is too small */
>>> if (eofb->eof_flags & XFS_EOF_FLAGS_MINFILESIZE &&
>>> XFS_ISIZE(ip) < eofb->eof_min_file_size)
>>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
>>> index ecab261..c7cb632 100644
>>> --- a/fs/xfs/xfs_ioctl.c
>>> +++ b/fs/xfs/xfs_ioctl.c
>>> @@ -1613,6 +1613,9 @@ xfs_file_ioctl(
>>> struct xfs_fs_eofblocks eofb;
>>> struct xfs_eofblocks keofb;
>>>
>>> + if (IS_RDONLY(inode))
>>> + return -XFS_ERROR(EROFS);
>>> +
>>> if (copy_from_user(&eofb, arg, sizeof(eofb)))
>>> return -XFS_ERROR(EFAULT);
>>>
>>> @@ -1630,6 +1633,7 @@ xfs_file_ioctl(
>>> if (error)
>>> return -error;
>>>
>>> + keofb.eof_flags |= XFS_EOF_FLAGS_PERM_CHECK;
>>
>> And perhaps this should also be in the new helper..?
>
> Okay, yep I can move this and the other struct xfs_fs_eofblocks checks
> you mentioned into the _from_user() helper.
>
>> Brian
>>
>>> return -xfs_icache_free_eofblocks(mp, &keofb);
>>> }
>>>
>>>
>>
>
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
