[PATCH v5 6/7] xfs: add permission check to free eofblocks ioctl

[PATCH v5 6/7] xfs: add permission check to free eofblocks ioctl

[Dwight Engen]

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@oracle.com>
---
 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 |	\
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;
 		return -xfs_icache_free_eofblocks(mp, &keofb);
 	}
 
-- 
1.8.1.4

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

Re: [PATCH v5 6/7] xfs: add permission check to free eofblocks ioctl

[Brian Foster]

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@oracle.com>
> ---
>  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.

> 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..?

Brian

>  		return -xfs_icache_free_eofblocks(mp, &keofb);
>  	}
>  
> 

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

Re: [PATCH v5 6/7] xfs: add permission check to free eofblocks ioctl

[Dwight Engen]

On Wed, 24 Jul 2013 10:27:43 -0400
Brian Foster <bfoster@redhat.com> 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@oracle.com>
> > ---
> >  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?

> > 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@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v5 6/7] xfs: add permission check to free eofblocks ioctl

[Brian Foster]

On 07/24/2013 12:22 PM, Dwight Engen wrote:
> On Wed, 24 Jul 2013 10:27:43 -0400
> Brian Foster <bfoster@redhat.com> 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@oracle.com>
>>> ---
>>>  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@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs