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