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