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
