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 11:30:54 +0900 From: Gwan-gyeong Mun <gwan-gyeong.mun@...el.com> To: Kees Cook <keescook@...omium.org> CC: Andrzej Hajda <andrzej.hajda@...el.com>, Andi Shyti <andi.shyti@...ux.intel.com>, <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/23/22 5:12 AM, Kees Cook wrote: > On Tue, Aug 23, 2022 at 04:32:10AM +0900, Gwan-gyeong Mun wrote: >> 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: >>>>> [...] >>>>>> +#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? > > No, this inverts the style of all the other check_*() functions, so it > should remain "non-zero is failure". > Hi Kees, Yes, I will not invert this part as you commented. >> /** >> * 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); \ >> })) > > Please don't use the __builtin*s, instead stick to the check_* family, > as they correctly wrap the builtins and perform type checking, etc. As > mentioned, check_assign() should just be: > > #define check_assign(value, ptr) check_add_overflow(0, value, ptr) > > I don't think any of the other code is needed? What's the use-case for > the other stuff? i.e. Why does anything need overflows_type()? > And, the reason for using the __builtin_add_overflow() built-in function directly instead of using the check_add_overflow() function is , #define check_add_overflow(a, b, d) __must_check_overflow(({ \ typeof(a) __a = (a); \ typeof(b) __b = (b); \ typeof(d) __d = (d); \ (void) (&__a == &__b); \ (void) (&__a == __d); \ __builtin_add_overflow(__a, __b, __d); \ })) In this part of the implementation of check_add_overflow() (void) (&__a == &__b); (void) (&__a == __d); When comparing the pointer types of a, b, and d, if the pointer types of source and ptr in check_assign() are different, a warning may occur when building, I used the __builtin_add_overflow() built-in function directly. Br, G.G. > -Kees >
Powered by blists - more mailing lists