Re: [Intel-gfx] [PATCH v7 1/8] overflow: Move and add few utility macros into overflow

[Andrzej Hajda]

On 22.08.2022 16:05, Andrzej Hajda wrote:
> On 18.08.2022 02:12, Kees Cook wrote:
> > On Thu, Aug 18, 2022 at 01:07:29AM +0200, Andi Shyti wrote:
> > > Hi Kees,
> > >
> > > would you mind taking a look at this patch?
> >
> > Hi! Thanks for the heads-up!
> >
> > > Thanks,
> > > Andi
> > >
> > > On Tue, Aug 16, 2022 at 06:35:18PM +0900, Gwan-gyeong Mun wrote:
> > > > It moves overflows_type utility macro into overflow header from 
> > > > i915_utils
> > > > header. The overflows_type can be used to catch the truncation 
> > > > between data
> > > > types. And it adds safe_conversion() macro which performs a type 
> > > > conversion
> > > > (cast) of an source value into a new variable, checking that the
> > > > destination is large enough to hold the source value. And the 
> > > > functionality
> > > > of overflows_type has been improved to handle the signbit.
> > > > The is_unsigned_type macro has been added to check the sign bit of the
> > > > built-in type.
> > > >
> > > > v3: Add is_type_unsigned() macro (Mauro)
> > > >      Modify overflows_type() macro to consider signed data types 
> > > > (Mauro)
> > > >      Fix the problem that safe_conversion() macro always returns true
> > > > v4: Fix kernel-doc markups
> > > > v6: Move macro addition location so that it can be used by other 
> > > > than drm
> > > >      subsystem (Jani, Mauro, Andi)
> > > >      Change is_type_unsigned to is_unsigned_type to have the same 
> > > > name form
> > > >      as is_signed_type macro
> > > >
> > > > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@xxxxxxxxx>
> > > > Cc: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
> > > > Cc: Matthew Auld <matthew.auld@xxxxxxxxx>
> > > > Cc: Nirmoy Das <nirmoy.das@xxxxxxxxx>
> > > > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > > > Cc: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
> > > > Reviewed-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx> (v5)
> > > > ---

(...)

> > > > +
> > > > +/**
> > > > + * overflows_type - helper for checking the truncation between data 
> > > > types
> > > > + * @x: Source for overflow type comparison
> > > > + * @T: Destination for overflow type comparison
> > > > + *
> > > > + * It compares the values and size of each data type between the 
> > > > first and
> > > > + * second argument to check whether truncation can occur when 
> > > > assigning the
> > > > + * first argument to the variable of the second argument.
> > > > + * Source and Destination can be used with or without sign bit.
> > > > + * Composite data structures such as union and structure are not 
> > > > considered.
> > > > + * Enum data types are not considered.
> > > > + * Floating point data types are not considered.
> > > > + *
> > > > + * Returns:
> > > > + * True if truncation can occur, false otherwise.
> > > > + */
> > > > +#define overflows_type(x, T) \
> > > > +    (is_unsigned_type(x) ? \
> > > > +        is_unsigned_type(T) ? \
> > > > +            (sizeof(x) > sizeof(T) && (x) >> BITS_PER_TYPE(T)) ? 1 
> > > > : 0 \
> > > > +            : (sizeof(x) >= sizeof(T) && (x) >> (BITS_PER_TYPE(T) - 
> > > > 1)) ? 1 : 0 \
> > > > +    : is_unsigned_type(T) ? \
> > > > +        ((x) < 0) ? 1 : (sizeof(x) > sizeof(T) && (x) >> 
> > > > BITS_PER_TYPE(T)) ? 1 : 0 \
> > > > +        : (sizeof(x) > sizeof(T)) ? \
> > > > +            ((x) < 0) ? (((x) * -1) >> BITS_PER_TYPE(T)) ? 1 : 0 \
> > > > +                : ((x) >> BITS_PER_TYPE(T)) ? 1 : 0 \
> > > > +            : 0)
> >
> > Like the other, I'd much rather this was rephrased in terms of the
> > existing macros (e.g. type_min()/type_max().)
>
>
> I am not sure how it could be rephrased with type_(min|max), but I guess 
> the shortest could be sth like:
>
> #define overflows_type(x, T) __builtin_add_overflow_p(x, (typeof(T))0, 
> (typeof(T))0)

Except this macro is available since gcc 7, but apparently 
__builtin_add_overflow is supported since gcc 5, which should be OK:
#define overflows_type(x, T) ({ typeof(T) r = 0; 
__builtin_add_overflow_p((x), r, r); })

Regards
Andrzej

> Regards
> Andrzej
>
>
> > > > +
> > > > +/**
> > > > + * safe_conversion - perform a type conversion (cast) of an source 
> > > > value into
> > > > + * a new variable, checking that the destination is large enough to 
> > > > hold the
> > > > + * source value.
> > > > + * @ptr: Destination pointer address
> > > > + * @value: Source value
> > > > + *
> > > > + * Returns:
> > > > + * If the value would overflow the destination, it returns false.
> > > > + */
> > > > +#define safe_conversion(ptr, value) ({ \
> > > > +    typeof(value) __v = (value); \
> > > > +    typeof(ptr) __ptr = (ptr); \
> > > > +    overflows_type(__v, *__ptr) ? 0 : ((*__ptr = 
> > > > (typeof(*__ptr))__v), 1); \
> > > > +})
> >
> > I try to avoid "safe" as an adjective for interface names, since it
> > doesn't really answer "safe from what?" This looks more like "assign, but
> > zero when out of bounds". And it can be built from existing macros here:
> >
> >     if (check_add_overflow(0, value, ptr))
> >         *ptr = 0;
> >
> > I actually want to push back on this a bit, because there can still be
> > logic bugs built around this kind of primitive. Shouldn't out-of-bounds
> > assignments be seen as a direct failure? I would think this would be
> > sufficient:
> >
> > #define check_assign(value, ptr)    check_add_overflow(0, value, ptr)
> >
> > And callers would do:
> >
> >     if (check_assign(value, &var))
> >         return -EINVAL;
> >
> > etc.