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

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

[Alexey Dobriyan]

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.

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

[Darrick J. Wong]

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

--D

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

[Christoph Hellwig]

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