Message-ID: <247cf9b4-5027-5099-18e8-874df1375b74@intel.com>
Date:   Wed, 21 Sep 2022 17:10:36 +0300
From:   Gwan-gyeong Mun <gwan-gyeong.mun@...el.com>
To:     Kees Cook <keescook@...omium.org>
CC:     <intel-gfx@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>,
        <dri-devel@...ts.freedesktop.org>, <mchehab@...nel.org>,
        <chris@...is-wilson.co.uk>, <matthew.auld@...el.com>,
        <thomas.hellstrom@...ux.intel.com>, <jani.nikula@...el.com>,
        <nirmoy.das@...el.com>, <airlied@...hat.com>, <daniel@...ll.ch>,
        <andi.shyti@...ux.intel.com>, <andrzej.hajda@...el.com>,
        <mauro.chehab@...ux.intel.com>, <linux@...musvillemoes.dk>,
        <vitor@...saru.org>, <dlatypov@...gle.com>,
        <ndesaulniers@...gle.com>
Subject: Re: [PATCH v10 3/9] compiler_types.h: Add assert_type to catch type
 mis-match while compiling



On 9/13/22 3:01 PM, Kees Cook wrote:
> On Fri, Sep 09, 2022 at 07:59:07PM +0900, Gwan-gyeong Mun wrote:
>> It adds assert_type and assert_typable macros to catch type mis-match while
>> compiling. The existing typecheck() macro outputs build warnings, but the
>> newly added assert_type() macro uses the _Static_assert() keyword (which is
>> introduced in C11) to generate a build break when the types are different
>> and can be used to detect explicit build errors.
>> Unlike the assert_type() macro, assert_typable() macro allows a constant
>> value as the second argument.
>>
>> Suggested-by: Kees Cook <keescook@...omium.org>
>> 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>
>> Cc: Mauro Carvalho Chehab <mchehab@...nel.org>
>> Cc: Andrzej Hajda <andrzej.hajda@...el.com>
>> Cc: Kees Cook <keescook@...omium.org>
>> ---
>>   include/linux/compiler_types.h | 39 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 39 insertions(+)
>>
>> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
>> index 4f2a819fd60a..19cc125918bb 100644
>> --- a/include/linux/compiler_types.h
>> +++ b/include/linux/compiler_types.h
>> @@ -294,6 +294,45 @@ struct ftrace_likely_data {
>>   /* Are two types/vars the same type (ignoring qualifiers)? */
>>   #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
>>   
>> +/**
>> + * assert_type - break compile if the first argument's data type and the second
>> + *               argument's data type are not the same
>> + *
>> + * @t1: data type or variable
>> + * @t2: data type or variable
>> + *
>> + * The first and second arguments can be data types or variables or mixed (the
>> + * first argument is the data type and the second argument is variable or vice
>> + * versa). It determines whether the first argument's data type and the second
>> + * argument's data type are the same while compiling, and it breaks compile if
>> + * the two types are not the same.
>> + * See also assert_typable().
>> + */
>> +#define assert_type(t1, t2) _Static_assert(__same_type(t1, t2))
>> +
>> +/**
>> + * assert_typable - break compile if the first argument's data type and the
>> + *                  second argument's data type are not the same
>> + *
>> + * @t: data type or variable
>> + * @n: data type or variable or constant value
>> + *
>> + * The first and second arguments can be data types or variables or mixed (the
>> + * first argument is the data type and the second argument is variable or vice
>> + * versa). Unlike the assert_type() macro, this macro allows a constant value
>> + * as the second argument. And if the second argument is a constant value, it
>> + * always passes. And it doesn't mean that the types are explicitly the same.
>> + * When a constant value is used as the second argument, if you need an
>> + * overflow check when assigning a constant value to a variable of the type of
>> + * the first argument, you can use the overflows_type() macro. When a constant
> 
> I wonder if the overflows_type() check should happen in this test? It
> seems weird that assert_typable(u8, 1024) would pass...
> 
Yes, that's right. If a constant is used as an argument here, it seems 
necessary to check whether an overflow occurs when the constant value is 
assigned to the target type or target variable.

>> + * value is not used as a second argument, it determines whether the first
>> + * argument's data type and the second argument's data type are the same while
>> + * compiling, and it breaks compile if the two types are not the same.
>> + * See also assert_type() and overflows_type().
>> + */
>> +#define assert_typable(t, n) _Static_assert(__builtin_constant_p(n) ||	\
>> +					    __same_type(t, typeof(n)))
> 
> Totally untested -- I'm not sure if this gets the right semantics for
> constant expressoins, etc...
> 
> static_assert(__builtin_choose_expression(__builtin_constant_p(n), \
> 			overflows_type(n, typeof(t)), \
> 			__same_type(t, typeof(n))))
> 
> 
However, if we change the macro in the form below, the "error: 
expression in static assertion is not constant" error occurs due to the 
restriction [1][2] of _Static_assert() as you mentioned.
( overflows_type() internally uses the __builtin_add_overflow() builtin 
function [3], which returns a bool type.)

#define assert_same_typable(t, n) static_assert(			     \
		__builtin_choose_expr(__builtin_constant_p(n),		     \
				      overflows_type(n, typeof(t)) == false, \
				      __same_type(t, typeof(n))))

Can I have your opinion on the new addition of 
overflows_type_return_const_expr(), which returns a constant value at 
compile time to check whether an overflow occurs when assigning a 
constant value to an argument type?
If it is allowable to add the macro,   I would try to use the macro that 
returns "an integer constant expression" after checking for overflow 
between the constant value and the argument type at compile time with 
reference to implemented in the previous version. [4] or [5]

#define assert_same_typable(t, n) static_assert(				\
	__builtin_choose_expr(__builtin_constant_p(n),				\
			      overflows_type_return_const_expr(n,t) == 0,	\
			      __same_type(t, typeof(n))))


option (1): add is_unsigned_type() and overflows_type_return_const_expr()
#define is_unsigned_type(x)     (!is_signed_type(x))
#define overflows_type_return_const_expr(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)


or option (2): modify current __type_half_max(), type_max(), type_min() 
and add overflows_type_return_const_expr()

#define __type_half_max(x) (((typeof(x))1) << (BITS_PER_TYPE(x) - 1 - 
is_signed_type(x)))
#define type_max(x) ((typeof(x))((__type_half_max(x) - 1) + 
__type_half_max(x)))
#define type_min(x) ((typeof(x))((typeof(x))-type_max(x)-(typeof(x))1))
#define overflows_type_return_const_expr(x,T) (	\
	is_unsigned_type(x) ? \
		x > type_max(T) ? 1 : 0 \
	: is_unsigned_type(T) ? \
		x < 0 || x > type_max(T) ? 1 : 0 \
		: x < type_min(T) || x > type_max(T) ? 1 : 0 )

> Also, can you please add KUnit tests for these new helpers into
> lib/overflow_kunit.c?
> 
yes the kunit tests for assert_same_typable() and assert_same_type() 
will be added in the case of normal build in the form below so that the 
build of other test cases is not interrupted. [6]

And the added overflows_type() and check_assign() and 
check_assign_user_ptr() macros use the check_add_overflow() macro, and 
this macro is verified with another test case. Is it necessary to add it?
And if it's okay to add the overflows_type_return_const_expr() macro 
mentioned above, I'll add the macro in the new version and add a test case.

[1] https://en.cppreference.com/w/c/language/_Static_assert
_Static_assert ( expression , message )		(since C11)

[2] C11 standard (ISO/IEC 9899:2011):
6.7.10 Static assertions

Syntax
1 static_assert-declaration:
	_Static_assert ( constant-expression , string-literal ) ;

Constraints
2 The constant expression shall compare unequal to 0.

Semantics
3 The constant expression shall be an integer constant expression. If 
the value of the constant expression compares unequal to 0, the 
declaration has no effect. Otherwise, the constraint is violated and the 
implementation shall produce a diagnostic message that includes the text 
of the string literal, except that characters not in the basic source 
character set are not required to appear in the message.

[3] https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html
    6.56 Built-in Functions to Perform Arithmetic with Overflow Checking
    Built-in Function: bool __builtin_add_overflow (type1 a, type2 b, 
type3 *res)

[4] https://patchwork.freedesktop.org/patch/494722/?series=104704&rev=6

[5] 
https://lore.kernel.org/all/52c09fde-f788-4c2b-efdc-d1783dbc0f6c@intel.com/

[6]

/* Arg is: type */
#define TEST_ASSERT_SAME_TYPE(t) do {	\
    typeof(t) __t1 = type_max(t);	\
    typeof(t) __t2 = type_min(t);	\
    assert_same_type(t, t);		\
    assert_same_type(t, __t1);		\
    assert_same_type(__t1, t);		\
    assert_same_type(__t1, __t2);	\
} while (0)

/* Arg is: type */
#define TEST_ASSERT_SAME_TYPABLE(t) do {	\
    typeof(t) __t1 = type_max(t);		\
    typeof(t) __t2 = type_min(t);		\
    assert_same_typable(t, __t1);		\
    assert_same_typable(t, type_max(t));		\
    assert_same_typable(t, type_min(t));		\
    assert_same_typable(__t1, type_max(t));	\
    assert_same_typable(__t1, type_min(t));	\
    assert_same_typable(__t1, __t2);		\
} while (0)


TEST_ASSERT_SAME_TYPE(u8);
TEST_ASSERT_SAME_TYPE(u16);
TEST_ASSERT_SAME_TYPE(u32);
TEST_ASSERT_SAME_TYPE(u64);
TEST_ASSERT_SAME_TYPE(s8);
TEST_ASSERT_SAME_TYPE(s16);
TEST_ASSERT_SAME_TYPE(s32);
TEST_ASSERT_SAME_TYPE(s64);
TEST_ASSERT_SAME_TYPABLE(u8);
TEST_ASSERT_SAME_TYPABLE(u16);
TEST_ASSERT_SAME_TYPABLE(u32);
TEST_ASSERT_SAME_TYPABLE(u64);
TEST_ASSERT_SAME_TYPABLE(s8);
TEST_ASSERT_SAME_TYPABLE(s16);
TEST_ASSERT_SAME_TYPABLE(s32);
TEST_ASSERT_SAME_TYPABLE(s64);

Powered by blists - more mailing lists