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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxx> 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



[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux