| Message ID | 20220816093525.184940-2-gwan-gyeong.mun@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes integer overflow or integer truncation issues in page lookups, ttm place configuration and scatterlist creation | expand |
Hi Kees, would you mind taking a look at this patch? 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@intel.com> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > Cc: Matthew Auld <matthew.auld@intel.com> > Cc: Nirmoy Das <nirmoy.das@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org> (v5) > --- > drivers/gpu/drm/i915/i915_utils.h | 5 +-- > include/linux/overflow.h | 54 +++++++++++++++++++++++++++++++ > 2 files changed, 55 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h > index c10d68cdc3ca..eb0ded23fa9c 100644 > --- a/drivers/gpu/drm/i915/i915_utils.h > +++ b/drivers/gpu/drm/i915/i915_utils.h > @@ -32,6 +32,7 @@ > #include <linux/types.h> > #include <linux/workqueue.h> > #include <linux/sched/clock.h> > +#include <linux/overflow.h> > > #ifdef CONFIG_X86 > #include <asm/hypervisor.h> > @@ -111,10 +112,6 @@ bool i915_error_injected(void); > #define range_overflows_end_t(type, start, size, max) \ > range_overflows_end((type)(start), (type)(size), (type)(max)) > > -/* Note we don't consider signbits :| */ > -#define overflows_type(x, T) \ > - (sizeof(x) > sizeof(T) && (x) >> BITS_PER_TYPE(T)) > - > #define ptr_mask_bits(ptr, n) ({ \ > unsigned long __v = (unsigned long)(ptr); \ > (typeof(ptr))(__v & -BIT(n)); \ > diff --git a/include/linux/overflow.h b/include/linux/overflow.h > index f1221d11f8e5..462a03454377 100644 > --- a/include/linux/overflow.h > +++ b/include/linux/overflow.h > @@ -35,6 +35,60 @@ > #define type_max(T) ((T)((__type_half_max(T) - 1) + __type_half_max(T))) > #define type_min(T) ((T)((T)-type_max(T)-(T)1)) > > +/** > + * is_unsigned_type - helper for checking data type which is an unsigned data > + * type or not > + * @x: The data type to check > + * > + * Returns: > + * True if the data type is an unsigned data type, false otherwise. > + */ > +#define is_unsigned_type(x) ((typeof(x))-1 >= (typeof(x))0) > + > +/** > + * 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) > + > +/** > + * 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); \ > +}) > + > /* > * Avoids triggering -Wtype-limits compilation warning, > * while using unsigned data types to check a < 0. > -- > 2.37.1
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@intel.com> > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > > Cc: Matthew Auld <matthew.auld@intel.com> > > Cc: Nirmoy Das <nirmoy.das@intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Andi Shyti <andi.shyti@linux.intel.com> > > Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org> (v5) > > --- > > drivers/gpu/drm/i915/i915_utils.h | 5 +-- > > include/linux/overflow.h | 54 +++++++++++++++++++++++++++++++ > > 2 files changed, 55 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h > > index c10d68cdc3ca..eb0ded23fa9c 100644 > > --- a/drivers/gpu/drm/i915/i915_utils.h > > +++ b/drivers/gpu/drm/i915/i915_utils.h > > @@ -32,6 +32,7 @@ > > #include <linux/types.h> > > #include <linux/workqueue.h> > > #include <linux/sched/clock.h> > > +#include <linux/overflow.h> > > > > #ifdef CONFIG_X86 > > #include <asm/hypervisor.h> > > @@ -111,10 +112,6 @@ bool i915_error_injected(void); > > #define range_overflows_end_t(type, start, size, max) \ > > range_overflows_end((type)(start), (type)(size), (type)(max)) > > > > -/* Note we don't consider signbits :| */ > > -#define overflows_type(x, T) \ > > - (sizeof(x) > sizeof(T) && (x) >> BITS_PER_TYPE(T)) > > - > > #define ptr_mask_bits(ptr, n) ({ \ > > unsigned long __v = (unsigned long)(ptr); \ > > (typeof(ptr))(__v & -BIT(n)); \ > > diff --git a/include/linux/overflow.h b/include/linux/overflow.h > > index f1221d11f8e5..462a03454377 100644 > > --- a/include/linux/overflow.h > > +++ b/include/linux/overflow.h > > @@ -35,6 +35,60 @@ > > #define type_max(T) ((T)((__type_half_max(T) - 1) + __type_half_max(T))) > > #define type_min(T) ((T)((T)-type_max(T)-(T)1)) > > > > +/** > > + * is_unsigned_type - helper for checking data type which is an unsigned data > > + * type or not > > + * @x: The data type to check > > + * > > + * Returns: > > + * True if the data type is an unsigned data type, false otherwise. > > + */ > > +#define is_unsigned_type(x) ((typeof(x))-1 >= (typeof(x))0) I'd rather not have separate logic for this. Instead, I'd like it to be: #define is_unsigned_type(x) (!is_signed_type(x)) > > + > > +/** > > + * 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().) > > + > > +/** > > + * 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.
On Wed, 17 Aug 2022, Kees Cook <keescook@chromium.org> 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 for your review. This actually reaffirms my belief that we need to get these macros out of i915_utils.h and into the common headers, where we can get more eyes on them. BR, Jani. > >> >> 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@intel.com> >> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> >> > Cc: Matthew Auld <matthew.auld@intel.com> >> > Cc: Nirmoy Das <nirmoy.das@intel.com> >> > Cc: Jani Nikula <jani.nikula@intel.com> >> > Cc: Andi Shyti <andi.shyti@linux.intel.com> >> > Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org> (v5) >> > --- >> > drivers/gpu/drm/i915/i915_utils.h | 5 +-- >> > include/linux/overflow.h | 54 +++++++++++++++++++++++++++++++ >> > 2 files changed, 55 insertions(+), 4 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h >> > index c10d68cdc3ca..eb0ded23fa9c 100644 >> > --- a/drivers/gpu/drm/i915/i915_utils.h >> > +++ b/drivers/gpu/drm/i915/i915_utils.h >> > @@ -32,6 +32,7 @@ >> > #include <linux/types.h> >> > #include <linux/workqueue.h> >> > #include <linux/sched/clock.h> >> > +#include <linux/overflow.h> >> > >> > #ifdef CONFIG_X86 >> > #include <asm/hypervisor.h> >> > @@ -111,10 +112,6 @@ bool i915_error_injected(void); >> > #define range_overflows_end_t(type, start, size, max) \ >> > range_overflows_end((type)(start), (type)(size), (type)(max)) >> > >> > -/* Note we don't consider signbits :| */ >> > -#define overflows_type(x, T) \ >> > - (sizeof(x) > sizeof(T) && (x) >> BITS_PER_TYPE(T)) >> > - >> > #define ptr_mask_bits(ptr, n) ({ \ >> > unsigned long __v = (unsigned long)(ptr); \ >> > (typeof(ptr))(__v & -BIT(n)); \ >> > diff --git a/include/linux/overflow.h b/include/linux/overflow.h >> > index f1221d11f8e5..462a03454377 100644 >> > --- a/include/linux/overflow.h >> > +++ b/include/linux/overflow.h >> > @@ -35,6 +35,60 @@ >> > #define type_max(T) ((T)((__type_half_max(T) - 1) + __type_half_max(T))) >> > #define type_min(T) ((T)((T)-type_max(T)-(T)1)) >> > >> > +/** >> > + * is_unsigned_type - helper for checking data type which is an unsigned data >> > + * type or not >> > + * @x: The data type to check >> > + * >> > + * Returns: >> > + * True if the data type is an unsigned data type, false otherwise. >> > + */ >> > +#define is_unsigned_type(x) ((typeof(x))-1 >= (typeof(x))0) > > I'd rather not have separate logic for this. Instead, I'd like it to be: > > #define is_unsigned_type(x) (!is_signed_type(x)) > >> > + >> > +/** >> > + * 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().) > >> > + >> > +/** >> > + * 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.
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@intel.com> >>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> >>> Cc: Matthew Auld <matthew.auld@intel.com> >>> Cc: Nirmoy Das <nirmoy.das@intel.com> >>> Cc: Jani Nikula <jani.nikula@intel.com> >>> Cc: Andi Shyti <andi.shyti@linux.intel.com> >>> Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org> (v5) >>> --- >>> drivers/gpu/drm/i915/i915_utils.h | 5 +-- >>> include/linux/overflow.h | 54 +++++++++++++++++++++++++++++++ >>> 2 files changed, 55 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h >>> index c10d68cdc3ca..eb0ded23fa9c 100644 >>> --- a/drivers/gpu/drm/i915/i915_utils.h >>> +++ b/drivers/gpu/drm/i915/i915_utils.h >>> @@ -32,6 +32,7 @@ >>> #include <linux/types.h> >>> #include <linux/workqueue.h> >>> #include <linux/sched/clock.h> >>> +#include <linux/overflow.h> >>> >>> #ifdef CONFIG_X86 >>> #include <asm/hypervisor.h> >>> @@ -111,10 +112,6 @@ bool i915_error_injected(void); >>> #define range_overflows_end_t(type, start, size, max) \ >>> range_overflows_end((type)(start), (type)(size), (type)(max)) >>> >>> -/* Note we don't consider signbits :| */ >>> -#define overflows_type(x, T) \ >>> - (sizeof(x) > sizeof(T) && (x) >> BITS_PER_TYPE(T)) >>> - >>> #define ptr_mask_bits(ptr, n) ({ \ >>> unsigned long __v = (unsigned long)(ptr); \ >>> (typeof(ptr))(__v & -BIT(n)); \ >>> diff --git a/include/linux/overflow.h b/include/linux/overflow.h >>> index f1221d11f8e5..462a03454377 100644 >>> --- a/include/linux/overflow.h >>> +++ b/include/linux/overflow.h >>> @@ -35,6 +35,60 @@ >>> #define type_max(T) ((T)((__type_half_max(T) - 1) + __type_half_max(T))) >>> #define type_min(T) ((T)((T)-type_max(T)-(T)1)) >>> >>> +/** >>> + * is_unsigned_type - helper for checking data type which is an unsigned data >>> + * type or not >>> + * @x: The data type to check >>> + * >>> + * Returns: >>> + * True if the data type is an unsigned data type, false otherwise. >>> + */ >>> +#define is_unsigned_type(x) ((typeof(x))-1 >= (typeof(x))0) > > I'd rather not have separate logic for this. Instead, I'd like it to be: > > #define is_unsigned_type(x) (!is_signed_type(x)) > >>> + >>> +/** >>> + * 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) 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. > >
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@intel.com> >>>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> >>>> Cc: Matthew Auld <matthew.auld@intel.com> >>>> Cc: Nirmoy Das <nirmoy.das@intel.com> >>>> Cc: Jani Nikula <jani.nikula@intel.com> >>>> Cc: Andi Shyti <andi.shyti@linux.intel.com> >>>> Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.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. >> >> >
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: >>> 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@intel.com> >>>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> >>>> Cc: Matthew Auld <matthew.auld@intel.com> >>>> Cc: Nirmoy Das <nirmoy.das@intel.com> >>>> Cc: Jani Nikula <jani.nikula@intel.com> >>>> Cc: Andi Shyti <andi.shyti@linux.intel.com> >>>> Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org> (v5) >>>> --- >>>> drivers/gpu/drm/i915/i915_utils.h | 5 +-- >>>> include/linux/overflow.h | 54 >>>> +++++++++++++++++++++++++++++++ >>>> 2 files changed, 55 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_utils.h >>>> b/drivers/gpu/drm/i915/i915_utils.h >>>> index c10d68cdc3ca..eb0ded23fa9c 100644 >>>> --- a/drivers/gpu/drm/i915/i915_utils.h >>>> +++ b/drivers/gpu/drm/i915/i915_utils.h >>>> @@ -32,6 +32,7 @@ >>>> #include <linux/types.h> >>>> #include <linux/workqueue.h> >>>> #include <linux/sched/clock.h> >>>> +#include <linux/overflow.h> >>>> #ifdef CONFIG_X86 >>>> #include <asm/hypervisor.h> >>>> @@ -111,10 +112,6 @@ bool i915_error_injected(void); >>>> #define range_overflows_end_t(type, start, size, max) \ >>>> range_overflows_end((type)(start), (type)(size), (type)(max)) >>>> -/* Note we don't consider signbits :| */ >>>> -#define overflows_type(x, T) \ >>>> - (sizeof(x) > sizeof(T) && (x) >> BITS_PER_TYPE(T)) >>>> - >>>> #define ptr_mask_bits(ptr, n) ({ \ >>>> unsigned long __v = (unsigned long)(ptr); \ >>>> (typeof(ptr))(__v & -BIT(n)); \ >>>> diff --git a/include/linux/overflow.h b/include/linux/overflow.h >>>> index f1221d11f8e5..462a03454377 100644 >>>> --- a/include/linux/overflow.h >>>> +++ b/include/linux/overflow.h >>>> @@ -35,6 +35,60 @@ >>>> #define type_max(T) ((T)((__type_half_max(T) - 1) + >>>> __type_half_max(T))) >>>> #define type_min(T) ((T)((T)-type_max(T)-(T)1)) >>>> +/** >>>> + * is_unsigned_type - helper for checking data type which is an >>>> unsigned data >>>> + * type or not >>>> + * @x: The data type to check >>>> + * >>>> + * Returns: >>>> + * True if the data type is an unsigned data type, false otherwise. >>>> + */ >>>> +#define is_unsigned_type(x) ((typeof(x))-1 >= (typeof(x))0) >> >> I'd rather not have separate logic for this. Instead, I'd like it to be: >> >> #define is_unsigned_type(x) (!is_signed_type(x)) >> >>>> + >>>> +/** >>>> + * 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().) > > Thanks for all of your comments. The version that implements overflows_type() using type_min() and type_max() includes modifications to the following macros. In implementations of is_signed_type(), __type_half_max(), type_max(), type_min(), where types are used as variables, the addition of typeof() is necessary. And the operation was confirmed through previously shared test cases. https://patchwork.freedesktop.org/patch/492374/?series=104704&rev=3 #define is_signed_type(x) (((typeof(x))(-1)) < (typeof(x))1) #define is_unsigned_type(x) (!is_signed_type(x)) #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(x, T) __must_check_overflow( \ 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 ) > 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) > And it was confirmed that the method using the gcc built-in functions suggested by Andrzej works the same in all cases where it is used. #define overflows_type(x, T) __must_check_overflow(({ \ typeof(T) r = 0; \ __builtin_add_overflow_p((x), r, r); \ })) And if you refer to this link (https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html), it is explained like this. The compiler will attempt to use hardware instructions to implement these built-in functions where possible, like conditional jump on overflow after addition, conditional jump on carry etc. Andrzej's suggested way seems better to me. What do you think? Kees Cook, can I ask for your feedback? Additionally, unlike the first implemented method (v7's overflows_type() macro), the macros tested above generate errors at build time for pointer types. __type_half_max() throws error "error: invalid operands to binary <<" or For __builtin_add_overflow_p() I get the error "__builtin_add_overflow_p' does not have integral type". So, overflow check for pointer type was confirmed by adding the following macro. #define overflows_ptr(x, T) __must_check_overflow(({ \ typecheck_pointer(T); \ ((x) < 0) ? 1 : (sizeof(x) > sizeof(T) && (x) >> BITS_PER_TYPE(T)) ? 1 : 0; \ })) > 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; >> 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? /** * 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); \ })) Br, G.G. >> etc. >> >> >
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". > /** > * 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()? -Kees
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 >
diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h index c10d68cdc3ca..eb0ded23fa9c 100644 --- a/drivers/gpu/drm/i915/i915_utils.h +++ b/drivers/gpu/drm/i915/i915_utils.h @@ -32,6 +32,7 @@ #include <linux/types.h> #include <linux/workqueue.h> #include <linux/sched/clock.h> +#include <linux/overflow.h> #ifdef CONFIG_X86 #include <asm/hypervisor.h> @@ -111,10 +112,6 @@ bool i915_error_injected(void); #define range_overflows_end_t(type, start, size, max) \ range_overflows_end((type)(start), (type)(size), (type)(max)) -/* Note we don't consider signbits :| */ -#define overflows_type(x, T) \ - (sizeof(x) > sizeof(T) && (x) >> BITS_PER_TYPE(T)) - #define ptr_mask_bits(ptr, n) ({ \ unsigned long __v = (unsigned long)(ptr); \ (typeof(ptr))(__v & -BIT(n)); \ diff --git a/include/linux/overflow.h b/include/linux/overflow.h index f1221d11f8e5..462a03454377 100644 --- a/include/linux/overflow.h +++ b/include/linux/overflow.h @@ -35,6 +35,60 @@ #define type_max(T) ((T)((__type_half_max(T) - 1) + __type_half_max(T))) #define type_min(T) ((T)((T)-type_max(T)-(T)1)) +/** + * is_unsigned_type - helper for checking data type which is an unsigned data + * type or not + * @x: The data type to check + * + * Returns: + * True if the data type is an unsigned data type, false otherwise. + */ +#define is_unsigned_type(x) ((typeof(x))-1 >= (typeof(x))0) + +/** + * 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) + +/** + * 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); \ +}) + /* * Avoids triggering -Wtype-limits compilation warning, * while using unsigned data types to check a < 0.
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@intel.com> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> Cc: Matthew Auld <matthew.auld@intel.com> Cc: Nirmoy Das <nirmoy.das@intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Cc: Andi Shyti <andi.shyti@linux.intel.com> Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org> (v5) --- drivers/gpu/drm/i915/i915_utils.h | 5 +-- include/linux/overflow.h | 54 +++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 4 deletions(-)