Messages in this thread | | | Date | Tue, 23 Aug 2022 11:30:54 +0900 | Subject | Re: [PATCH v7 1/8] overflow: Move and add few utility macros into overflow | From | Gwan-gyeong Mun <> |
| |
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 >
| |