diff mbox series

[v13,5/9] drm/i915: Check for integer truncation on scatterlist creation

Message ID 20220928081300.101516-6-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

Commit Message

Gwan-gyeong Mun Sept. 28, 2022, 8:12 a.m. UTC 
From: Chris Wilson <chris@chris-wilson.co.uk>

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 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 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.
Failing that type check, we have a second check at sg_alloc_table time
to make sure the values we are passing into the scatterlist API are not
truncated.

It uses pgoff_t for locals that are dealing with page indices, in this
case, the page count is the limit of the page index.
And it uses safe_conversion() macro which performs a type conversion (cast)
of an integer value into a new variable, checking that the destination is
large enough to hold the source value.

v2: Move added i915_utils's macro into drm_util header (Jani N)
v5: Fix macros to be enclosed in parentheses for complex values
    Fix too long line warning
v8: Replace safe_conversion() with check_assign() (Kees)

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Brian Welty <brian.welty@intel.com>
Cc: Matthew Auld <matthew.auld@intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>
Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_internal.c |  6 ++++--
 drivers/gpu/drm/i915/gem/i915_gem_object.h   |  3 ---
 drivers/gpu/drm/i915/gem/i915_gem_phys.c     |  4 ++++
 drivers/gpu/drm/i915/gem/i915_gem_shmem.c    |  5 ++++-
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c      |  4 ++++
 drivers/gpu/drm/i915/gem/i915_gem_userptr.c  |  5 ++++-
 drivers/gpu/drm/i915/gvt/dmabuf.c            |  9 +++++----
 drivers/gpu/drm/i915/i915_scatterlist.h      | 11 +++++++++++
 8 files changed, 36 insertions(+), 11 deletions(-)

Comments

Jani Nikula Sept. 28, 2022, 8:51 a.m. UTC | #1 
On Wed, 28 Sep 2022, Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> wrote:
> diff --git a/drivers/gpu/drm/i915/i915_scatterlist.h b/drivers/gpu/drm/i915/i915_scatterlist.h
> index 9ddb3e743a3e..1d1802beb42b 100644
> --- a/drivers/gpu/drm/i915/i915_scatterlist.h
> +++ b/drivers/gpu/drm/i915/i915_scatterlist.h
> @@ -220,4 +220,15 @@ struct i915_refct_sgt *i915_rsgt_from_buddy_resource(struct ttm_resource *res,
>  						     u64 region_start,
>  						     u32 page_alignment);
>  
> +/* Wrap scatterlist.h to sanity check for integer truncation */
> +typedef unsigned int __sg_size_t; /* see linux/scatterlist.h */
> +#define sg_alloc_table(sgt, nents, gfp) \
> +	overflows_type(nents, __sg_size_t) ? -E2BIG \
> +		: ((sg_alloc_table)(sgt, (__sg_size_t)(nents), gfp))
> +
> +#define sg_alloc_table_from_pages_segment(sgt, pages, npages, offset, size, max_segment, gfp) \
> +	overflows_type(npages, __sg_size_t) ? -E2BIG \
> +		: ((sg_alloc_table_from_pages_segment)(sgt, pages, (__sg_size_t)(npages), offset, \
> +						       size, max_segment, gfp))
> +
>  #endif

No. I don't think we should shadow sg_alloc_table() and
sg_alloc_table_from_pages_segment().

Either get this in scatterlist.h (preferred) or prefix with i915_ or
whatever to indicate it's our local thing.

i915_scatterlist.h already has too much scatterlist "namespace" abuse
that I'd rather see gone than violated more.


BR,
Jani.
Linus Torvalds Sept. 28, 2022, 5:09 p.m. UTC | #2 
On Wed, Sep 28, 2022 at 1:15 AM Gwan-gyeong Mun
<gwan-gyeong.mun@intel.com> wrote:
>
> +       if (check_assign(obj->base.size >> PAGE_SHIFT, &npages))
> +               return -E2BIG;

I have to say, I find that new "check_assign()" macro use to be disgusting.

It's one thing to check for overflows.

It's another thing entirely to just assign something to a local variable.

This disgusting "let's check and assign" needs to die. It makes the
code a completely unreadable mess. The "user" wersion is even worse.

If you worry about overflow, then use a mix of

 (a) use a sufficiently large type to begin with

 (b) check for value range separately

and in this particular case, I also suspect that the whole range check
should have been somewhere else entirely - at the original creation of
that "obj" structure, not at one random end-point where it is used.

In other words, THIS WHOLE PATCH is just end-points checking the size
requirements of that "base.size" thing much too late, when it should
have been checked originally for some "maximum acceptable base size"
instead.

And that "maximum acceptable base size" should *not* be about "this is
the size of the variables we use". It should be a sanity check of
"this value is sane and fits in sane use cases".

Because "let's plug security checks" is most definitely not about
picking random assignments and saying "let's check this one". It's
about trying to catch things earlier than that.

Kees, you need to reign in the craziness in overflow.h.

                 Linus
Kees Cook Sept. 28, 2022, 6:06 p.m. UTC | #3 
On Wed, Sep 28, 2022 at 10:09:04AM -0700, Linus Torvalds wrote:
> Kees, you need to reign in the craziness in overflow.h.

Understood. I've been trying to help the drm folks walk a line between
having a bunch of custom macros hidden away in the drm includes and
building up generalized versions that are actually helpful beyond drm.
But I can see that it doesn't help to have a "do two things at the same
time" macro for the assignment checking.
Gwan-gyeong Mun Oct. 7, 2022, 4:38 p.m. UTC | #4 
On 9/28/22 11:51 AM, Jani Nikula wrote:
> On Wed, 28 Sep 2022, Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> wrote:
>> diff --git a/drivers/gpu/drm/i915/i915_scatterlist.h b/drivers/gpu/drm/i915/i915_scatterlist.h
>> index 9ddb3e743a3e..1d1802beb42b 100644
>> --- a/drivers/gpu/drm/i915/i915_scatterlist.h
>> +++ b/drivers/gpu/drm/i915/i915_scatterlist.h
>> @@ -220,4 +220,15 @@ struct i915_refct_sgt *i915_rsgt_from_buddy_resource(struct ttm_resource *res,
>>   						     u64 region_start,
>>   						     u32 page_alignment);
>>   
>> +/* Wrap scatterlist.h to sanity check for integer truncation */
>> +typedef unsigned int __sg_size_t; /* see linux/scatterlist.h */
>> +#define sg_alloc_table(sgt, nents, gfp) \
>> +	overflows_type(nents, __sg_size_t) ? -E2BIG \
>> +		: ((sg_alloc_table)(sgt, (__sg_size_t)(nents), gfp))
>> +
>> +#define sg_alloc_table_from_pages_segment(sgt, pages, npages, offset, size, max_segment, gfp) \
>> +	overflows_type(npages, __sg_size_t) ? -E2BIG \
>> +		: ((sg_alloc_table_from_pages_segment)(sgt, pages, (__sg_size_t)(npages), offset, \
>> +						       size, max_segment, gfp))
>> +
>>   #endif
> 
> No. I don't think we should shadow sg_alloc_table() and
> sg_alloc_table_from_pages_segment().
> 
> Either get this in scatterlist.h (preferred) or prefix with i915_ or
> whatever to indicate it's our local thing.
> 
> i915_scatterlist.h already has too much scatterlist "namespace" abuse
> that I'd rather see gone than violated more.
> 
> 
Hi Jani,
Thanks for you comments.

I will update this patch by removing the shadowing of 
sg_alloc_table_from_pages_segment() / sg_alloc_table(), so that the 
caller checks when overflow checking is required.

Br,
G.G.

> BR,
> Jani.
> 
> 
>
Gwan-gyeong Mun Oct. 7, 2022, 4:40 p.m. UTC | #5 
Linus and Kees, I also understood that I should not make and use the 
macro that performs assignment and checking at the same time, and I will 
drop it and update it.

Kees, the overflows_type() macro had several updates as input from you 
and the community, and there is an advantage when moving to Linux common.
What are your thoughts on continuing to add the overflows_type() macro 
to overflows.h?

Br,

G.G.

On 9/28/22 9:06 PM, Kees Cook wrote:
> On Wed, Sep 28, 2022 at 10:09:04AM -0700, Linus Torvalds wrote:
>> Kees, you need to reign in the craziness in overflow.h.
> 
> Understood. I've been trying to help the drm folks walk a line between
> having a bunch of custom macros hidden away in the drm includes and
> building up generalized versions that are actually helpful beyond drm.
> But I can see that it doesn't help to have a "do two things at the same
> time" macro for the assignment checking.
>
Gwan-gyeong Mun Oct. 7, 2022, 4:45 p.m. UTC | #6 
On 9/28/22 8:09 PM, Linus Torvalds wrote:
> On Wed, Sep 28, 2022 at 1:15 AM Gwan-gyeong Mun
> <gwan-gyeong.mun@intel.com> wrote:
>>
>> +       if (check_assign(obj->base.size >> PAGE_SHIFT, &npages))
>> +               return -E2BIG;
> 
> I have to say, I find that new "check_assign()" macro use to be disgusting.
> 
> It's one thing to check for overflows.
> 
> It's another thing entirely to just assign something to a local variable.
> 
> This disgusting "let's check and assign" needs to die. It makes the
> code a completely unreadable mess. The "user" wersion is even worse.
> 
> If you worry about overflow, then use a mix of
> 
>   (a) use a sufficiently large type to begin with
> 
>   (b) check for value range separately
> 
> and in this particular case, I also suspect that the whole range check
> should have been somewhere else entirely - at the original creation of
> that "obj" structure, not at one random end-point where it is used.
> 
> In other words, THIS WHOLE PATCH is just end-points checking the size
> requirements of that "base.size" thing much too late, when it should
> have been checked originally for some "maximum acceptable base size"
> instead.
> 
> And that "maximum acceptable base size" should *not* be about "this is
> the size of the variables we use". It should be a sanity check of
> "this value is sane and fits in sane use cases".
> 
> Because "let's plug security checks" is most definitely not about
> picking random assignments and saying "let's check this one". It's
> about trying to catch things earlier than that.
Linus, but the size check of the object in the i915 is already done at 
the time of creation.
And this patch series is designed to prevent problems that may arise 
from the difference between the data structure used internally by 
drm/i915/ttm (unsigned long) and the data structure provided and used by 
the scatter/getter api (unsigned int).

The current implementation of the i915 uses sg_table / scatterlist to 
manage and use memory resources at a low level.
When creating an object of i915, it is based on drm_gem_object, which is 
the data structure of drm. The size of object is size_t [1][2].
And i915 uses ttm. the ttm_resource_manager manages resources with 
ttm_resource structure [3] for resource management.
When creating sgt with sg_alloc_table()[4] in i915, size of struct 
drm_gem_object[1] and num_pages of struct ttm_resource[3] are used as 
nents arguments.
(Of course, there are places that explicitly use unsigned int variables.)
Even where sg_alloc_table_from_pages_segment() [5] is used, there are 
places where the size of struct drm_gem_object [1] is used as the 
n_pages argument after bit shift operation with PAGE_SHIFT.

As above, when using drm, ttm, sgt infrastructure in i915, there is a 
type mismatch in size in the driver implementation.

Because the types are different, when assigning a value from a large 
type variable to a small type variable, if the overflow check is used as 
a safety guard in i915 before sgt api call, implicit value truncation 
will not occur when a problem occurs. The log output makes it easy to 
detect that a problem has already occurred before sgt apis are called.
When a bug related to this issue occurs, it will not delay the reporting 
of the problem of this issue.

Because the above one is one of a workaround solution, if the types used 
in the scatter/getter api would be changed to such as size_t or another 
configurable type, it would be a more proper solution. But it might 
affect lots of drivers and frameworks. therefore I suggest a current 
solution before the changing of sgt area.


Br,

G.G.


[1]
     struct drm_gem_object {
     ...
         size_t size;
     ...

[2]
     #ifndef __kernel_size_t
     #if __BITS_PER_LONG != 64
     typedef unsigned int    __kernel_size_t;
     #else
     typedef __kernel_ulong_t __kernel_size_t;

     typedef __kernel_size_t        size_t;

[3]
     struct ttm_resource {
         unsigned long start;
         unsigned long num_pages;
         uint32_t mem_type;
         uint32_t placement;
         struct ttm_bus_placement bus;
         struct ttm_buffer_object *bo;

         /**
          * @lru: Least recently used list, see &ttm_resource_manager.lru
          */
         struct list_head lru;
     };


[4] int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t 
gfp_mask)

[5] int sg_alloc_table_from_pages_segment(struct sg_table *sgt, struct 
page **pages,
                 unsigned int n_pages, unsigned int offset,
                 unsigned long size, unsigned int max_segment,
                 gfp_t gfp_mask)



> 
> Kees, you need to reign in the craziness in overflow.h.
> 
>                   Linus
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
index c698f95af15f..53fa27e1c950 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
@@ -37,10 +37,13 @@  static int i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj)
 	struct sg_table *st;
 	struct scatterlist *sg;
 	unsigned int sg_page_sizes;
-	unsigned int npages;
+	pgoff_t npages; /* restricted by sg_alloc_table */
 	int max_order;
 	gfp_t gfp;
 
+	if (check_assign(obj->base.size >> PAGE_SHIFT, &npages))
+		return -E2BIG;
+
 	max_order = MAX_ORDER;
 #ifdef CONFIG_SWIOTLB
 	if (is_swiotlb_active(obj->base.dev->dev)) {
@@ -67,7 +70,6 @@  static int i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj)
 	if (!st)
 		return -ENOMEM;
 
-	npages = obj->base.size / PAGE_SIZE;
 	if (sg_alloc_table(st, npages, GFP_KERNEL)) {
 		kfree(st);
 		return -ENOMEM;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index 26e7f86dbed9..9f8e29112c31 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -26,9 +26,6 @@  enum intel_region_id;
  * this and catch if we ever need to fix it. In the meantime, if you do
  * spot such a local variable, please consider fixing!
  *
- * Aside from our own locals (for which we have no excuse!):
- * - sg_table embeds unsigned int for nents
- *
  * We can check for invalidly typed locals with typecheck(), see for example
  * i915_gem_object_get_sg().
  */
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
index 0d0e46dae559..88ba7266a3a5 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
@@ -28,6 +28,10 @@  static int i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj)
 	void *dst;
 	int i;
 
+	/* Contiguous chunk, with a single scatterlist element */
+	if (overflows_type(obj->base.size, sg->length))
+		return -E2BIG;
+
 	if (GEM_WARN_ON(i915_gem_object_needs_bit17_swizzle(obj)))
 		return -EINVAL;
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index f42ca1179f37..339b0a9cf2d0 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -193,13 +193,16 @@  static int shmem_get_pages(struct drm_i915_gem_object *obj)
 	struct drm_i915_private *i915 = to_i915(obj->base.dev);
 	struct intel_memory_region *mem = obj->mm.region;
 	struct address_space *mapping = obj->base.filp->f_mapping;
-	const unsigned long page_count = obj->base.size / PAGE_SIZE;
 	unsigned int max_segment = i915_sg_segment_size();
 	struct sg_table *st;
 	struct sgt_iter sgt_iter;
+	pgoff_t page_count;
 	struct page *page;
 	int ret;
 
+	if (check_assign(obj->base.size >> PAGE_SHIFT, &page_count))
+		return -E2BIG;
+
 	/*
 	 * Assert that the object is not currently in any GPU domain. As it
 	 * wasn't in the GTT, there shouldn't be any way it could have been in
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 0f9dd790ceed..8d7c392d335c 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -792,6 +792,10 @@  static int i915_ttm_get_pages(struct drm_i915_gem_object *obj)
 {
 	struct ttm_place requested, busy[I915_TTM_MAX_PLACEMENTS];
 	struct ttm_placement placement;
+	pgoff_t num_pages;
+
+	if (check_assign(obj->base.size >> PAGE_SHIFT, &num_pages))
+		return -E2BIG;
 
 	GEM_BUG_ON(obj->mm.n_placements > I915_TTM_MAX_PLACEMENTS);
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index 8423df021b71..48237e443863 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -128,13 +128,16 @@  static void i915_gem_object_userptr_drop_ref(struct drm_i915_gem_object *obj)
 
 static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
 {
-	const unsigned long num_pages = obj->base.size >> PAGE_SHIFT;
 	unsigned int max_segment = i915_sg_segment_size();
 	struct sg_table *st;
 	unsigned int sg_page_sizes;
 	struct page **pvec;
+	pgoff_t num_pages; /* limited by sg_alloc_table_from_pages_segment */
 	int ret;
 
+	if (check_assign(obj->base.size >> PAGE_SHIFT, &num_pages))
+		return -E2BIG;
+
 	st = kmalloc(sizeof(*st), GFP_KERNEL);
 	if (!st)
 		return -ENOMEM;
diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c b/drivers/gpu/drm/i915/gvt/dmabuf.c
index 01e54b45c5c1..bc6e823584ad 100644
--- a/drivers/gpu/drm/i915/gvt/dmabuf.c
+++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
@@ -42,8 +42,7 @@ 
 
 #define GEN8_DECODE_PTE(pte) (pte & GENMASK_ULL(63, 12))
 
-static int vgpu_gem_get_pages(
-		struct drm_i915_gem_object *obj)
+static int vgpu_gem_get_pages(struct drm_i915_gem_object *obj)
 {
 	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
 	struct intel_vgpu *vgpu;
@@ -52,7 +51,10 @@  static int vgpu_gem_get_pages(
 	int i, j, ret;
 	gen8_pte_t __iomem *gtt_entries;
 	struct intel_vgpu_fb_info *fb_info;
-	u32 page_num;
+	pgoff_t page_num;
+
+	if (check_assign(obj->base.size >> PAGE_SHIFT, &page_num))
+		return -E2BIG;
 
 	fb_info = (struct intel_vgpu_fb_info *)obj->gvt_info;
 	if (drm_WARN_ON(&dev_priv->drm, !fb_info))
@@ -66,7 +68,6 @@  static int vgpu_gem_get_pages(
 	if (unlikely(!st))
 		return -ENOMEM;
 
-	page_num = obj->base.size >> PAGE_SHIFT;
 	ret = sg_alloc_table(st, page_num, GFP_KERNEL);
 	if (ret) {
 		kfree(st);
diff --git a/drivers/gpu/drm/i915/i915_scatterlist.h b/drivers/gpu/drm/i915/i915_scatterlist.h
index 9ddb3e743a3e..1d1802beb42b 100644
--- a/drivers/gpu/drm/i915/i915_scatterlist.h
+++ b/drivers/gpu/drm/i915/i915_scatterlist.h
@@ -220,4 +220,15 @@  struct i915_refct_sgt *i915_rsgt_from_buddy_resource(struct ttm_resource *res,
 						     u64 region_start,
 						     u32 page_alignment);
 
+/* Wrap scatterlist.h to sanity check for integer truncation */
+typedef unsigned int __sg_size_t; /* see linux/scatterlist.h */
+#define sg_alloc_table(sgt, nents, gfp) \
+	overflows_type(nents, __sg_size_t) ? -E2BIG \
+		: ((sg_alloc_table)(sgt, (__sg_size_t)(nents), gfp))
+
+#define sg_alloc_table_from_pages_segment(sgt, pages, npages, offset, size, max_segment, gfp) \
+	overflows_type(npages, __sg_size_t) ? -E2BIG \
+		: ((sg_alloc_table_from_pages_segment)(sgt, pages, (__sg_size_t)(npages), offset, \
+						       size, max_segment, gfp))
+
 #endif