lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 23 Aug 2022 04:32:10 +0900 From: Gwan-gyeong Mun <gwan-gyeong.mun@...el.com> To: Andrzej Hajda <andrzej.hajda@...el.com>, Kees Cook <keescook@...omium.org>, Andi Shyti <andi.shyti@...ux.intel.com> CC: <thomas.hellstrom@...ux.intel.com>, <jani.nikula@...el.com>, <intel-gfx@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>, <chris@...is-wilson.co.uk>, <airlied@...ux.ie>, <linux-hardening@...r.kernel.org>, <matthew.auld@...el.com>, <mchehab@...nel.org>, <nirmoy.das@...el.com>, Mauro Carvalho Chehab <mauro.chehab@...ux.intel.com> Subject: Re: [PATCH v7 1/8] overflow: Move and add few utility macros into overflow 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@...el.com> >>>> Cc: Thomas Hellström <thomas.hellstrom@...ux.intel.com> >>>> Cc: Matthew Auld <matthew.auld@...el.com> >>>> Cc: Nirmoy Das <nirmoy.das@...el.com> >>>> Cc: Jani Nikula <jani.nikula@...el.com> >>>> Cc: Andi Shyti <andi.shyti@...ux.intel.com> >>>> Reviewed-by: Mauro Carvalho Chehab <mchehab@...nel.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. >> >> >
Powered by blists - more mailing lists