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

[Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>]

On 8/22/22 11:05 PM, 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@intel.com>
>>>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>>> Cc: Matthew Auld <matthew.auld@intel.com>
>>>> Cc: Nirmoy Das <nirmoy.das@intel.com>
>>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
>>>> Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org> (v5)
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_utils.h |  5 +--
>>>>   include/linux/overflow.h          | 54 
>>>> +++++++++++++++++++++++++++++++
>>>>   2 files changed, 55 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_utils.h 
>>>> b/drivers/gpu/drm/i915/i915_utils.h
>>>> index c10d68cdc3ca..eb0ded23fa9c 100644
>>>> --- a/drivers/gpu/drm/i915/i915_utils.h
>>>> +++ b/drivers/gpu/drm/i915/i915_utils.h
>>>> @@ -32,6 +32,7 @@
>>>>   #include <linux/types.h>
>>>>   #include <linux/workqueue.h>
>>>>   #include <linux/sched/clock.h>
>>>> +#include <linux/overflow.h>
>>>>   #ifdef CONFIG_X86
>>>>   #include <asm/hypervisor.h>
>>>> @@ -111,10 +112,6 @@ bool i915_error_injected(void);
>>>>   #define range_overflows_end_t(type, start, size, max) \
>>>>       range_overflows_end((type)(start), (type)(size), (type)(max))
>>>> -/* Note we don't consider signbits :| */
>>>> -#define overflows_type(x, T) \
>>>> -    (sizeof(x) > sizeof(T) && (x) >> BITS_PER_TYPE(T))
>>>> -
>>>>   #define ptr_mask_bits(ptr, n) ({                    \
>>>>       unsigned long __v = (unsigned long)(ptr);            \
>>>>       (typeof(ptr))(__v & -BIT(n));                    \
>>>> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
>>>> index f1221d11f8e5..462a03454377 100644
>>>> --- a/include/linux/overflow.h
>>>> +++ b/include/linux/overflow.h
>>>> @@ -35,6 +35,60 @@
>>>>   #define type_max(T) ((T)((__type_half_max(T) - 1) + 
>>>> __type_half_max(T)))
>>>>   #define type_min(T) ((T)((T)-type_max(T)-(T)1))
>>>> +/**
>>>> + * is_unsigned_type - helper for checking data type which is an 
>>>> unsigned data
>>>> + * type or not
>>>> + * @x: The data type to check
>>>> + *
>>>> + * Returns:
>>>> + * True if the data type is an unsigned data type, false otherwise.
>>>> + */
>>>> +#define is_unsigned_type(x) ((typeof(x))-1 >= (typeof(x))0)
>>
>> I'd rather not have separate logic for this. Instead, I'd like it to be:
>>
>> #define is_unsigned_type(x) (!is_signed_type(x))
>>
>>>> +
>>>> +/**
>>>> + * 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().)
> 
> 
Thanks for all of your comments.

The version that implements overflows_type() using type_min() and 
type_max() includes modifications to the following macros.
In implementations of is_signed_type(), __type_half_max(), type_max(), 
type_min(), where types are used as variables, the addition of typeof() 
is necessary.
And the operation was confirmed through previously shared test cases.
https://patchwork.freedesktop.org/patch/492374/?series=104704&rev=3

#define is_signed_type(x)	(((typeof(x))(-1)) < (typeof(x))1)
#define is_unsigned_type(x)	(!is_signed_type(x))
#define __type_half_max(x) (((typeof(x))1) << (BITS_PER_TYPE(x) - 1 - 
is_signed_type(x)))
#define type_max(x) ((typeof(x))((__type_half_max(x) - 1) + 
__type_half_max(x)))
#define type_min(x) ((typeof(x))((typeof(x))-type_max(x)-(typeof(x))1))


#define overflows_type(x, T)	__must_check_overflow(	\
	is_unsigned_type(x) ?				\
		x > type_max(T) ? 1 : 0 		\
	: is_unsigned_type(T) ?				\
		x < 0 || x > type_max(T) ? 1 : 0	\
		: x < type_min(T) || x > type_max(T) ? 1 : 0 )


> 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)
> 
And it was confirmed that the method using the gcc built-in functions 
suggested by Andrzej works the same in all cases where it is used.

#define overflows_type(x, T) __must_check_overflow(({	\
	typeof(T) r = 0;				\
	__builtin_add_overflow_p((x), r, r);		\
}))

And if you refer to this link 
(https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html), it 
is explained like this.
   The compiler will attempt to use hardware instructions to implement 
these built-in functions where possible, like conditional jump on 
overflow after addition, conditional jump on carry etc.

Andrzej's suggested way seems better to me. What do you think? Kees 
Cook, can I ask for your feedback?

Additionally, unlike the first implemented method (v7's overflows_type() 
macro), the macros tested above generate errors at build time for 
pointer types.
__type_half_max() throws error "error: invalid operands to binary <<"
  or
For __builtin_add_overflow_p() I get the error 
"__builtin_add_overflow_p' does not have integral type".

So, overflow check for pointer type was confirmed by adding the 
following macro.

#define overflows_ptr(x, T) __must_check_overflow(({	\
	typecheck_pointer(T);				\
	((x) < 0) ? 1 : (sizeof(x) > sizeof(T) && (x) >> BITS_PER_TYPE(T)) ? 1 
: 0; \
}))

> 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;
>>
Yes, I also like check_assign() you suggested more than safe_conversion.
As shown below, it would be more readable to return true when assign 
succeeds and false when it fails. What do you think?
/**
  * check_assign - 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.
  *
  * @value: Source value
  * @ptr: Destination pointer address, If the pointer type is not used, 
a warning message is output during build.
  *
  * Returns:
  * If the value would overflow the destination, it returns false. If 
not return true.
  */
#define check_assign(value, ptr) __must_check_overflow(({	\
	typecheck_pointer(ptr); 		\
	!__builtin_add_overflow(0, value, ptr);	\
}))

Br,
G.G.
>> etc.
>>
>>
>