This patch series fixes integer overflow or integer truncation issues in
page lookups, ttm place configuration and scatterlist creation, etc.
We need to check that we avoid integer overflows when looking up a page,
and so fix all the instances where we have mistakenly used a plain integer
instead of a more suitable long.
And there is an impedance mismatch between the scatterlist API using
unsigned int and our memory/page accounting in unsigned long. That is we
may try to create a scatterlist for a large object that overflows returning
a small table into which we try to fit very many pages. As the object size
is under the control of userspace, we have to be prudent and catch the
conversion errors. To catch the implicit truncation as we switch from
unsigned long into the scatterlist's unsigned int, we use improved
overflows_type check and report E2BIG prior to the operation. This is
already used in our create ioctls to indicate if the uABI request is simply
too large for the backing store.
And ttm place also has the same problem with scatterlist creation,
and we fix the integer truncation problem with the way approached by
scatterlist creation.
And It corrects the error code to return -E2BIG when creating gem objects
using ttm or shmem, if the size is too large in each case.
In order to provide a common macro, it moves and adds a few utility macros
into overflow/util_macros header
v9: Fix overflows_type() to use __builtin_add_overflow() instead of
__builtin_add_overflow_p() (Andrzej)
Fix overflows_ptr() to use overflows_type() with the unsigned long type (Andrzej)
v8: Add check_assign() and remove safe_conversion() (Kees)
Replace safe_conversion() with check_assign() (Kees)
Fix overflows_type() to use gcc's built-in overflow function (Andrzej)
Add overflows_ptr() to allow overflow checking when assigning a value
into a pointer variable (G.G.)
v7: Fix to use WARN_ON() macro where GEM_BUG_ON() macro was used. (Jani)
v6: Move macro addition location so that it can be used by other than drm subsystem (Jani, Mauro, Andi)
Fix to follow general use case for GEM_BUG_ON(). (Jani)
v5: Fix an alignment to match open parenthesis
Fix macros to be enclosed in parentheses for complex values
Fix too long line warning
v4: Fix build warnins that reported by kernel test robot. (kernel test robot <[email protected]>)
Add kernel-doc markups to the kAPI functions and macros (Mauoro)
v3: Modify overflows_type() macro to consider signed data types and
add is_type_unsigned() macro (Mauro)
Make not use the same macro name on a function. (Mauro)
For kernel-doc, macros and functions are handled in the same namespace,
the same macro name on a function prevents ever adding documentation for it.
Not to change execution inside a macro. (Mauro)
Fix the problem that safe_conversion() macro always returns true (G.G)
Add safe_conversion_gem_bug_on() macro and remove temporal SAFE_CONVERSION() macro. (G.G.)
Chris Wilson (3):
drm/i915/gem: Typecheck page lookups
drm/i915: Check for integer truncation on scatterlist creation
drm/i915: Remove truncation warning for large objects
Gwan-gyeong Mun (5):
overflow: Move and add few utility macros into overflow
util_macros: Add exact_type macro to catch type mis-match while
compiling
drm/i915: Check for integer truncation on the configuration of ttm
place
drm/i915: Check if the size is too big while creating shmem file
drm/i915: Use error code as -E2BIG when the size of gem ttm object is
too large
drivers/gpu/drm/i915/gem/i915_gem_internal.c | 6 +-
drivers/gpu/drm/i915/gem/i915_gem_object.c | 7 +-
drivers/gpu/drm/i915/gem/i915_gem_object.h | 303 +++++++++++++++---
drivers/gpu/drm/i915/gem/i915_gem_pages.c | 27 +-
drivers/gpu/drm/i915/gem/i915_gem_phys.c | 4 +
drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 19 +-
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 23 +-
drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 5 +-
.../drm/i915/gem/selftests/i915_gem_context.c | 12 +-
.../drm/i915/gem/selftests/i915_gem_mman.c | 8 +-
.../drm/i915/gem/selftests/i915_gem_object.c | 8 +-
drivers/gpu/drm/i915/gvt/dmabuf.c | 9 +-
drivers/gpu/drm/i915/i915_gem.c | 18 +-
drivers/gpu/drm/i915/i915_scatterlist.h | 11 +
drivers/gpu/drm/i915/i915_user_extensions.c | 3 +-
drivers/gpu/drm/i915/i915_utils.h | 6 +-
drivers/gpu/drm/i915/i915_vma.c | 8 +-
drivers/gpu/drm/i915/intel_region_ttm.c | 17 +-
include/linux/overflow.h | 62 ++++
include/linux/util_macros.h | 25 ++
20 files changed, 486 insertions(+), 95 deletions(-)
--
2.37.1
It moves overflows_type utility macro into overflow header from i915_utils
header. The overflows_type can be used to catch the truncaion (overflow)
between different data types. And it adds check_assign() macro which
performs an assigning source value into destination ptr along with an
overflow check. overflow_type macro has been improved to handle the signbit
by gcc's built-in overflow check function. And it adds overflows_ptr()
helper macro for checking the overflows between a value and a pointer
type/value.
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
v8: Add check_assign() and remove safe_conversion() (Kees)
Fix overflows_type() to use gcc's built-in overflow function (Andrzej)
Add overflows_ptr() to allow overflow checking when assigning a value
into a pointer variable (G.G.)
v9: Fix overflows_type() to use __builtin_add_overflow() instead of
__builtin_add_overflow_p() (Andrzej)
Fix overflows_ptr() to use overflows_type() with the unsigned long type
(Andrzej)
Signed-off-by: Gwan-gyeong Mun <[email protected]>
Cc: Thomas Hellström <[email protected]>
Cc: Matthew Auld <[email protected]>
Cc: Nirmoy Das <[email protected]>
Cc: Jani Nikula <[email protected]>
Cc: Andi Shyti <[email protected]>
Cc: Andrzej Hajda <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Kees Cook <[email protected]>
Reviewed-by: Mauro Carvalho Chehab <[email protected]> (v5)
---
drivers/gpu/drm/i915/i915_user_extensions.c | 3 +-
drivers/gpu/drm/i915/i915_utils.h | 5 +-
include/linux/overflow.h | 62 +++++++++++++++++++++
3 files changed, 64 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..6f6b5b910968 100644
--- a/drivers/gpu/drm/i915/i915_user_extensions.c
+++ b/drivers/gpu/drm/i915/i915_user_extensions.c
@@ -50,8 +50,7 @@ int i915_user_extensions(struct i915_user_extension __user *ext,
if (err)
return err;
- if (get_user(next, &ext->next_extension) ||
- overflows_type(next, ext))
+ if (get_user(next, &ext->next_extension) || overflows_ptr(next))
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 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..6af9df1d67a1 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -52,6 +52,68 @@ static inline bool __must_check __must_check_overflow(bool overflow)
return unlikely(overflow);
}
+/**
+ * overflows_type - helper for checking the overflows between data types or
+ * values
+ *
+ * @x: Source value or data type for overflow check
+ * @T: Destination value or data type for overflow check
+ *
+ * It compares the values or data type between the first and second argument to
+ * check whether overflow can occur when assigning the first argument to the
+ * variable of the second argument. Source and Destination can be singned or
+ * unsigned data types. Source and Destination can be different data types.
+ *
+ * Returns:
+ * True if overflow can occur, false otherwise.
+ */
+#define overflows_type(x, T) __must_check_overflow(({ \
+ typeof(T) v = 0; \
+ __builtin_add_overflow((x), v, &v); \
+}))
+
+/**
+ * overflows_ptr - helper for checking the occurrence of overflows when a value
+ * assigns to the pointer data type
+ *
+ * @x: Source value for overflow check
+ *
+ * gcc's built-in overflow check functions don't support checking between the
+ * pointer type and non-pointer type. And ILP32 and LP64 have the same bit size
+ * between long and pointer. Therefore it internally compares the source value
+ * and unsigned long data type for checking overflow.
+ *
+ * Returns:
+ * True if overflow can occur, false otherwise.
+ */
+#define overflows_ptr(x) __must_check_overflow(overflows_type(x, unsigned long))
+
+/**
+ * check_assign - perform an assigning source value into destination ptr along
+ * with an overflow check.
+ *
+ * @value: Source value
+ * @ptr: Destination pointer address, If the pointer type is not used,
+ * a warning message is output during build.
+ *
+ * It checks internally the ptr is a pointer type. And it uses gcc's built-in
+ * overflow check function.
+ * It does not use the check_*() wrapper functions, but directly uses gcc's
+ * built-in overflow check function so that it can be used even when
+ * the type of value and the type pointed to by ptr are different without build
+ * warning messages.
+ *
+ * Returns:
+ * If the value would overflow the destination, it returns true. If not return
+ * false. When overflow does not occur, the assigning into ptr from value
+ * succeeds. It follows the return policy as other check_*_overflow() functions
+ * return non-zero as a failure.
+ */
+#define check_assign(value, ptr) __must_check_overflow(({ \
+ typecheck_pointer(ptr); \
+ __builtin_add_overflow(0, value, ptr); \
+}))
+
/*
* For simplicity and code hygiene, the fallback code below insists on
* a, b and *d having the same type (similar to the min() and max()
--
2.37.1
There is an impedance mismatch between the first/last valid page
frame number of ttm place in unsigned and our memory/page accounting in
unsigned long.
As the object size is under the control of userspace, we have to be prudent
and catch the conversion errors.
To catch the implicit truncation as we switch from unsigned long to
unsigned, we use overflows_type check and report E2BIG or overflow_type
prior to the operation.
v3: Not to change execution inside a macro. (Mauro)
Add safe_conversion_gem_bug_on() macro and remove temporal
SAFE_CONVERSION() macro.
v4: Fix unhandled GEM_BUG_ON() macro call from safe_conversion_gem_bug_on()
v6: Fix to follow general use case for GEM_BUG_ON(). (Jani)
v7: Fix to use WARN_ON() macro where GEM_BUG_ON() macro was used. (Jani)
v8: Replace safe_conversion() with check_assign() (Kees)
Signed-off-by: Gwan-gyeong Mun <[email protected]>
Cc: Chris Wilson <[email protected]>
Cc: Matthew Auld <[email protected]>
Cc: Thomas Hellström <[email protected]>
Cc: Jani Nikula <[email protected]>
Reviewed-by: Nirmoy Das <[email protected]> (v2)
Reviewed-by: Mauro Carvalho Chehab <[email protected]> (v3)
Reported-by: kernel test robot <[email protected]>
Reviewed-by: Andrzej Hajda <[email protected]> (v5)
---
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 6 +++---
drivers/gpu/drm/i915/intel_region_ttm.c | 17 ++++++++++++++---
2 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index d708c4c2a9bd..615541b650fa 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -140,14 +140,14 @@ i915_ttm_place_from_region(const struct intel_memory_region *mr,
if (flags & I915_BO_ALLOC_CONTIGUOUS)
place->flags |= TTM_PL_FLAG_CONTIGUOUS;
if (offset != I915_BO_INVALID_OFFSET) {
- place->fpfn = offset >> PAGE_SHIFT;
- place->lpfn = place->fpfn + (size >> PAGE_SHIFT);
+ WARN_ON(check_assign(offset >> PAGE_SHIFT, &place->fpfn));
+ WARN_ON(check_assign(place->fpfn + (size >> PAGE_SHIFT), &place->lpfn));
} else if (mr->io_size && mr->io_size < mr->total) {
if (flags & I915_BO_ALLOC_GPU_ONLY) {
place->flags |= TTM_PL_FLAG_TOPDOWN;
} else {
place->fpfn = 0;
- place->lpfn = mr->io_size >> PAGE_SHIFT;
+ WARN_ON(check_assign(mr->io_size >> PAGE_SHIFT, &place->lpfn));
}
}
}
diff --git a/drivers/gpu/drm/i915/intel_region_ttm.c b/drivers/gpu/drm/i915/intel_region_ttm.c
index 575d67bc6ffe..37a964b20b36 100644
--- a/drivers/gpu/drm/i915/intel_region_ttm.c
+++ b/drivers/gpu/drm/i915/intel_region_ttm.c
@@ -209,14 +209,23 @@ intel_region_ttm_resource_alloc(struct intel_memory_region *mem,
if (flags & I915_BO_ALLOC_CONTIGUOUS)
place.flags |= TTM_PL_FLAG_CONTIGUOUS;
if (offset != I915_BO_INVALID_OFFSET) {
- place.fpfn = offset >> PAGE_SHIFT;
- place.lpfn = place.fpfn + (size >> PAGE_SHIFT);
+ if (WARN_ON(check_assign(offset >> PAGE_SHIFT, &place.fpfn))) {
+ ret = -E2BIG;
+ goto out;
+ }
+ if (WARN_ON(check_assign(place.fpfn + (size >> PAGE_SHIFT), &place.lpfn))) {
+ ret = -E2BIG;
+ goto out;
+ }
} else if (mem->io_size && mem->io_size < mem->total) {
if (flags & I915_BO_ALLOC_GPU_ONLY) {
place.flags |= TTM_PL_FLAG_TOPDOWN;
} else {
place.fpfn = 0;
- place.lpfn = mem->io_size >> PAGE_SHIFT;
+ if (WARN_ON(check_assign(mem->io_size >> PAGE_SHIFT, &place.lpfn))) {
+ ret = -E2BIG;
+ goto out;
+ }
}
}
@@ -224,6 +233,8 @@ intel_region_ttm_resource_alloc(struct intel_memory_region *mem,
mock_bo.bdev = &mem->i915->bdev;
ret = man->func->alloc(man, &mock_bo, &place, &res);
+
+out:
if (ret == -ENOSPC)
ret = -ENXIO;
if (!ret)
--
2.37.1
From: Chris Wilson <[email protected]>
Having addressed the issues surrounding incorrect types for local
variables and potential integer truncation in using the scatterlist API,
we have closed all the loop holes we had previously identified with
dangerously large object creation. As such, we can eliminate the warning
put in place to remind us to complete the review.
Signed-off-by: Chris Wilson <[email protected]>
Signed-off-by: Gwan-gyeong Mun <[email protected]>
Cc: Tvrtko Ursulin <[email protected]>
Cc: Brian Welty <[email protected]>
Cc: Matthew Auld <[email protected]>
Cc: Thomas Hellström <[email protected]>
Testcase: igt@gem_create@create-massive
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/4991
Reviewed-by: Nirmoy Das <[email protected]>
Reviewed-by: Mauro Carvalho Chehab <[email protected]>
Reviewed-by: Andrzej Hajda <[email protected]>
---
drivers/gpu/drm/i915/gem/i915_gem_object.h | 15 ---------------
1 file changed, 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index 0cf31adbfd41..dd2762da332f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -20,25 +20,10 @@
enum intel_region_id;
-/*
- * XXX: There is a prevalence of the assumption that we fit the
- * object's page count inside a 32bit _signed_ variable. Let's document
- * this and catch if we ever need to fix it. In the meantime, if you do
- * spot such a local variable, please consider fixing!
- *
- * We can check for invalidly typed locals with typecheck(), see for example
- * i915_gem_object_get_sg().
- */
-#define GEM_CHECK_SIZE_OVERFLOW(sz) \
- GEM_WARN_ON((sz) >> PAGE_SHIFT > INT_MAX)
-
static inline bool i915_gem_object_size_2big(u64 size)
{
struct drm_i915_gem_object *obj;
- if (GEM_CHECK_SIZE_OVERFLOW(size))
- return true;
-
if (overflows_type(size, obj->base.size))
return true;
--
2.37.1
On 24.08.2022 10:45, 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 truncaion (overflow)
> between different data types. And it adds check_assign() macro which
> performs an assigning source value into destination ptr along with an
> overflow check. overflow_type macro has been improved to handle the signbit
> by gcc's built-in overflow check function. And it adds overflows_ptr()
> helper macro for checking the overflows between a value and a pointer
> type/value.
>
> 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
> v8: Add check_assign() and remove safe_conversion() (Kees)
> Fix overflows_type() to use gcc's built-in overflow function (Andrzej)
> Add overflows_ptr() to allow overflow checking when assigning a value
> into a pointer variable (G.G.)
> v9: Fix overflows_type() to use __builtin_add_overflow() instead of
> __builtin_add_overflow_p() (Andrzej)
> Fix overflows_ptr() to use overflows_type() with the unsigned long type
> (Andrzej)
>
> Signed-off-by: Gwan-gyeong Mun <[email protected]>
> Cc: Thomas Hellström <[email protected]>
> Cc: Matthew Auld <[email protected]>
> Cc: Nirmoy Das <[email protected]>
> Cc: Jani Nikula <[email protected]>
> Cc: Andi Shyti <[email protected]>
> Cc: Andrzej Hajda <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Kees Cook <[email protected]>
> Reviewed-by: Mauro Carvalho Chehab <[email protected]> (v5)
> ---
>
Reviewed-by: Andrzej Hajda <[email protected]>
Regards
Andrzej
On Wed, Aug 24, 2022 at 05:45:07PM +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 truncaion (overflow)
> between different data types. And it adds check_assign() macro which
> performs an assigning source value into destination ptr along with an
> overflow check. overflow_type macro has been improved to handle the signbit
> by gcc's built-in overflow check function. And it adds overflows_ptr()
> helper macro for checking the overflows between a value and a pointer
> type/value.
>
> 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
> v8: Add check_assign() and remove safe_conversion() (Kees)
> Fix overflows_type() to use gcc's built-in overflow function (Andrzej)
> Add overflows_ptr() to allow overflow checking when assigning a value
> into a pointer variable (G.G.)
> v9: Fix overflows_type() to use __builtin_add_overflow() instead of
> __builtin_add_overflow_p() (Andrzej)
> Fix overflows_ptr() to use overflows_type() with the unsigned long type
> (Andrzej)
>
> Signed-off-by: Gwan-gyeong Mun <[email protected]>
> Cc: Thomas Hellstr?m <[email protected]>
> Cc: Matthew Auld <[email protected]>
> Cc: Nirmoy Das <[email protected]>
> Cc: Jani Nikula <[email protected]>
> Cc: Andi Shyti <[email protected]>
> Cc: Andrzej Hajda <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Kees Cook <[email protected]>
> Reviewed-by: Mauro Carvalho Chehab <[email protected]> (v5)
> ---
> drivers/gpu/drm/i915/i915_user_extensions.c | 3 +-
> drivers/gpu/drm/i915/i915_utils.h | 5 +-
> include/linux/overflow.h | 62 +++++++++++++++++++++
> 3 files changed, 64 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..6f6b5b910968 100644
> --- a/drivers/gpu/drm/i915/i915_user_extensions.c
> +++ b/drivers/gpu/drm/i915/i915_user_extensions.c
> @@ -50,8 +50,7 @@ int i915_user_extensions(struct i915_user_extension __user *ext,
> if (err)
> return err;
>
> - if (get_user(next, &ext->next_extension) ||
> - overflows_type(next, ext))
> + if (get_user(next, &ext->next_extension) || overflows_ptr(next))
> return -EFAULT;
>
> ext = u64_to_user_ptr(next);
I continue to dislike the layers of macros and specialization here.
This is just a fancy version of check_assign():
if (get_user(next, &ext->next_extension) || check_assign(next, &ext))
return -EFAULT;
However, the __builtin_*_overflow() family only wants to work on
integral types, so this needs to be slightly expanded:
uintptr_t kptr;
...
if (get_user(next, &ext->next_extension) || check_assign(next, &kptr))
return -EFAULT;
ext = (void * __user)kptr;
But, it does seem like the actual problem here is that u64_to_user_ptr()
is not performing the checking? It's used heavily in the drm code.
Is a check_assign_user_ptr() needed?
> 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..6af9df1d67a1 100644
> --- a/include/linux/overflow.h
> +++ b/include/linux/overflow.h
> @@ -52,6 +52,68 @@ static inline bool __must_check __must_check_overflow(bool overflow)
> return unlikely(overflow);
> }
>
> +/**
> + * overflows_type - helper for checking the overflows between data types or
> + * values
> + *
> + * @x: Source value or data type for overflow check
> + * @T: Destination value or data type for overflow check
> + *
> + * It compares the values or data type between the first and second argument to
> + * check whether overflow can occur when assigning the first argument to the
> + * variable of the second argument. Source and Destination can be singned or
> + * unsigned data types. Source and Destination can be different data types.
> + *
> + * Returns:
> + * True if overflow can occur, false otherwise.
> + */
> +#define overflows_type(x, T) __must_check_overflow(({ \
> + typeof(T) v = 0; \
> + __builtin_add_overflow((x), v, &v); \
> +}))
I'd like to avoid "externalizing" these kinds of checks when the better
path is to catch the issue at operation type (add, sub, mul, assign).
Looking at existing users, I see stuff like:
if (overflows_type(item.query_id - 1, unsigned long))
return -EINVAL;
func_idx = item.query_id - 1;
This requires too much open-coded checking, IMO. It's better as:
if (check_assign(item.query_id - 1, &func_idx))
return -EINVAL;
or other similar:
if (overflows_type(user->slice_mask, context->slice_mask) ||
...
context->slice_mask = user->slice_mask;
and some that don't make sense to me. Why check argument types? Is this
trying to avoid implicit type conversions?
So, if it's _really_ needed, I can live with adding overflows_type().
> +
> +/**
> + * overflows_ptr - helper for checking the occurrence of overflows when a value
> + * assigns to the pointer data type
> + *
> + * @x: Source value for overflow check
> + *
> + * gcc's built-in overflow check functions don't support checking between the
> + * pointer type and non-pointer type. And ILP32 and LP64 have the same bit size
> + * between long and pointer. Therefore it internally compares the source value
> + * and unsigned long data type for checking overflow.
> + *
> + * Returns:
> + * True if overflow can occur, false otherwise.
> + */
> +#define overflows_ptr(x) __must_check_overflow(overflows_type(x, unsigned long))
I'd rather not have this -- it's just a specialized use of
overflows_type(), and only used in 1 place.
> +
> +/**
> + * check_assign - perform an assigning source value into destination ptr along
> + * with an overflow check.
> + *
> + * @value: Source value
> + * @ptr: Destination pointer address, If the pointer type is not used,
> + * a warning message is output during build.
> + *
> + * It checks internally the ptr is a pointer type. And it uses gcc's built-in
> + * overflow check function.
> + * It does not use the check_*() wrapper functions, but directly uses gcc's
> + * built-in overflow check function so that it can be used even when
> + * the type of value and the type pointed to by ptr are different without build
> + * warning messages.
This is a good point: the check_{add,sub,mul}_overflow() helpers
currently require all the params be the same type, which rather limits
their usage. Perhaps this can be weakened now that we're not using[1]
the fallback logic any more? (Separate patch.)
> + *
> + * Returns:
> + * If the value would overflow the destination, it returns true. If not return
> + * false. When overflow does not occur, the assigning into ptr from value
> + * succeeds. It follows the return policy as other check_*_overflow() functions
> + * return non-zero as a failure.
> + */
> +#define check_assign(value, ptr) __must_check_overflow(({ \
> + typecheck_pointer(ptr); \
> + __builtin_add_overflow(0, value, ptr); \
> +}))
But yes, this looks correct. I really like it. :)
> +
> /*
> * For simplicity and code hygiene, the fallback code below insists on
> * a, b and *d having the same type (similar to the min() and max()
> --
> 2.37.1
>
-Kees
[1] 4eb6bd55cfb2 ("compiler.h: drop fallback overflow checkers")
--
Kees Cook
On 25.08.2022 18:47, Kees Cook wrote:
> On Wed, Aug 24, 2022 at 05:45:07PM +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 truncaion (overflow)
>> between different data types. And it adds check_assign() macro which
>> performs an assigning source value into destination ptr along with an
>> overflow check. overflow_type macro has been improved to handle the signbit
>> by gcc's built-in overflow check function. And it adds overflows_ptr()
>> helper macro for checking the overflows between a value and a pointer
>> type/value.
>>
>> 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
>> v8: Add check_assign() and remove safe_conversion() (Kees)
>> Fix overflows_type() to use gcc's built-in overflow function (Andrzej)
>> Add overflows_ptr() to allow overflow checking when assigning a value
>> into a pointer variable (G.G.)
>> v9: Fix overflows_type() to use __builtin_add_overflow() instead of
>> __builtin_add_overflow_p() (Andrzej)
>> Fix overflows_ptr() to use overflows_type() with the unsigned long type
>> (Andrzej)
>>
>> Signed-off-by: Gwan-gyeong Mun <[email protected]>
>> Cc: Thomas Hellström <[email protected]>
>> Cc: Matthew Auld <[email protected]>
>> Cc: Nirmoy Das <[email protected]>
>> Cc: Jani Nikula <[email protected]>
>> Cc: Andi Shyti <[email protected]>
>> Cc: Andrzej Hajda <[email protected]>
>> Cc: Mauro Carvalho Chehab <[email protected]>
>> Cc: Kees Cook <[email protected]>
>> Reviewed-by: Mauro Carvalho Chehab <[email protected]> (v5)
>> ---
>> drivers/gpu/drm/i915/i915_user_extensions.c | 3 +-
>> drivers/gpu/drm/i915/i915_utils.h | 5 +-
>> include/linux/overflow.h | 62 +++++++++++++++++++++
>> 3 files changed, 64 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..6f6b5b910968 100644
>> --- a/drivers/gpu/drm/i915/i915_user_extensions.c
>> +++ b/drivers/gpu/drm/i915/i915_user_extensions.c
>> @@ -50,8 +50,7 @@ int i915_user_extensions(struct i915_user_extension __user *ext,
>> if (err)
>> return err;
>>
>> - if (get_user(next, &ext->next_extension) ||
>> - overflows_type(next, ext))
>> + if (get_user(next, &ext->next_extension) || overflows_ptr(next))
>> return -EFAULT;
>>
>> ext = u64_to_user_ptr(next);
>
> I continue to dislike the layers of macros and specialization here.
> This is just a fancy version of check_assign():
>
> if (get_user(next, &ext->next_extension) || check_assign(next, &ext))
> return -EFAULT;
>
> However, the __builtin_*_overflow() family only wants to work on
> integral types, so this needs to be slightly expanded:
>
> uintptr_t kptr;
> ...
> if (get_user(next, &ext->next_extension) || check_assign(next, &kptr))
> return -EFAULT;
>
> ext = (void * __user)kptr;
>
> But, it does seem like the actual problem here is that u64_to_user_ptr()
> is not performing the checking? It's used heavily in the drm code.
>
> Is a check_assign_user_ptr() needed?
>
>> 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..6af9df1d67a1 100644
>> --- a/include/linux/overflow.h
>> +++ b/include/linux/overflow.h
>> @@ -52,6 +52,68 @@ static inline bool __must_check __must_check_overflow(bool overflow)
>> return unlikely(overflow);
>> }
>>
>> +/**
>> + * overflows_type - helper for checking the overflows between data types or
>> + * values
>> + *
>> + * @x: Source value or data type for overflow check
>> + * @T: Destination value or data type for overflow check
>> + *
>> + * It compares the values or data type between the first and second argument to
>> + * check whether overflow can occur when assigning the first argument to the
>> + * variable of the second argument. Source and Destination can be singned or
>> + * unsigned data types. Source and Destination can be different data types.
>> + *
>> + * Returns:
>> + * True if overflow can occur, false otherwise.
>> + */
>> +#define overflows_type(x, T) __must_check_overflow(({ \
>> + typeof(T) v = 0; \
>> + __builtin_add_overflow((x), v, &v); \
>> +}))
>
> I'd like to avoid "externalizing" these kinds of checks when the better
> path is to catch the issue at operation type (add, sub, mul, assign).
> Looking at existing users, I see stuff like:
>
> if (overflows_type(item.query_id - 1, unsigned long))
> return -EINVAL;
>
> func_idx = item.query_id - 1;
>
> This requires too much open-coded checking, IMO. It's better as:
>
> if (check_assign(item.query_id - 1, &func_idx))
> return -EINVAL;
>
> or other similar:
>
> if (overflows_type(user->slice_mask, context->slice_mask) ||
> ...
> context->slice_mask = user->slice_mask;
>
> and some that don't make sense to me. Why check argument types? Is this
> trying to avoid implicit type conversions?
>
> So, if it's _really_ needed, I can live with adding overflows_type().
>
>> +
>> +/**
>> + * overflows_ptr - helper for checking the occurrence of overflows when a value
>> + * assigns to the pointer data type
>> + *
>> + * @x: Source value for overflow check
>> + *
>> + * gcc's built-in overflow check functions don't support checking between the
>> + * pointer type and non-pointer type. And ILP32 and LP64 have the same bit size
>> + * between long and pointer. Therefore it internally compares the source value
>> + * and unsigned long data type for checking overflow.
>> + *
>> + * Returns:
>> + * True if overflow can occur, false otherwise.
>> + */
>> +#define overflows_ptr(x) __must_check_overflow(overflows_type(x, unsigned long))
>
> I'd rather not have this -- it's just a specialized use of
> overflows_type(), and only used in 1 place.
>
>> +
>> +/**
>> + * check_assign - perform an assigning source value into destination ptr along
>> + * with an overflow check.
>> + *
>> + * @value: Source value
>> + * @ptr: Destination pointer address, If the pointer type is not used,
>> + * a warning message is output during build.
>> + *
>> + * It checks internally the ptr is a pointer type. And it uses gcc's built-in
>> + * overflow check function.
>> + * It does not use the check_*() wrapper functions, but directly uses gcc's
>> + * built-in overflow check function so that it can be used even when
>> + * the type of value and the type pointed to by ptr are different without build
>> + * warning messages.
>
> This is a good point: the check_{add,sub,mul}_overflow() helpers
> currently require all the params be the same type, which rather limits
> their usage. Perhaps this can be weakened now that we're not using[1]
> the fallback logic any more? (Separate patch.)
>
>> + *
>> + * Returns:
>> + * If the value would overflow the destination, it returns true. If not return
>> + * false. When overflow does not occur, the assigning into ptr from value
>> + * succeeds. It follows the return policy as other check_*_overflow() functions
>> + * return non-zero as a failure.
>> + */
>> +#define check_assign(value, ptr) __must_check_overflow(({ \
>> + typecheck_pointer(ptr); \
>> + __builtin_add_overflow(0, value, ptr); \
>> +}))
>
> But yes, this looks correct. I really like it. :)
One more thing, I suspect __builtin_add_overflow checks already if ptr
is pointer, so typecheck_pointer seems redundant.
Regards
Andrzej
>
>> +
>> /*
>> * For simplicity and code hygiene, the fallback code below insists on
>> * a, b and *d having the same type (similar to the min() and max()
>> --
>> 2.37.1
>>
>
> -Kees
>
> [1] 4eb6bd55cfb2 ("compiler.h: drop fallback overflow checkers")
>
On 8/26/22 1:47 AM, Kees Cook wrote:
> On Wed, Aug 24, 2022 at 05:45:07PM +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 truncaion (overflow)
>> between different data types. And it adds check_assign() macro which
>> performs an assigning source value into destination ptr along with an
>> overflow check. overflow_type macro has been improved to handle the signbit
>> by gcc's built-in overflow check function. And it adds overflows_ptr()
>> helper macro for checking the overflows between a value and a pointer
>> type/value.
>>
>> 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
>> v8: Add check_assign() and remove safe_conversion() (Kees)
>> Fix overflows_type() to use gcc's built-in overflow function (Andrzej)
>> Add overflows_ptr() to allow overflow checking when assigning a value
>> into a pointer variable (G.G.)
>> v9: Fix overflows_type() to use __builtin_add_overflow() instead of
>> __builtin_add_overflow_p() (Andrzej)
>> Fix overflows_ptr() to use overflows_type() with the unsigned long type
>> (Andrzej)
>>
>> Signed-off-by: Gwan-gyeong Mun <[email protected]>
>> Cc: Thomas Hellström <[email protected]>
>> Cc: Matthew Auld <[email protected]>
>> Cc: Nirmoy Das <[email protected]>
>> Cc: Jani Nikula <[email protected]>
>> Cc: Andi Shyti <[email protected]>
>> Cc: Andrzej Hajda <[email protected]>
>> Cc: Mauro Carvalho Chehab <[email protected]>
>> Cc: Kees Cook <[email protected]>
>> Reviewed-by: Mauro Carvalho Chehab <[email protected]> (v5)
>> ---
>> drivers/gpu/drm/i915/i915_user_extensions.c | 3 +-
>> drivers/gpu/drm/i915/i915_utils.h | 5 +-
>> include/linux/overflow.h | 62 +++++++++++++++++++++
>> 3 files changed, 64 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..6f6b5b910968 100644
>> --- a/drivers/gpu/drm/i915/i915_user_extensions.c
>> +++ b/drivers/gpu/drm/i915/i915_user_extensions.c
>> @@ -50,8 +50,7 @@ int i915_user_extensions(struct i915_user_extension __user *ext,
>> if (err)
>> return err;
>>
>> - if (get_user(next, &ext->next_extension) ||
>> - overflows_type(next, ext))
>> + if (get_user(next, &ext->next_extension) || overflows_ptr(next))
>> return -EFAULT;
>>
>> ext = u64_to_user_ptr(next);
>
> I continue to dislike the layers of macros and specialization here.
> This is just a fancy version of check_assign():
>
> if (get_user(next, &ext->next_extension) || check_assign(next, &ext))
> return -EFAULT;
>
> However, the __builtin_*_overflow() family only wants to work on
> integral types, so this needs to be slightly expanded:
>
> uintptr_t kptr;
> ...
> if (get_user(next, &ext->next_extension) || check_assign(next, &kptr))
> return -EFAULT;
>
> ext = (void * __user)kptr;
>
> But, it does seem like the actual problem here is that u64_to_user_ptr()
> is not performing the checking? It's used heavily in the drm code.
>
> Is a check_assign_user_ptr() needed?
Hi Kees,
Yes, the reason that an additional overflow check is performed when
assigning a pointer type is that no overflow check is provided when
using u64_to_user_ptr())
I also agree with the explicit overflow check when assigning to pointer
type variables.
I'll add check_assign_user_ptr() to the new version of the patch and use it.
>
>> 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..6af9df1d67a1 100644
>> --- a/include/linux/overflow.h
>> +++ b/include/linux/overflow.h
>> @@ -52,6 +52,68 @@ static inline bool __must_check __must_check_overflow(bool overflow)
>> return unlikely(overflow);
>> }
>>
>> +/**
>> + * overflows_type - helper for checking the overflows between data types or
>> + * values
>> + *
>> + * @x: Source value or data type for overflow check
>> + * @T: Destination value or data type for overflow check
>> + *
>> + * It compares the values or data type between the first and second argument to
>> + * check whether overflow can occur when assigning the first argument to the
>> + * variable of the second argument. Source and Destination can be singned or
>> + * unsigned data types. Source and Destination can be different data types.
>> + *
>> + * Returns:
>> + * True if overflow can occur, false otherwise.
>> + */
>> +#define overflows_type(x, T) __must_check_overflow(({ \
>> + typeof(T) v = 0; \
>> + __builtin_add_overflow((x), v, &v); \
>> +}))
>
> I'd like to avoid "externalizing" these kinds of checks when the better
> path is to catch the issue at operation type (add, sub, mul, assign).
> Looking at existing users, I see stuff like:
>
I'll update overflows_type() with updated check_add_overflow()
( https://lore.kernel.org/all/202208311040.C6CA8253@keescook )
> if (overflows_type(item.query_id - 1, unsigned long))
> return -EINVAL;
>
> func_idx = item.query_id - 1;
>
> This requires too much open-coded checking, IMO. It's better as:
>
> if (check_assign(item.query_id - 1, &func_idx))
> return -EINVAL;
>
> or other similar:
>
> if (overflows_type(user->slice_mask, context->slice_mask) ||
> ...
> context->slice_mask = user->slice_mask;
>
> and some that don't make sense to me. Why check argument types? Is this
> trying to avoid implicit type conversions?
>
> So, if it's _really_ needed, I can live with adding overflows_type().
>
thanks for checking in detail.
Yes, it is thought that the above-mentioned codes can be modified using
check_assign(). And it seems that it can be applied as an additional
patch after check_assign() is applied.
And the code in the form below
if (overflows_type(size, obj->base.size))
return ERR_PTR(-E2BIG);
It seems that it can be fixed by updating type_max/type_min/__type_half_max.
#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))
if (size > type_max(obj->base.size))
return ERR_PTR(-E2BIG);
And when a type is used as an argument, it is used for comparison with
the size used internally (not directly exposed outside the data
structure or accessible through pointer referencing in multiple steps).
It seems that max_type can be used in these cases as well, but when it
is used to check before calling a function that actually allocates
memory using overflows_type(), it seems that readability( or intuitively
understood)can be good.
And in the alloc_table(), the function calls
"GEM_BUG_ON(overflows_type(count * PAGE_SIZE, sg->length));", it is a
case of checking and creating while looping. In this case,
overflows_type() seems convenient.
In all the confirmed cases by me, it is possible to avoid using
overflows_type() by changing or modifying the code in a different form.
In all the confirmed cases by me, it is possible to avoid using
overflows_type() by changing or modifying the code in a different form.
but there are also codes that can be easily or intuitively understood
when added, so in my opinion, it would be good to add overflows_type()
macro.
>> +
>> +/**
>> + * overflows_ptr - helper for checking the occurrence of overflows when a value
>> + * assigns to the pointer data type
>> + *
>> + * @x: Source value for overflow check
>> + *
>> + * gcc's built-in overflow check functions don't support checking between the
>> + * pointer type and non-pointer type. And ILP32 and LP64 have the same bit size
>> + * between long and pointer. Therefore it internally compares the source value
>> + * and unsigned long data type for checking overflow.
>> + *
>> + * Returns:
>> + * True if overflow can occur, false otherwise.
>> + */
>> +#define overflows_ptr(x) __must_check_overflow(overflows_type(x, unsigned long))
>
> I'd rather not have this -- it's just a specialized use of
> overflows_type(), and only used in 1 place.
>
okay I'll drop it.
>> +
>> +/**
>> + * check_assign - perform an assigning source value into destination ptr along
>> + * with an overflow check.
>> + *
>> + * @value: Source value
>> + * @ptr: Destination pointer address, If the pointer type is not used,
>> + * a warning message is output during build.
>> + *
>> + * It checks internally the ptr is a pointer type. And it uses gcc's built-in
>> + * overflow check function.
>> + * It does not use the check_*() wrapper functions, but directly uses gcc's
>> + * built-in overflow check function so that it can be used even when
>> + * the type of value and the type pointed to by ptr are different without build
>> + * warning messages.
>
> This is a good point: the check_{add,sub,mul}_overflow() helpers
> currently require all the params be the same type, which rather limits
> their usage. Perhaps this can be weakened now that we're not using[1]
> the fallback logic any more? (Separate patch.)
>
>> + *
>> + * Returns:
>> + * If the value would overflow the destination, it returns true. If not return
>> + * false. When overflow does not occur, the assigning into ptr from value
>> + * succeeds. It follows the return policy as other check_*_overflow() functions
>> + * return non-zero as a failure.
>> + */
>> +#define check_assign(value, ptr) __must_check_overflow(({ \
>> + typecheck_pointer(ptr); \
>> + __builtin_add_overflow(0, value, ptr); \
>> +}))
>
> But yes, this looks correct. I really like it. :)
>
>> +
>> /*
>> * For simplicity and code hygiene, the fallback code below insists on
>> * a, b and *d having the same type (similar to the min() and max()
>> --
>> 2.37.1
>>
>
> -Kees
>
> [1] 4eb6bd55cfb2 ("compiler.h: drop fallback overflow checkers")
>
On 8/26/22 10:44 PM, Andrzej Hajda wrote:
> On 25.08.2022 18:47, Kees Cook wrote:
>> On Wed, Aug 24, 2022 at 05:45:07PM +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 truncaion (overflow)
>>> between different data types. And it adds check_assign() macro which
>>> performs an assigning source value into destination ptr along with an
>>> overflow check. overflow_type macro has been improved to handle the
>>> signbit
>>> by gcc's built-in overflow check function. And it adds overflows_ptr()
>>> helper macro for checking the overflows between a value and a pointer
>>> type/value.
>>>
>>> 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
>>> v8: Add check_assign() and remove safe_conversion() (Kees)
>>> Fix overflows_type() to use gcc's built-in overflow function
>>> (Andrzej)
>>> Add overflows_ptr() to allow overflow checking when assigning a
>>> value
>>> into a pointer variable (G.G.)
>>> v9: Fix overflows_type() to use __builtin_add_overflow() instead of
>>> __builtin_add_overflow_p() (Andrzej)
>>> Fix overflows_ptr() to use overflows_type() with the unsigned
>>> long type
>>> (Andrzej)
>>>
>>> Signed-off-by: Gwan-gyeong Mun <[email protected]>
>>> Cc: Thomas Hellström <[email protected]>
>>> Cc: Matthew Auld <[email protected]>
>>> Cc: Nirmoy Das <[email protected]>
>>> Cc: Jani Nikula <[email protected]>
>>> Cc: Andi Shyti <[email protected]>
>>> Cc: Andrzej Hajda <[email protected]>
>>> Cc: Mauro Carvalho Chehab <[email protected]>
>>> Cc: Kees Cook <[email protected]>
>>> Reviewed-by: Mauro Carvalho Chehab <[email protected]> (v5)
>>> ---
>>> drivers/gpu/drm/i915/i915_user_extensions.c | 3 +-
>>> drivers/gpu/drm/i915/i915_utils.h | 5 +-
>>> include/linux/overflow.h | 62 +++++++++++++++++++++
>>> 3 files changed, 64 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..6f6b5b910968 100644
>>> --- a/drivers/gpu/drm/i915/i915_user_extensions.c
>>> +++ b/drivers/gpu/drm/i915/i915_user_extensions.c
>>> @@ -50,8 +50,7 @@ int i915_user_extensions(struct i915_user_extension
>>> __user *ext,
>>> if (err)
>>> return err;
>>> - if (get_user(next, &ext->next_extension) ||
>>> - overflows_type(next, ext))
>>> + if (get_user(next, &ext->next_extension) ||
>>> overflows_ptr(next))
>>> return -EFAULT;
>>> ext = u64_to_user_ptr(next);
>>
>> I continue to dislike the layers of macros and specialization here.
>> This is just a fancy version of check_assign():
>>
>> if (get_user(next, &ext->next_extension) || check_assign(next, &ext))
>> return -EFAULT;
>>
>> However, the __builtin_*_overflow() family only wants to work on
>> integral types, so this needs to be slightly expanded:
>>
>> uintptr_t kptr;
>> ...
>> if (get_user(next, &ext->next_extension) || check_assign(next,
>> &kptr))
>> return -EFAULT;
>>
>> ext = (void * __user)kptr;
>>
>> But, it does seem like the actual problem here is that u64_to_user_ptr()
>> is not performing the checking? It's used heavily in the drm code.
>>
>> Is a check_assign_user_ptr() needed?
>>
>>> 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..6af9df1d67a1 100644
>>> --- a/include/linux/overflow.h
>>> +++ b/include/linux/overflow.h
>>> @@ -52,6 +52,68 @@ static inline bool __must_check
>>> __must_check_overflow(bool overflow)
>>> return unlikely(overflow);
>>> }
>>> +/**
>>> + * overflows_type - helper for checking the overflows between data
>>> types or
>>> + * values
>>> + *
>>> + * @x: Source value or data type for overflow check
>>> + * @T: Destination value or data type for overflow check
>>> + *
>>> + * It compares the values or data type between the first and second
>>> argument to
>>> + * check whether overflow can occur when assigning the first
>>> argument to the
>>> + * variable of the second argument. Source and Destination can be
>>> singned or
>>> + * unsigned data types. Source and Destination can be different data
>>> types.
>>> + *
>>> + * Returns:
>>> + * True if overflow can occur, false otherwise.
>>> + */
>>> +#define overflows_type(x, T) __must_check_overflow(({ \
>>> + typeof(T) v = 0; \
>>> + __builtin_add_overflow((x), v, &v); \
>>> +}))
>>
>> I'd like to avoid "externalizing" these kinds of checks when the better
>> path is to catch the issue at operation type (add, sub, mul, assign).
>> Looking at existing users, I see stuff like:
>>
>> if (overflows_type(item.query_id - 1, unsigned long))
>> return -EINVAL;
>>
>> func_idx = item.query_id - 1;
>>
>> This requires too much open-coded checking, IMO. It's better as:
>>
>> if (check_assign(item.query_id - 1, &func_idx))
>> return -EINVAL;
>>
>> or other similar:
>>
>> if (overflows_type(user->slice_mask, context->slice_mask) ||
>> ...
>> context->slice_mask = user->slice_mask;
>>
>> and some that don't make sense to me. Why check argument types? Is this
>> trying to avoid implicit type conversions?
>>
>> So, if it's _really_ needed, I can live with adding overflows_type().
>>
>>> +
>>> +/**
>>> + * overflows_ptr - helper for checking the occurrence of overflows
>>> when a value
>>> + * assigns to the pointer data type
>>> + *
>>> + * @x: Source value for overflow check
>>> + *
>>> + * gcc's built-in overflow check functions don't support checking
>>> between the
>>> + * pointer type and non-pointer type. And ILP32 and LP64 have the
>>> same bit size
>>> + * between long and pointer. Therefore it internally compares the
>>> source value
>>> + * and unsigned long data type for checking overflow.
>>> + *
>>> + * Returns:
>>> + * True if overflow can occur, false otherwise.
>>> + */
>>> +#define overflows_ptr(x) __must_check_overflow(overflows_type(x,
>>> unsigned long))
>>
>> I'd rather not have this -- it's just a specialized use of
>> overflows_type(), and only used in 1 place.
>>
>>> +
>>> +/**
>>> + * check_assign - perform an assigning source value into destination
>>> ptr along
>>> + * with an overflow check.
>>> + *
>>> + * @value: Source value
>>> + * @ptr: Destination pointer address, If the pointer type is not used,
>>> + * a warning message is output during build.
>>> + *
>>> + * It checks internally the ptr is a pointer type. And it uses gcc's
>>> built-in
>>> + * overflow check function.
>>> + * It does not use the check_*() wrapper functions, but directly
>>> uses gcc's
>>> + * built-in overflow check function so that it can be used even when
>>> + * the type of value and the type pointed to by ptr are different
>>> without build
>>> + * warning messages.
>>
>> This is a good point: the check_{add,sub,mul}_overflow() helpers
>> currently require all the params be the same type, which rather limits
>> their usage. Perhaps this can be weakened now that we're not using[1]
>> the fallback logic any more? (Separate patch.)
>>
>>> + *
>>> + * Returns:
>>> + * If the value would overflow the destination, it returns true. If
>>> not return
>>> + * false. When overflow does not occur, the assigning into ptr from
>>> value
>>> + * succeeds. It follows the return policy as other
>>> check_*_overflow() functions
>>> + * return non-zero as a failure.
>>> + */
>>> +#define check_assign(value, ptr) __must_check_overflow(({ \
>>> + typecheck_pointer(ptr); \
>>> + __builtin_add_overflow(0, value, ptr); \
>>> +}))
>>
>> But yes, this looks correct. I really like it. :)
>
>
> One more thing, I suspect __builtin_add_overflow checks already if ptr
> is pointer, so typecheck_pointer seems redundant.
>
thanks for check in detail.
I'll remove redundant code and send new version.
> Regards
> Andrzej
>
>
>>
>>> +
>>> /*
>>> * For simplicity and code hygiene, the fallback code below insists on
>>> * a, b and *d having the same type (similar to the min() and max()
>>> --
>>> 2.37.1
>>>
>>
>> -Kees
>>
>> [1] 4eb6bd55cfb2 ("compiler.h: drop fallback overflow checkers")
>>
>