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

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

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