| 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 linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <cbaff83b-db5e-c41d-ffab-86562ef60e8b@intel.com>
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