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: Mon, 22 Aug 2022 16:26:24 +0200 From: Andrzej Hajda <andrzej.hajda@...el.com> To: Kees Cook <keescook@...omium.org>, Andi Shyti <andi.shyti@...ux.intel.com> Cc: thomas.hellstrom@...ux.intel.com, mchehab@...nel.org, jani.nikula@...el.com, intel-gfx@...ts.freedesktop.org, linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org, airlied@...ux.ie, nirmoy.das@...el.com, linux-hardening@...r.kernel.org, chris@...is-wilson.co.uk, matthew.auld@...el.com Subject: Re: [Intel-gfx] [PATCH v7 1/8] overflow: Move and add few utility macros into overflow 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@...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) >>>> --- (...) >>>> + >>>> +/** >>>> + * 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. >> >> >
Powered by blists - more mailing lists