by Gwan-gyeong Mun

[permalink] [raw]

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

On 9/28/22 8:09 PM, Linus Torvalds wrote:
> On Wed, Sep 28, 2022 at 1:15 AM Gwan-gyeong Mun
> <[email protected]> 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