Commit f47e6306 authored by Chris Wilson's avatar Chris Wilson Committed by Rodrigo Vivi
Browse files

drm/i915/gem: Typecheck page lookups 



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. Be pedantic and add integer
typechecking to the lookup so that we can be sure that we are safe.
And it also uses pgoff_t as our page lookups must remain compatible with
the page cache, pgoff_t is currently exactly unsigned long.

v2: Move added i915_utils's macro into drm_util header (Jani N)
v3: 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.
v4: Add kernel-doc markups to the kAPI functions and macros (Mauoro)
v5: Fix an alignment to match open parenthesis
v6: Rebase
v10: Use assert_typable instead of exactly_pgoff_t() macro. (Kees)
v11: Change the use of assert_typable to assert_same_typable (G.G)
v12: Change to use static_assert(__castable_to_type(n ,T)) style since
     the assert_same_typable() macro has been dropped. (G.G)
v13: Change the use of __castable_to_type() to castable_to_type()
     Remove an unnecessary header include line. (G.G)
v16: Fix "ERROR:SPACING" Checkpatch report (G.G)

Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Matthew Auld <matthew.auld@intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
Co-developed-by: default avatarGwan-gyeong Mun <gwan-gyeong.mun@intel.com>
Signed-off-by: default avatarGwan-gyeong Mun <gwan-gyeong.mun@intel.com>
Reviewed-by: Nirmoy Das <nirmoy.das@intel.com> (v2)
Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org> (v3)
Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> (v5)
Signed-off-by: default avatarRodrigo Vivi <rodrigo.vivi@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20221228192252.917299-2-gwan-gyeong.mun@intel.com
parent b501d4dc

drivers/gpu/drm/i915/gem/i915_gem_object.c

+5 −2
Original line number Diff line number Diff line
@@ -427,10 +427,11 @@ void __i915_gem_object_invalidate_frontbuffer(struct drm_i915_gem_object *obj, 
static void

i915_gem_object_read_from_page_kmap(struct drm_i915_gem_object *obj, u64 offset, void *dst, int size)

{

	pgoff_t idx = offset >> PAGE_SHIFT;

	void *src_map;

	void *src_ptr;



	src_map = kmap_atomic(i915_gem_object_get_page(obj, offset >> PAGE_SHIFT));

	src_map = kmap_atomic(i915_gem_object_get_page(obj, idx));



	src_ptr = src_map + offset_in_page(offset);

	if (!(obj->cache_coherent & I915_BO_CACHE_COHERENT_FOR_READ))
@@ -443,9 +444,10 @@ i915_gem_object_read_from_page_kmap(struct drm_i915_gem_object *obj, u64 offset, 
static void

i915_gem_object_read_from_page_iomap(struct drm_i915_gem_object *obj, u64 offset, void *dst, int size)

{

	pgoff_t idx = offset >> PAGE_SHIFT;

	dma_addr_t dma = i915_gem_object_get_dma_address(obj, idx);

	void __iomem *src_map;

	void __iomem *src_ptr;

	dma_addr_t dma = i915_gem_object_get_dma_address(obj, offset >> PAGE_SHIFT);



	src_map = io_mapping_map_wc(&obj->mm.region->iomap,

				    dma - obj->mm.region->region.start,
@@ -484,6 +486,7 @@ static bool object_has_mappable_iomem(struct drm_i915_gem_object *obj) 
 */

int i915_gem_object_read_from_page(struct drm_i915_gem_object *obj, u64 offset, void *dst, int size)

{

	GEM_BUG_ON(overflows_type(offset >> PAGE_SHIFT, pgoff_t));

	GEM_BUG_ON(offset >= obj->base.size);

	GEM_BUG_ON(offset_in_page(offset) > PAGE_SIZE - size);

	GEM_BUG_ON(!i915_gem_object_has_pinned_pages(obj));

drivers/gpu/drm/i915/gem/i915_gem_object.h

+270 −23
Original line number Diff line number Diff line
@@ -27,8 +27,10 @@ enum intel_region_id; 
 * 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 num_pages

 * - get_user_pages*() mixed ints with longs

 * - sg_table embeds unsigned int for nents

 *

 * 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)
@@ -363,44 +365,289 @@ i915_gem_object_get_tile_row_size(const struct drm_i915_gem_object *obj) 
int i915_gem_object_set_tiling(struct drm_i915_gem_object *obj,

			       unsigned int tiling, unsigned int stride);



/**

 * __i915_gem_object_page_iter_get_sg - helper to find the target scatterlist

 * pointer and the target page position using pgoff_t n input argument and

 * i915_gem_object_page_iter

 * @obj: i915 GEM buffer object

 * @iter: i915 GEM buffer object page iterator

 * @n: page offset

 * @offset: searched physical offset,

 *          it will be used for returning physical page offset value

 *

 * Context: Takes and releases the mutex lock of the i915_gem_object_page_iter.

 *          Takes and releases the RCU lock to search the radix_tree of

 *          i915_gem_object_page_iter.

 *

 * Returns:

 * The target scatterlist pointer and the target page position.

 *

 * Recommended to use wrapper macro: i915_gem_object_page_iter_get_sg()

 */

struct scatterlist *

__i915_gem_object_get_sg(struct drm_i915_gem_object *obj,

__i915_gem_object_page_iter_get_sg(struct drm_i915_gem_object *obj,

				   struct i915_gem_object_page_iter *iter,

			 unsigned int n,

			 unsigned int *offset, bool dma);

				   pgoff_t  n,

				   unsigned int *offset);



/**

 * i915_gem_object_page_iter_get_sg - wrapper macro for

 * __i915_gem_object_page_iter_get_sg()

 * @obj: i915 GEM buffer object

 * @it: i915 GEM buffer object page iterator

 * @n: page offset

 * @offset: searched physical offset,

 *          it will be used for returning physical page offset value

 *

 * Context: Takes and releases the mutex lock of the i915_gem_object_page_iter.

 *          Takes and releases the RCU lock to search the radix_tree of

 *          i915_gem_object_page_iter.

 *

 * Returns:

 * The target scatterlist pointer and the target page position.

 *

 * In order to avoid the truncation of the input parameter, it checks the page

 * offset n's type from the input parameter before calling

 * __i915_gem_object_page_iter_get_sg().

 */

#define i915_gem_object_page_iter_get_sg(obj, it, n, offset) ({	\

	static_assert(castable_to_type(n, pgoff_t));		\

	__i915_gem_object_page_iter_get_sg(obj, it, n, offset);	\

})



/**

 * __i915_gem_object_get_sg - helper to find the target scatterlist

 * pointer and the target page position using pgoff_t n input argument and

 * drm_i915_gem_object. It uses an internal shmem scatterlist lookup function.

 * @obj: i915 GEM buffer object

 * @n: page offset

 * @offset: searched physical offset,

 *          it will be used for returning physical page offset value

 *

 * It uses drm_i915_gem_object's internal shmem scatterlist lookup function as

 * i915_gem_object_page_iter and calls __i915_gem_object_page_iter_get_sg().

 *

 * Returns:

 * The target scatterlist pointer and the target page position.

 *

 * Recommended to use wrapper macro: i915_gem_object_get_sg()

 * See also __i915_gem_object_page_iter_get_sg()

 */

static inline struct scatterlist *

i915_gem_object_get_sg(struct drm_i915_gem_object *obj,

		       unsigned int n,

__i915_gem_object_get_sg(struct drm_i915_gem_object *obj, pgoff_t n,

			 unsigned int *offset)

{

	return __i915_gem_object_get_sg(obj, &obj->mm.get_page, n, offset, false);

	return __i915_gem_object_page_iter_get_sg(obj, &obj->mm.get_page, n, offset);

}



/**

 * i915_gem_object_get_sg - wrapper macro for __i915_gem_object_get_sg()

 * @obj: i915 GEM buffer object

 * @n: page offset

 * @offset: searched physical offset,

 *          it will be used for returning physical page offset value

 *

 * Returns:

 * The target scatterlist pointer and the target page position.

 *

 * In order to avoid the truncation of the input parameter, it checks the page

 * offset n's type from the input parameter before calling

 * __i915_gem_object_get_sg().

 * See also __i915_gem_object_page_iter_get_sg()

 */

#define i915_gem_object_get_sg(obj, n, offset) ({	\

	static_assert(castable_to_type(n, pgoff_t));	\

	__i915_gem_object_get_sg(obj, n, offset);	\

})



/**

 * __i915_gem_object_get_sg_dma - helper to find the target scatterlist

 * pointer and the target page position using pgoff_t n input argument and

 * drm_i915_gem_object. It uses an internal DMA mapped scatterlist lookup function

 * @obj: i915 GEM buffer object

 * @n: page offset

 * @offset: searched physical offset,

 *          it will be used for returning physical page offset value

 *

 * It uses drm_i915_gem_object's internal DMA mapped scatterlist lookup function

 * as i915_gem_object_page_iter and calls __i915_gem_object_page_iter_get_sg().

 *

 * Returns:

 * The target scatterlist pointer and the target page position.

 *

 * Recommended to use wrapper macro: i915_gem_object_get_sg_dma()

 * See also __i915_gem_object_page_iter_get_sg()

 */

static inline struct scatterlist *

i915_gem_object_get_sg_dma(struct drm_i915_gem_object *obj,

			   unsigned int n,

__i915_gem_object_get_sg_dma(struct drm_i915_gem_object *obj, pgoff_t n,

			     unsigned int *offset)

{

	return __i915_gem_object_get_sg(obj, &obj->mm.get_dma_page, n, offset, true);

	return __i915_gem_object_page_iter_get_sg(obj, &obj->mm.get_dma_page, n, offset);

}



/**

 * i915_gem_object_get_sg_dma - wrapper macro for __i915_gem_object_get_sg_dma()

 * @obj: i915 GEM buffer object

 * @n: page offset

 * @offset: searched physical offset,

 *          it will be used for returning physical page offset value

 *

 * Returns:

 * The target scatterlist pointer and the target page position.

 *

 * In order to avoid the truncation of the input parameter, it checks the page

 * offset n's type from the input parameter before calling

 * __i915_gem_object_get_sg_dma().

 * See also __i915_gem_object_page_iter_get_sg()

 */

#define i915_gem_object_get_sg_dma(obj, n, offset) ({	\

	static_assert(castable_to_type(n, pgoff_t));	\

	__i915_gem_object_get_sg_dma(obj, n, offset);	\

})



/**

 * __i915_gem_object_get_page - helper to find the target page with a page offset

 * @obj: i915 GEM buffer object

 * @n: page offset

 *

 * It uses drm_i915_gem_object's internal shmem scatterlist lookup function as

 * i915_gem_object_page_iter and calls __i915_gem_object_page_iter_get_sg()

 * internally.

 *

 * Returns:

 * The target page pointer.

 *

 * Recommended to use wrapper macro: i915_gem_object_get_page()

 * See also __i915_gem_object_page_iter_get_sg()

 */

struct page *

i915_gem_object_get_page(struct drm_i915_gem_object *obj,

			 unsigned int n);

__i915_gem_object_get_page(struct drm_i915_gem_object *obj, pgoff_t n);



/**

 * i915_gem_object_get_page - wrapper macro for __i915_gem_object_get_page

 * @obj: i915 GEM buffer object

 * @n: page offset

 *

 * Returns:

 * The target page pointer.

 *

 * In order to avoid the truncation of the input parameter, it checks the page

 * offset n's type from the input parameter before calling

 * __i915_gem_object_get_page().

 * See also __i915_gem_object_page_iter_get_sg()

 */

#define i915_gem_object_get_page(obj, n) ({		\

	static_assert(castable_to_type(n, pgoff_t));	\

	__i915_gem_object_get_page(obj, n);		\

})



/**

 * __i915_gem_object_get_dirty_page - helper to find the target page with a page

 * offset

 * @obj: i915 GEM buffer object

 * @n: page offset

 *

 * It works like i915_gem_object_get_page(), but it marks the returned page dirty.

 *

 * Returns:

 * The target page pointer.

 *

 * Recommended to use wrapper macro: i915_gem_object_get_dirty_page()

 * See also __i915_gem_object_page_iter_get_sg() and __i915_gem_object_get_page()

 */

struct page *

i915_gem_object_get_dirty_page(struct drm_i915_gem_object *obj,

			       unsigned int n);

__i915_gem_object_get_dirty_page(struct drm_i915_gem_object *obj, pgoff_t n);



/**

 * i915_gem_object_get_dirty_page - wrapper macro for __i915_gem_object_get_dirty_page

 * @obj: i915 GEM buffer object

 * @n: page offset

 *

 * Returns:

 * The target page pointer.

 *

 * In order to avoid the truncation of the input parameter, it checks the page

 * offset n's type from the input parameter before calling

 * __i915_gem_object_get_dirty_page().

 * See also __i915_gem_object_page_iter_get_sg() and __i915_gem_object_get_page()

 */

#define i915_gem_object_get_dirty_page(obj, n) ({	\

	static_assert(castable_to_type(n, pgoff_t));	\

	__i915_gem_object_get_dirty_page(obj, n);	\

})



/**

 * __i915_gem_object_get_dma_address_len - helper to get bus addresses of

 * targeted DMA mapped scatterlist from i915 GEM buffer object and it's length

 * @obj: i915 GEM buffer object

 * @n: page offset

 * @len: DMA mapped scatterlist's DMA bus addresses length to return

 *

 * Returns:

 * Bus addresses of targeted DMA mapped scatterlist

 *

 * Recommended to use wrapper macro: i915_gem_object_get_dma_address_len()

 * See also __i915_gem_object_page_iter_get_sg() and __i915_gem_object_get_sg_dma()

 */

dma_addr_t

i915_gem_object_get_dma_address_len(struct drm_i915_gem_object *obj,

				    unsigned long n,

__i915_gem_object_get_dma_address_len(struct drm_i915_gem_object *obj, pgoff_t n,

				      unsigned int *len);



/**

 * i915_gem_object_get_dma_address_len - wrapper macro for

 * __i915_gem_object_get_dma_address_len

 * @obj: i915 GEM buffer object

 * @n: page offset

 * @len: DMA mapped scatterlist's DMA bus addresses length to return

 *

 * Returns:

 * Bus addresses of targeted DMA mapped scatterlist

 *

 * In order to avoid the truncation of the input parameter, it checks the page

 * offset n's type from the input parameter before calling

 * __i915_gem_object_get_dma_address_len().

 * See also __i915_gem_object_page_iter_get_sg() and

 * __i915_gem_object_get_dma_address_len()

 */

#define i915_gem_object_get_dma_address_len(obj, n, len) ({	\

	static_assert(castable_to_type(n, pgoff_t));		\

	__i915_gem_object_get_dma_address_len(obj, n, len);	\

})



/**

 * __i915_gem_object_get_dma_address - helper to get bus addresses of

 * targeted DMA mapped scatterlist from i915 GEM buffer object

 * @obj: i915 GEM buffer object

 * @n: page offset

 *

 * Returns:

 * Bus addresses of targeted DMA mapped scatterlis

 *

 * Recommended to use wrapper macro: i915_gem_object_get_dma_address()

 * See also __i915_gem_object_page_iter_get_sg() and __i915_gem_object_get_sg_dma()

 */

dma_addr_t

i915_gem_object_get_dma_address(struct drm_i915_gem_object *obj,

				unsigned long n);

__i915_gem_object_get_dma_address(struct drm_i915_gem_object *obj, pgoff_t n);



/**

 * i915_gem_object_get_dma_address - wrapper macro for

 * __i915_gem_object_get_dma_address

 * @obj: i915 GEM buffer object

 * @n: page offset

 *

 * Returns:

 * Bus addresses of targeted DMA mapped scatterlist

 *

 * In order to avoid the truncation of the input parameter, it checks the page

 * offset n's type from the input parameter before calling

 * __i915_gem_object_get_dma_address().

 * See also __i915_gem_object_page_iter_get_sg() and

 * __i915_gem_object_get_dma_address()

 */

#define i915_gem_object_get_dma_address(obj, n) ({	\

	static_assert(castable_to_type(n, pgoff_t));	\

	__i915_gem_object_get_dma_address(obj, n);	\

})



void __i915_gem_object_set_pages(struct drm_i915_gem_object *obj,

				 struct sg_table *pages);

drivers/gpu/drm/i915/gem/i915_gem_pages.c

+13 −14
Original line number Diff line number Diff line
@@ -521,14 +521,16 @@ void __i915_gem_object_release_map(struct drm_i915_gem_object *obj) 
}



struct scatterlist *

__i915_gem_object_get_sg(struct drm_i915_gem_object *obj,

__i915_gem_object_page_iter_get_sg(struct drm_i915_gem_object *obj,

				   struct i915_gem_object_page_iter *iter,

			 unsigned int n,

			 unsigned int *offset,

			 bool dma)

				   pgoff_t n,

				   unsigned int *offset)



{

	struct scatterlist *sg;

	const bool dma = iter == &obj->mm.get_dma_page ||

			 iter == &obj->ttm.get_io_page;

	unsigned int idx, count;

	struct scatterlist *sg;



	might_sleep();

	GEM_BUG_ON(n >= obj->base.size >> PAGE_SHIFT);
@@ -636,7 +638,7 @@ __i915_gem_object_get_sg(struct drm_i915_gem_object *obj, 
}



struct page *

i915_gem_object_get_page(struct drm_i915_gem_object *obj, unsigned int n)

__i915_gem_object_get_page(struct drm_i915_gem_object *obj, pgoff_t n)

{

	struct scatterlist *sg;

	unsigned int offset;
@@ -649,8 +651,7 @@ i915_gem_object_get_page(struct drm_i915_gem_object *obj, unsigned int n) 


/* Like i915_gem_object_get_page(), but mark the returned page dirty */

struct page *

i915_gem_object_get_dirty_page(struct drm_i915_gem_object *obj,

			       unsigned int n)

__i915_gem_object_get_dirty_page(struct drm_i915_gem_object *obj, pgoff_t n)

{

	struct page *page;
@@ -662,9 +663,8 @@ i915_gem_object_get_dirty_page(struct drm_i915_gem_object *obj, 
}



dma_addr_t

i915_gem_object_get_dma_address_len(struct drm_i915_gem_object *obj,

				    unsigned long n,

				    unsigned int *len)

__i915_gem_object_get_dma_address_len(struct drm_i915_gem_object *obj,

				      pgoff_t n, unsigned int *len)

{

	struct scatterlist *sg;

	unsigned int offset;
@@ -678,8 +678,7 @@ i915_gem_object_get_dma_address_len(struct drm_i915_gem_object *obj, 
}



dma_addr_t

i915_gem_object_get_dma_address(struct drm_i915_gem_object *obj,

				unsigned long n)

__i915_gem_object_get_dma_address(struct drm_i915_gem_object *obj, pgoff_t n)

{

	return i915_gem_object_get_dma_address_len(obj, n, NULL);

}

drivers/gpu/drm/i915/gem/i915_gem_ttm.c

+1 −1
Original line number Diff line number Diff line
@@ -689,7 +689,7 @@ static unsigned long i915_ttm_io_mem_pfn(struct ttm_buffer_object *bo, 
	GEM_WARN_ON(bo->ttm);



	base = obj->mm.region->iomap.base - obj->mm.region->region.start;

	sg = __i915_gem_object_get_sg(obj, &obj->ttm.get_io_page, page_offset, &ofs, true);

	sg = i915_gem_object_page_iter_get_sg(obj, &obj->ttm.get_io_page, page_offset, &ofs);



	return ((base + sg_dma_address(sg)) >> PAGE_SHIFT) + ofs;

}

drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c

+7 −5
Original line number Diff line number Diff line
@@ -469,7 +469,8 @@ static int gpu_fill(struct intel_context *ce, 
static int cpu_fill(struct drm_i915_gem_object *obj, u32 value)

{

	const bool has_llc = HAS_LLC(to_i915(obj->base.dev));

	unsigned int n, m, need_flush;

	unsigned int need_flush;

	unsigned long n, m;

	int err;



	i915_gem_object_lock(obj, NULL);
@@ -499,7 +500,8 @@ static int cpu_fill(struct drm_i915_gem_object *obj, u32 value) 
static noinline int cpu_check(struct drm_i915_gem_object *obj,

			      unsigned int idx, unsigned int max)

{

	unsigned int n, m, needs_flush;

	unsigned int needs_flush;

	unsigned long n;

	int err;



	i915_gem_object_lock(obj, NULL);
@@ -508,7 +510,7 @@ static noinline int cpu_check(struct drm_i915_gem_object *obj, 
		goto out_unlock;



	for (n = 0; n < real_page_count(obj); n++) {

		u32 *map;

		u32 *map, m;



		map = kmap_atomic(i915_gem_object_get_page(obj, n));

		if (needs_flush & CLFLUSH_BEFORE)
@@ -516,7 +518,7 @@ static noinline int cpu_check(struct drm_i915_gem_object *obj, 


		for (m = 0; m < max; m++) {

			if (map[m] != m) {

				pr_err("%pS: Invalid value at object %d page %d/%ld, offset %d/%d: found %x expected %x\n",

				pr_err("%pS: Invalid value at object %d page %ld/%ld, offset %d/%d: found %x expected %x\n",

				       __builtin_return_address(0), idx,

				       n, real_page_count(obj), m, max,

				       map[m], m);
@@ -527,7 +529,7 @@ static noinline int cpu_check(struct drm_i915_gem_object *obj, 


		for (; m < DW_PER_PAGE; m++) {

			if (map[m] != STACK_MAGIC) {

				pr_err("%pS: Invalid value at object %d page %d, offset %d: found %x expected %x (uninitialised)\n",

				pr_err("%pS: Invalid value at object %d page %ld, offset %d: found %x expected %x (uninitialised)\n",

				       __builtin_return_address(0), idx, n, m,

				       map[m], STACK_MAGIC);

				err = -EINVAL;