Re: [PATCH v2 2/2] drm/xe/display: Use xe_bo_addr_iter in remapped DPT pinning

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] drm/xe/display: 32-bit overflow in write_dpt_remapped_tiled() size calculation
--

commit 061485566c342a900f9594f9a2b0fa400ea6f8c5
Author: Maarten Lankhorst <dev@lankhorst.se>

drm/xe/display: Use xe_bo_addr_iter in remapped DPT pinning

This patch replaces per-page xe_bo_addr() lookups in
write_dpt_remapped_linear() and write_dpt_remapped_tiled() with
xe_bo_addr_iter_first() and xe_bo_addr_iter_addr() cursor iteration to
reduce pinning complexity from O(N^2) to O(N).

> diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> index 15fbd3fe6cbb7..7d53f77308a46 100644
> --- a/drivers/gpu/drm/xe/display/xe_fb_pin.c
> +++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> @@ -17,6 +17,7 @@
>  #include "xe_ggtt.h"
>  #include "xe_pat.h"
>  #include "xe_pm.h"
> +#include "xe_res_cursor.h"
>  #include "xe_vram_types.h"
>  
>  static void
> @@ -68,14 +69,14 @@ write_dpt_remapped_linear(struct xe_bo *bo, struct iosys_map *map,
>  	const u64 pte = xe_ggtt_encode_pte_flags(ggtt, bo,
>  						 xe_cache_pat_idx(xe, XE_CACHE_NONE));
>  	unsigned int offset = plane->offset * XE_PAGE_SIZE;
> -	unsigned int size = plane->size;
> +	unsigned int size = plane->size * XE_PAGE_SIZE;
> +	struct xe_res_cursor cur;
>  
> -	while (size--) {
> -		u64 addr = xe_bo_addr(bo, offset, XE_PAGE_SIZE);
> +	for (xe_bo_addr_iter_first(bo, &cur, offset, size); cur.remaining; xe_res_next(&cur, XE_PAGE_SIZE)) {
> +		u64 addr = xe_bo_addr_iter_addr(&cur);
>  
>  		iosys_map_wr(map, dest, u64, addr | pte);
>  		dest += sizeof(u64);
> -		offset += XE_PAGE_SIZE;
>  	}
>  
>  	return dest;
> @@ -90,20 +91,22 @@ write_dpt_remapped_tiled(struct xe_bo *bo, struct iosys_map *map,
>  	struct xe_ggtt *ggtt = xe_device_get_root_tile(xe)->mem.ggtt;
>  	const u64 pte = xe_ggtt_encode_pte_flags(ggtt, bo,
>  						 xe_cache_pat_idx(xe, XE_CACHE_NONE));
> -	unsigned int offset, column, row;
> +	u32 offset = plane->offset * XE_PAGE_SIZE;
> +	struct xe_res_cursor cur;
>  
> -	for (row = 0; row < plane->height; row++) {
> -		offset = (plane->offset + plane->src_stride * row) *
> -			 XE_PAGE_SIZE;
> -
> -		for (column = 0; column < plane->width; column++) {
> -			u64 addr = xe_bo_addr(bo, offset, XE_PAGE_SIZE);
> +	xe_bo_addr_iter_first(bo, &cur, offset, plane->height * plane->src_stride * XE_PAGE_SIZE);
> +	while (cur.remaining) {

[Severity: Critical]
Can the size calculation in write_dpt_remapped_tiled() overflow a 32-bit
signed int and cause an out-of-bounds write past the end of map?

In include/drm/intel/gtt_view_types.h, struct intel_remapped_plane_info
defines height and src_stride as u16:

include/drm/intel/gtt_view_types.h:
	struct intel_remapped_plane_info {
		/* in gtt pages */
		u32 offset:31;
		u32 linear:1;
		union {
			/* in gtt pages for !linear */
			struct {
				u16 width;
				u16 height;
				u16 src_stride;
				u16 dst_stride;
			};

			/* in gtt pages for linear */
			u32 size;
		};
	} __packed;

And in drivers/gpu/drm/xe/xe_bo.h, XE_PAGE_SIZE is a signed int expression:

	#define XE_PTE_SHIFT			12
	#define XE_PAGE_SIZE			(1 << XE_PTE_SHIFT)

In write_dpt_remapped_tiled(), plane->height and plane->src_stride undergo
standard C integer promotion from u16 to 32-bit signed int, so the product
plane->height * plane->src_stride * XE_PAGE_SIZE is evaluated in 32-bit
signed arithmetic before being converted to u64 for xe_bo_addr_iter_first().

When userspace pins a maximum-dimension tiled framebuffer via
drm_mode_addfb2 -> intel_framebuffer_init() -> intel_fill_fb_info() and
xe_fb_pin_to_ggtt() -> xe_pin_fb_vma_dpt() -> write_dpt_remapped_tiled()
(for example, a 16384x16384 4-tiled or Y-tiled plane with a 128 KiB pitch
where plane->height is 512 and plane->src_stride is 1024, totaling 524,288
pages or 2 GiB), the multiplication 512 * 1024 * 4096 overflows INT_MAX to
-0x80000000 and sign-extends to u64 as 0xffffffff80000000ULL.

Because the outer loop condition was changed from row < plane->height to
while (cur.remaining), cur.remaining starts at 0xffffffff80000000ULL and does
not reach zero after all plane->height rows are written. This causes the loop
to overrun the BO resource cursor in xe_res_next() and write PTEs past the
end of the DPT iosys_map via iosys_map_wr().

Similarly, in both write_dpt_remapped_linear() and
write_dpt_remapped_tiled(), multiplying plane->offset and plane->size by
XE_PAGE_SIZE in 32-bit unsigned int / u32 variables truncates byte offsets
and sizes at or above 4 GiB. Should these byte offset and size calculations
use u64 arithmetic (or mul_u32_u32() / a 64-bit page size constant)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023114616.735366-1-dev@lankhorst.se?part=2