[PATCH] xfs: fix boundary test in xfs_attr_shortform_verify

[PATCH] xfs: fix boundary test in xfs_attr_shortform_verify

[Eric Sandeen]

The boundary test for the fixed-offset parts of xfs_attr_sf_entry
in xfs_attr_shortform_verify is off by one.  endp is the address
just past the end of the valid data; to check the last byte of
a structure at offset of size "size" we must subtract one.
(i.e. for an object at offset 10, size 4, last byte is 13 not 14).

This can be shown by:

# touch file
# setfattr -n root.a file

and subsequent verifications will fail when it's reread from disk.

This only matters for a last attribute which has a single-byte name
and no value, otherwise the combination of namelen & valuelen will
push endp out and this test won't fail.

Fixes: 1e1bbd8e7ee06 ("xfs: create structure verifier function for shortform xattrs")
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 8623c815164a..a0cf22f0c904 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -1037,7 +1037,7 @@ xfs_attr_shortform_verify(
 		 * Check the fixed-offset parts of the structure are
 		 * within the data buffer.
 		 */
-		if (((char *)sfep + sizeof(*sfep)) >= endp)
+		if (((char *)sfep + sizeof(*sfep)-1) >= endp)
 			return __this_address;
 
 		/* Don't allow names with known bad length. */

Re: [PATCH] xfs: fix boundary test in xfs_attr_shortform_verify

[Darrick J. Wong]

On Tue, Aug 25, 2020 at 03:25:29PM -0500, Eric Sandeen wrote:
> The boundary test for the fixed-offset parts of xfs_attr_sf_entry
> in xfs_attr_shortform_verify is off by one.  endp is the address
> just past the end of the valid data; to check the last byte of
> a structure at offset of size "size" we must subtract one.
> (i.e. for an object at offset 10, size 4, last byte is 13 not 14).
> 
> This can be shown by:
> 
> # touch file
> # setfattr -n root.a file
> 
> and subsequent verifications will fail when it's reread from disk.
> 
> This only matters for a last attribute which has a single-byte name
> and no value, otherwise the combination of namelen & valuelen will
> push endp out and this test won't fail.
> 
> Fixes: 1e1bbd8e7ee06 ("xfs: create structure verifier function for shortform xattrs")
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

NGGHGHGHG array[1]s that are actually array[0]...

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 8623c815164a..a0cf22f0c904 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -1037,7 +1037,7 @@ xfs_attr_shortform_verify(
>  		 * Check the fixed-offset parts of the structure are
>  		 * within the data buffer.
>  		 */
> -		if (((char *)sfep + sizeof(*sfep)) >= endp)
> +		if (((char *)sfep + sizeof(*sfep)-1) >= endp)
>  			return __this_address;
>  
>  		/* Don't allow names with known bad length. */
> 

Re: [PATCH] xfs: fix boundary test in xfs_attr_shortform_verify

[Dave Chinner]

On Tue, Aug 25, 2020 at 03:25:29PM -0500, Eric Sandeen wrote:
> The boundary test for the fixed-offset parts of xfs_attr_sf_entry
> in xfs_attr_shortform_verify is off by one.  endp is the address
> just past the end of the valid data; to check the last byte of
> a structure at offset of size "size" we must subtract one.
> (i.e. for an object at offset 10, size 4, last byte is 13 not 14).
> 
> This can be shown by:
> 
> # touch file
> # setfattr -n root.a file
> 
> and subsequent verifications will fail when it's reread from disk.
> 
> This only matters for a last attribute which has a single-byte name
> and no value, otherwise the combination of namelen & valuelen will
> push endp out and this test won't fail.
> 
> Fixes: 1e1bbd8e7ee06 ("xfs: create structure verifier function for shortform xattrs")
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 8623c815164a..a0cf22f0c904 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -1037,7 +1037,7 @@ xfs_attr_shortform_verify(
>  		 * Check the fixed-offset parts of the structure are
>  		 * within the data buffer.
>  		 */
> -		if (((char *)sfep + sizeof(*sfep)) >= endp)
> +		if (((char *)sfep + sizeof(*sfep)-1) >= endp)

whitespace? And a comment explaining the magic "- 1" would be nice.

Did you audit the code for other occurrences of this same problem?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: fix boundary test in xfs_attr_shortform_verify

[Eric Sandeen]

On 8/25/20 5:41 PM, Dave Chinner wrote:
> On Tue, Aug 25, 2020 at 03:25:29PM -0500, Eric Sandeen wrote:
>> The boundary test for the fixed-offset parts of xfs_attr_sf_entry
>> in xfs_attr_shortform_verify is off by one.  endp is the address
>> just past the end of the valid data; to check the last byte of
>> a structure at offset of size "size" we must subtract one.
>> (i.e. for an object at offset 10, size 4, last byte is 13 not 14).
>>
>> This can be shown by:
>>
>> # touch file
>> # setfattr -n root.a file
>>
>> and subsequent verifications will fail when it's reread from disk.
>>
>> This only matters for a last attribute which has a single-byte name
>> and no value, otherwise the combination of namelen & valuelen will
>> push endp out and this test won't fail.
>>
>> Fixes: 1e1bbd8e7ee06 ("xfs: create structure verifier function for shortform xattrs")
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
>> index 8623c815164a..a0cf22f0c904 100644
>> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
>> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
>> @@ -1037,7 +1037,7 @@ xfs_attr_shortform_verify(
>>  		 * Check the fixed-offset parts of the structure are
>>  		 * within the data buffer.
>>  		 */
>> -		if (((char *)sfep + sizeof(*sfep)) >= endp)
>> +		if (((char *)sfep + sizeof(*sfep)-1) >= endp)
> 
> whitespace? And a comment explaining the magic "- 1" would be nice.

I was following the whitespace example in the various similar macros
i.e. XFS_ATTR_SF_ENTSIZE but if people want spaces that's fine by me.  :)

ditto for degree of commenting on magical -1's; on the one hand it's a
common usage.  On the other hand, we often get it wrong so a comment
probably would help.

> Did you audit the code for other occurrences of this same problem?

No.  I should do that, good point.  Now I do wonder if

                /*
                 * Check that the variable-length part of the structure is
                 * within the data buffer.  The next entry starts after the
                 * name component, so nextentry is an acceptable test.
                 */
                next_sfep = XFS_ATTR_SF_NEXTENTRY(sfep);
                if ((char *)next_sfep > endp)
                        return __this_address;

should be >= but I'll have to unravel all the macros to see.  In that case
though the missing "=" makes it too lenient not too strict, at least.

In general though, auditing for proper "offset + length [-1] >[=] $THING"

where $THING may be last byte or one-past-last-byte is a few days of work, because
we have no real consistency about how we do these things and it requires lots of
code-reading to get all the context and knowledge of how we're counting.

Not really trying to make excuses but I did want to get the demonstrable
flaw fixed fairly quickly.	

Thanks though, these are good points.

-Eric

> Cheers,
> 
> Dave.
> 

Re: [PATCH] xfs: fix boundary test in xfs_attr_shortform_verify

[Darrick J. Wong]

On Wed, Aug 26, 2020 at 09:32:13AM -0500, Eric Sandeen wrote:
> On 8/25/20 5:41 PM, Dave Chinner wrote:
> > On Tue, Aug 25, 2020 at 03:25:29PM -0500, Eric Sandeen wrote:
> >> The boundary test for the fixed-offset parts of xfs_attr_sf_entry
> >> in xfs_attr_shortform_verify is off by one.  endp is the address
> >> just past the end of the valid data; to check the last byte of
> >> a structure at offset of size "size" we must subtract one.
> >> (i.e. for an object at offset 10, size 4, last byte is 13 not 14).
> >>
> >> This can be shown by:
> >>
> >> # touch file
> >> # setfattr -n root.a file
> >>
> >> and subsequent verifications will fail when it's reread from disk.
> >>
> >> This only matters for a last attribute which has a single-byte name
> >> and no value, otherwise the combination of namelen & valuelen will
> >> push endp out and this test won't fail.
> >>
> >> Fixes: 1e1bbd8e7ee06 ("xfs: create structure verifier function for shortform xattrs")
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> >>
> >> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> >> index 8623c815164a..a0cf22f0c904 100644
> >> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> >> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> >> @@ -1037,7 +1037,7 @@ xfs_attr_shortform_verify(
> >>  		 * Check the fixed-offset parts of the structure are
> >>  		 * within the data buffer.
> >>  		 */
> >> -		if (((char *)sfep + sizeof(*sfep)) >= endp)
> >> +		if (((char *)sfep + sizeof(*sfep)-1) >= endp)
> > 
> > whitespace? And a comment explaining the magic "- 1" would be nice.
> 
> I was following the whitespace example in the various similar macros
> i.e. XFS_ATTR_SF_ENTSIZE but if people want spaces that's fine by me.  :)
> 
> ditto for degree of commenting on magical -1's; on the one hand it's a
> common usage.  On the other hand, we often get it wrong so a comment
> probably would help.
> 
> > Did you audit the code for other occurrences of this same problem?

TBH I think this ought to be fixed by changing the declaration of
xfs_attr_sf_entry.nameval to "uint8_t nameval[]" and using more modern
fugly macros like struct_sizeof() to calculate the entry sizes without
us all having to remember to subtract one from the struct size.

> No.  I should do that, good point.  Now I do wonder if
> 
>                 /*
>                  * Check that the variable-length part of the structure is
>                  * within the data buffer.  The next entry starts after the
>                  * name component, so nextentry is an acceptable test.
>                  */
>                 next_sfep = XFS_ATTR_SF_NEXTENTRY(sfep);
>                 if ((char *)next_sfep > endp)
>                         return __this_address;
> 
> should be >= but I'll have to unravel all the macros to see.  In that case
> though the missing "=" makes it too lenient not too strict, at least.

*endp points to the first byte after the end of the buffer, because it
is defined as (*sfp + size).  The end of the last *sfep in the sf attr
struct is supposed to coincide with the end of the buffer, so changing
this to >= is not correct.

--D

> In general though, auditing for proper "offset + length [-1] >[=] $THING"
> 
> where $THING may be last byte or one-past-last-byte is a few days of work, because
> we have no real consistency about how we do these things and it requires lots of
> code-reading to get all the context and knowledge of how we're counting.
> 
> Not really trying to make excuses but I did want to get the demonstrable
> flaw fixed fairly quickly.	
> 
> Thanks though, these are good points.
> 
> -Eric
> 
> > Cheers,
> > 
> > Dave.
> > 

Re: [PATCH] xfs: fix boundary test in xfs_attr_shortform_verify

[Eric Sandeen]

On 8/26/20 10:13 AM, Darrick J. Wong wrote:

...

> TBH I think this ought to be fixed by changing the declaration of
> xfs_attr_sf_entry.nameval to "uint8_t nameval[]" and using more modern
> fugly macros like struct_sizeof() to calculate the entry sizes without
> us all having to remember to subtract one from the struct size.

Fair, but I think that in the interest of time we should fix it up with a -1
which is consistent with the other bits of attr code first, then this can all
be cleaned up by making it a [] not [1], dropping the magical -1, turning
the macros into functions ala dir2, etc.

Sound ok?

>> No.  I should do that, good point.  Now I do wonder if
>>
>>                 /*
>>                  * Check that the variable-length part of the structure is
>>                  * within the data buffer.  The next entry starts after the
>>                  * name component, so nextentry is an acceptable test.
>>                  */
>>                 next_sfep = XFS_ATTR_SF_NEXTENTRY(sfep);
>>                 if ((char *)next_sfep > endp)
>>                         return __this_address;
>>
>> should be >= but I'll have to unravel all the macros to see.  In that case
>> though the missing "=" makes it too lenient not too strict, at least.
> 
> *endp points to the first byte after the end of the buffer, because it
> is defined as (*sfp + size).  The end of the last *sfep in the sf attr
> struct is supposed to coincide with the end of the buffer, so changing
> this to >= is not correct.

Let me think on that a little more ;)

-Eric

Re: [PATCH] xfs: fix boundary test in xfs_attr_shortform_verify

[Darrick J. Wong]

On Wed, Aug 26, 2020 at 10:39:26AM -0500, Eric Sandeen wrote:
> On 8/26/20 10:13 AM, Darrick J. Wong wrote:
> 
> ...
> 
> > TBH I think this ought to be fixed by changing the declaration of
> > xfs_attr_sf_entry.nameval to "uint8_t nameval[]" and using more modern
> > fugly macros like struct_sizeof() to calculate the entry sizes without
> > us all having to remember to subtract one from the struct size.
> 
> Fair, but I think that in the interest of time we should fix it up with a -1
> which is consistent with the other bits of attr code first, then this can all
> be cleaned up by making it a [] not [1], dropping the magical -1, turning
> the macros into functions ala dir2, etc.
> 
> Sound ok?

Yes.  sorry, I thought I was suggesting that we start with the quick -1
fix and move on to fixing the struct, but ENOCOFFEE and LPC sessions
start too early... :(

--d

> >> No.  I should do that, good point.  Now I do wonder if
> >>
> >>                 /*
> >>                  * Check that the variable-length part of the structure is
> >>                  * within the data buffer.  The next entry starts after the
> >>                  * name component, so nextentry is an acceptable test.
> >>                  */
> >>                 next_sfep = XFS_ATTR_SF_NEXTENTRY(sfep);
> >>                 if ((char *)next_sfep > endp)
> >>                         return __this_address;
> >>
> >> should be >= but I'll have to unravel all the macros to see.  In that case
> >> though the missing "=" makes it too lenient not too strict, at least.
> > 
> > *endp points to the first byte after the end of the buffer, because it
> > is defined as (*sfp + size).  The end of the last *sfep in the sf attr
> > struct is supposed to coincide with the end of the buffer, so changing
> > this to >= is not correct.
> 
> Let me think on that a little more ;)
> 
> -Eric

[PATCH V2] xfs: fix boundary test in xfs_attr_shortform_verify

[Eric Sandeen]

The boundary test for the fixed-offset parts of xfs_attr_sf_entry in
xfs_attr_shortform_verify is off by one, because the variable array
at the end is defined as nameval[1] not nameval[].
Hence we need to subtract 1 from the calculation.

This can be shown by:

# touch file
# setfattr -n root.a file

and verifications will fail when it's written to disk.

This only matters for a last attribute which has a single-byte name
and no value, otherwise the combination of namelen & valuelen will
push endp further out and this test won't fail.

Fixes: 1e1bbd8e7ee06 ("xfs: create structure verifier function for shortform xattrs")
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: Add whitespace and comments, tweak commit log.

Note: as Darrick points out, this should be made consistent w/ the dir2 arrays
by making the nameval variable size array [] not [1], and then we can lose all
the -1 magic sprinkled around.  At that time we should probably also make the
macros into proper functions, as was done for dir2.

For now, this is just the least invasive fixup for the problem at hand.

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 8623c815164a..383b08f2ac61 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -1036,8 +1036,10 @@ xfs_attr_shortform_verify(
 		 * struct xfs_attr_sf_entry has a variable length.
 		 * Check the fixed-offset parts of the structure are
 		 * within the data buffer.
+		 * xfs_attr_sf_entry is defined with a 1-byte variable
+		 * array at the end, so we must subtract that off.
 		 */
-		if (((char *)sfep + sizeof(*sfep)) >= endp)
+		if (((char *)sfep + sizeof(*sfep) - 1) >= endp)
 			return __this_address;
 
 		/* Don't allow names with known bad length. */

Re: [PATCH V2] xfs: fix boundary test in xfs_attr_shortform_verify

[Darrick J. Wong]

On Wed, Aug 26, 2020 at 11:19:54AM -0500, Eric Sandeen wrote:
> The boundary test for the fixed-offset parts of xfs_attr_sf_entry in
> xfs_attr_shortform_verify is off by one, because the variable array
> at the end is defined as nameval[1] not nameval[].
> Hence we need to subtract 1 from the calculation.
> 
> This can be shown by:
> 
> # touch file
> # setfattr -n root.a file
> 
> and verifications will fail when it's written to disk.
> 
> This only matters for a last attribute which has a single-byte name
> and no value, otherwise the combination of namelen & valuelen will
> push endp further out and this test won't fail.
> 
> Fixes: 1e1bbd8e7ee06 ("xfs: create structure verifier function for shortform xattrs")
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks ok.

From whom should I be expecting a test case?

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> 
> V2: Add whitespace and comments, tweak commit log.
> 
> Note: as Darrick points out, this should be made consistent w/ the dir2 arrays
> by making the nameval variable size array [] not [1], and then we can lose all
> the -1 magic sprinkled around.  At that time we should probably also make the
> macros into proper functions, as was done for dir2.
> 
> For now, this is just the least invasive fixup for the problem at hand.
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 8623c815164a..383b08f2ac61 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -1036,8 +1036,10 @@ xfs_attr_shortform_verify(
>  		 * struct xfs_attr_sf_entry has a variable length.
>  		 * Check the fixed-offset parts of the structure are
>  		 * within the data buffer.
> +		 * xfs_attr_sf_entry is defined with a 1-byte variable
> +		 * array at the end, so we must subtract that off.
>  		 */
> -		if (((char *)sfep + sizeof(*sfep)) >= endp)
> +		if (((char *)sfep + sizeof(*sfep) - 1) >= endp)
>  			return __this_address;
>  
>  		/* Don't allow names with known bad length. */
> 

Re: [PATCH V2] xfs: fix boundary test in xfs_attr_shortform_verify

[Eric Sandeen]

On 8/26/20 11:44 AM, Darrick J. Wong wrote:
> On Wed, Aug 26, 2020 at 11:19:54AM -0500, Eric Sandeen wrote:
>> The boundary test for the fixed-offset parts of xfs_attr_sf_entry in
>> xfs_attr_shortform_verify is off by one, because the variable array
>> at the end is defined as nameval[1] not nameval[].
>> Hence we need to subtract 1 from the calculation.
>>
>> This can be shown by:
>>
>> # touch file
>> # setfattr -n root.a file
>>
>> and verifications will fail when it's written to disk.
>>
>> This only matters for a last attribute which has a single-byte name
>> and no value, otherwise the combination of namelen & valuelen will
>> push endp further out and this test won't fail.
>>
>> Fixes: 1e1bbd8e7ee06 ("xfs: create structure verifier function for shortform xattrs")
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> 
> Looks ok.

thanks
 
> From whom should I be expecting a test case?

from me or a TBD delegate.  Will follow up soon.

-Eric

> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

Re: [PATCH] xfs: fix boundary test in xfs_attr_shortform_verify

[Christoph Hellwig]

On Wed, Aug 26, 2020 at 08:13:00AM -0700, Darrick J. Wong wrote:
> > ditto for degree of commenting on magical -1's; on the one hand it's a
> > common usage.  On the other hand, we often get it wrong so a comment
> > probably would help.
> > 
> > > Did you audit the code for other occurrences of this same problem?
> 
> TBH I think this ought to be fixed by changing the declaration of
> xfs_attr_sf_entry.nameval to "uint8_t nameval[]" and using more modern
> fugly macros like struct_sizeof() to calculate the entry sizes without
> us all having to remember to subtract one from the struct size.

Agreed that we absoutely need to do that.  It might be worth to
have the "simple" fix as a backportable small patch first, though.

Re: [PATCH V2] xfs: fix boundary test in xfs_attr_shortform_verify

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Can you follow up with the struct definition fix ASAP?

Re: [PATCH V2] xfs: fix boundary test in xfs_attr_shortform_verify

[Eric Sandeen]

On 8/27/20 3:12 AM, Christoph Hellwig wrote:
> Looks good,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> Can you follow up with the struct definition fix ASAP?

Working on delegating that task, yes.

-Eric

Re: [PATCH V2] xfs: fix boundary test in xfs_attr_shortform_verify

[Pavel Reichl]

> From whom should I be expecting a test case?
Hi,

I took the task, I hope it will be done soon - I'll do my best.

Pavel