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

["Darrick J. Wong" <darrick.wong@oracle.com>]

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