Re: "signed < sizeof()" bug in xfs_attr_shortform_verify() ?

[Christoph Hellwig <hch@infradead.org>]

On Tue, Aug 25, 2020 at 03:18:42PM -0700, Darrick J. Wong wrote:
> On Wed, Aug 26, 2020 at 12:10:48AM +0300, Alexey Dobriyan wrote:
> > xfs_attr_shortform_verify() contains the following code:
> > 
> > 
> > 	int64_t size = ifp->if_bytes;
> >         /*
> >          * Give up if the attribute is way too short.
> >          */
> >         if (size < sizeof(struct xfs_attr_sf_hdr))
> >                 return __this_address;
> > 
> > 
> > In general "if (signed < sizeof())" is wrong because of how type
> > promotions work. Such check won't catch small negative values.
> > 
> > I don't know XFS well enough to know if negative values were excluded
> > somewhere above the callchain, but maybe someone else does.
> 
> The initial allocations are always positive and the subsequent
> xfs_idata_realloc are checked to prevent if_bytes from going negative,
> but it does seem funny to me that if_bytes is declared int64_t...

Yes, the int64_t is weird.  IIRC the signednes was done to simplify
some arithmertics in the reallocation case, but that shouldn't really
leak out..