Re: [PATCH v5] overflow: Introduce overflows_type() and castable_to_type()

2022-11-02 Thread Kees Cook 
On Wed, Nov 02, 2022 at 12:52:32PM +0100, Rasmus Villemoes wrote:
> On 24/10/2022 22.11, Gwan-gyeong Mun wrote:
> > From: Kees Cook 
> > 
> > Implement a robust overflows_type() macro to test if a variable or
> > constant value would overflow another variable or type. This can be
> > used as a constant expression for static_assert() (which requires a
> > constant expression[1][2]) when used on constant values. This must be
> > constructed manually, since __builtin_add_overflow() does not produce
> > a constant expression[3].
> > 
> > Additionally adds castable_to_type(), similar to __same_type(), but for
> > checking if a constant value would overflow if cast to a given type.
> > 
> 
> > +#define __overflows_type_constexpr(x, T) ( \
> > +   is_unsigned_type(typeof(x)) ?   \
> > +   (x) > type_max(typeof(T)) ? 1 : 0   \
> > +   : is_unsigned_type(typeof(T)) ? \
> > +   (x) < 0 || (x) > type_max(typeof(T)) ? 1 : 0\
> > +   : (x) < type_min(typeof(T)) ||  \
> > + (x) > type_max(typeof(T)) ? 1 : 0)
> > +
> 
> Can't all these instances of "foo ? 1 : 0" be simplified to "foo"? That
> would improve the readability of this thing somewhat IMO.

Oh, good point. :P I'll fix these.

-- 
Kees Cook

Re: [PATCH v5] overflow: Introduce overflows_type() and castable_to_type()

2022-11-02 Thread Rasmus Villemoes 
On 24/10/2022 22.11, Gwan-gyeong Mun wrote:
> From: Kees Cook 
> 
> Implement a robust overflows_type() macro to test if a variable or
> constant value would overflow another variable or type. This can be
> used as a constant expression for static_assert() (which requires a
> constant expression[1][2]) when used on constant values. This must be
> constructed manually, since __builtin_add_overflow() does not produce
> a constant expression[3].
> 
> Additionally adds castable_to_type(), similar to __same_type(), but for
> checking if a constant value would overflow if cast to a given type.
> 

> +#define __overflows_type_constexpr(x, T) (   \
> + is_unsigned_type(typeof(x)) ?   \
> + (x) > type_max(typeof(T)) ? 1 : 0   \
> + : is_unsigned_type(typeof(T)) ? \
> + (x) < 0 || (x) > type_max(typeof(T)) ? 1 : 0\
> + : (x) < type_min(typeof(T)) ||  \
> +   (x) > type_max(typeof(T)) ? 1 : 0)
> +

Can't all these instances of "foo ? 1 : 0" be simplified to "foo"? That
would improve the readability of this thing somewhat IMO.

Rasmus

Re: [PATCH v5] overflow: Introduce overflows_type() and castable_to_type()

2022-11-02 Thread Gwan-gyeong Mun 




On 11/2/22 1:06 AM, Kees Cook wrote:

On Sat, Oct 29, 2022 at 11:01:38AM +0300, Gwan-gyeong Mun wrote:



On 10/29/22 10:32 AM, Kees Cook wrote:

On Sat, Oct 29, 2022 at 08:55:43AM +0300, Gwan-gyeong Mun wrote:

Hi Kees,


Hi! :)


I've updated to v5 with the last comment of Nathan.
Could you please kindly review what more is needed as we move forward with
this patch?


It looks fine to me -- I assume it'll go via the drm tree? Would you
rather I carry the non-drm changes in my tree instead?


Hi!
Yes, I think it would be better to run this patch on your tree.
this patch moves the macro of i915 to overflows.h and modifies one part of
drm's driver code, but I think this part can be easily applied when merging
into the drm tree.


I've rebased it to the hardening tree, and it should appear in -next
shortly:

https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/hardening=5904fcb776d0b518be96bca43f258db90f26ba9a


Thanks for making this patch go forward.

G.G.

Re: [PATCH v5] overflow: Introduce overflows_type() and castable_to_type()

2022-11-01 Thread Kees Cook 
On Sat, Oct 29, 2022 at 11:01:38AM +0300, Gwan-gyeong Mun wrote:
> 
> 
> On 10/29/22 10:32 AM, Kees Cook wrote:
> > On Sat, Oct 29, 2022 at 08:55:43AM +0300, Gwan-gyeong Mun wrote:
> > > Hi Kees,
> > 
> > Hi! :)
> > 
> > > I've updated to v5 with the last comment of Nathan.
> > > Could you please kindly review what more is needed as we move forward with
> > > this patch?
> > 
> > It looks fine to me -- I assume it'll go via the drm tree? Would you
> > rather I carry the non-drm changes in my tree instead?
> > 
> Hi!
> Yes, I think it would be better to run this patch on your tree.
> this patch moves the macro of i915 to overflows.h and modifies one part of
> drm's driver code, but I think this part can be easily applied when merging
> into the drm tree.

I've rebased it to the hardening tree, and it should appear in -next
shortly:

https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/hardening=5904fcb776d0b518be96bca43f258db90f26ba9a

-- 
Kees Cook

Re: [PATCH v5] overflow: Introduce overflows_type() and castable_to_type()

2022-10-29 Thread Gwan-gyeong Mun 




On 10/29/22 10:32 AM, Kees Cook wrote:

On Sat, Oct 29, 2022 at 08:55:43AM +0300, Gwan-gyeong Mun wrote:

Hi Kees,


Hi! :)


I've updated to v5 with the last comment of Nathan.
Could you please kindly review what more is needed as we move forward with
this patch?


It looks fine to me -- I assume it'll go via the drm tree? Would you
rather I carry the non-drm changes in my tree instead?


Hi!
Yes, I think it would be better to run this patch on your tree.
this patch moves the macro of i915 to overflows.h and modifies one part 
of drm's driver code, but I think this part can be easily applied when 
merging into the drm tree.


Many thanks,
G.G.

Re: [PATCH v5] overflow: Introduce overflows_type() and castable_to_type()

2022-10-29 Thread Kees Cook 
On Sat, Oct 29, 2022 at 08:55:43AM +0300, Gwan-gyeong Mun wrote:
> Hi Kees,

Hi! :)

> I've updated to v5 with the last comment of Nathan.
> Could you please kindly review what more is needed as we move forward with
> this patch?

It looks fine to me -- I assume it'll go via the drm tree? Would you
rather I carry the non-drm changes in my tree instead?

>
-- 
Kees Cook

Re: [PATCH v5] overflow: Introduce overflows_type() and castable_to_type()

2022-10-28 Thread Gwan-gyeong Mun 

Hi Kees,

I've updated to v5 with the last comment of Nathan.
Could you please kindly review what more is needed as we move forward 
with this patch?


Br,

G.G.

On 10/24/22 11:11 PM, Gwan-gyeong Mun wrote:

From: Kees Cook 

Implement a robust overflows_type() macro to test if a variable or
constant value would overflow another variable or type. This can be
used as a constant expression for static_assert() (which requires a
constant expression[1][2]) when used on constant values. This must be
constructed manually, since __builtin_add_overflow() does not produce
a constant expression[3].

Additionally adds castable_to_type(), similar to __same_type(), but for
checking if a constant value would overflow if cast to a given type.

Add unit tests for overflows_type(), __same_type(), and castable_to_type()
to the existing KUnit "overflow" test.

[1] https://en.cppreference.com/w/c/language/_Static_assert
[2] C11 standard (ISO/IEC 9899:2011): 6.7.10 Static assertions
[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,

Cc: Luc Van Oostenryck 
Cc: Nathan Chancellor 
Cc: Nick Desaulniers 
Cc: Tom Rix 
Cc: Daniel Latypov 
Cc: Vitor Massaru Iha 
Cc: "Gustavo A. R. Silva" 
Cc: Jani Nikula 
Cc: Mauro Carvalho Chehab 
Cc: linux-harden...@vger.kernel.org
Cc: l...@lists.linux.dev
Co-developed-by: Gwan-gyeong Mun 
Signed-off-by: Gwan-gyeong Mun 
Signed-off-by: Kees Cook 
---
v5: drop the cc-disable-warning and just disable the warning directly (Nathan)
v4:
  - move version v2 changelog commit message to under the --- marker (Mauro)
  - remove the #pragma addition in the code and modify the Makefile to handle 
the
same feature (Jani)
v3:
  - chagne to use uintptr_t type when checking for overflow of pointer type
variable
v2:
  - fix comment typo
  - wrap clang pragma to avoid GCC warnings
  - style nit cleanups
  - rename __castable_to_type() to castable_to_type()
  - remove prior overflows_type() definition
v1: https://lore.kernel.org/lkml/20220926003743.409911-1-keesc...@chromium.org
---
  drivers/gpu/drm/i915/i915_user_extensions.c |   2 +-
  drivers/gpu/drm/i915/i915_utils.h   |   4 -
  include/linux/compiler.h|   1 +
  include/linux/overflow.h|  48 +++
  lib/Makefile|   4 +
  lib/overflow_kunit.c| 383 +++-
  6 files changed, 436 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_user_extensions.c 
b/drivers/gpu/drm/i915/i915_user_extensions.c
index c822d0aafd2d..e3f808372c47 100644
--- a/drivers/gpu/drm/i915/i915_user_extensions.c
+++ b/drivers/gpu/drm/i915/i915_user_extensions.c
@@ -51,7 +51,7 @@ int i915_user_extensions(struct i915_user_extension __user 
*ext,
return err;
  
  		if (get_user(next, >next_extension) ||

-   overflows_type(next, ext))
+   overflows_type(next, uintptr_t))
return -EFAULT;
  
  		ext = u64_to_user_ptr(next);

diff --git a/drivers/gpu/drm/i915/i915_utils.h 
b/drivers/gpu/drm/i915/i915_utils.h
index 6c14d13364bf..67a66d4d5c70 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -111,10 +111,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/compiler.h b/include/linux/compiler.h
index 973a1bfd7ef5..947a60b801db 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -236,6 +236,7 @@ static inline void *offset_to_ptr(const int *off)
   * bool and also pointer types.
   */
  #define is_signed_type(type) (((type)(-1)) < (__force type)1)
+#define is_unsigned_type(type) (!is_signed_type(type))
  
  /*

   * This is needed in functions which generate the stack canary, see
diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index 19dfdd74835e..58eb34aa2af9 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -127,6 +127,54 @@ static inline bool __must_check __must_check_overflow(bool 
overflow)
(*_d >> _to_shift) != _a);\
  }))
  
+#define __overflows_type_constexpr(x, T) (			\

+   is_unsigned_type(typeof(x)) ?   \
+   (x) > type_max(typeof(T)) ? 1 : 0\
+   : is_unsigned_type(typeof(T)) ? \
+

[PATCH v5] overflow: Introduce overflows_type() and castable_to_type()

2022-10-24 Thread Gwan-gyeong Mun 
From: Kees Cook 

Implement a robust overflows_type() macro to test if a variable or
constant value would overflow another variable or type. This can be
used as a constant expression for static_assert() (which requires a
constant expression[1][2]) when used on constant values. This must be
constructed manually, since __builtin_add_overflow() does not produce
a constant expression[3].

Additionally adds castable_to_type(), similar to __same_type(), but for
checking if a constant value would overflow if cast to a given type.

Add unit tests for overflows_type(), __same_type(), and castable_to_type()
to the existing KUnit "overflow" test.

[1] https://en.cppreference.com/w/c/language/_Static_assert
[2] C11 standard (ISO/IEC 9899:2011): 6.7.10 Static assertions
[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,

Cc: Luc Van Oostenryck 
Cc: Nathan Chancellor 
Cc: Nick Desaulniers 
Cc: Tom Rix 
Cc: Daniel Latypov 
Cc: Vitor Massaru Iha 
Cc: "Gustavo A. R. Silva" 
Cc: Jani Nikula 
Cc: Mauro Carvalho Chehab 
Cc: linux-harden...@vger.kernel.org
Cc: l...@lists.linux.dev
Co-developed-by: Gwan-gyeong Mun 
Signed-off-by: Gwan-gyeong Mun 
Signed-off-by: Kees Cook 
---
v5: drop the cc-disable-warning and just disable the warning directly (Nathan)
v4:
 - move version v2 changelog commit message to under the --- marker (Mauro)
 - remove the #pragma addition in the code and modify the Makefile to handle the
   same feature (Jani)
v3:
 - chagne to use uintptr_t type when checking for overflow of pointer type
   variable
v2:
 - fix comment typo
 - wrap clang pragma to avoid GCC warnings
 - style nit cleanups
 - rename __castable_to_type() to castable_to_type()
 - remove prior overflows_type() definition
v1: https://lore.kernel.org/lkml/20220926003743.409911-1-keesc...@chromium.org
---
 drivers/gpu/drm/i915/i915_user_extensions.c |   2 +-
 drivers/gpu/drm/i915/i915_utils.h   |   4 -
 include/linux/compiler.h|   1 +
 include/linux/overflow.h|  48 +++
 lib/Makefile|   4 +
 lib/overflow_kunit.c| 383 +++-
 6 files changed, 436 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_user_extensions.c 
b/drivers/gpu/drm/i915/i915_user_extensions.c
index c822d0aafd2d..e3f808372c47 100644
--- a/drivers/gpu/drm/i915/i915_user_extensions.c
+++ b/drivers/gpu/drm/i915/i915_user_extensions.c
@@ -51,7 +51,7 @@ int i915_user_extensions(struct i915_user_extension __user 
*ext,
return err;
 
if (get_user(next, >next_extension) ||
-   overflows_type(next, ext))
+   overflows_type(next, uintptr_t))
return -EFAULT;
 
ext = u64_to_user_ptr(next);
diff --git a/drivers/gpu/drm/i915/i915_utils.h 
b/drivers/gpu/drm/i915/i915_utils.h
index 6c14d13364bf..67a66d4d5c70 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -111,10 +111,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/compiler.h b/include/linux/compiler.h
index 973a1bfd7ef5..947a60b801db 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -236,6 +236,7 @@ static inline void *offset_to_ptr(const int *off)
  * bool and also pointer types.
  */
 #define is_signed_type(type) (((type)(-1)) < (__force type)1)
+#define is_unsigned_type(type) (!is_signed_type(type))
 
 /*
  * This is needed in functions which generate the stack canary, see
diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index 19dfdd74835e..58eb34aa2af9 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -127,6 +127,54 @@ static inline bool __must_check __must_check_overflow(bool 
overflow)
(*_d >> _to_shift) != _a);  \
 }))
 
+#define __overflows_type_constexpr(x, T) ( \
+   is_unsigned_type(typeof(x)) ?   \
+   (x) > type_max(typeof(T)) ? 1 : 0   \
+   : is_unsigned_type(typeof(T)) ? \
+   (x) < 0 || (x) > type_max(typeof(T)) ? 1 : 0\
+   : (x) < type_min(typeof(T)) ||  \
+ (x) > type_max(typeof(T)) ? 1 : 0)
+
+#define __overflows_type(x, T)