[PATCH v2] drm/xe: Fix out-of-bounds field write in MI_STORE_DATA_IMM

[Lucas De Marchi]

On Thu, Jun 26, 2025 at 05:08:22PM +0000, Yao, Jia wrote:
>Hi maintainers:
>
>Could you help merge it ?

Even after v3 there were still some s/0x1fe/MAX_PTE_PER_SDI/ as I
mentioned in my review.

CI results of v1 should still be valid. I amended the commit and pushed
to drm-xe-next.

[1/1] drm/xe: Fix out-of-bounds field write in MI_STORE_DATA_IMM
       commit: c038bdba98c9f6a36378044a9d4385531a194d3e

thanks,
Lucas De Marchi

>
>Thanks,
>Jia
>
>-----Original Message-----
>From: Lin, Shuicheng <shuicheng.lin at intel.com>
>Sent: Wednesday, June 11, 2025 2:18 AM
>To: Brost, Matthew <matthew.brost at intel.com>; Yao, Jia <jia.yao at intel.com>; intel-xe at lists.freedesktop.org
>Cc: Nguyen, Brian3 <brian3.nguyen at intel.com>; Zuo, Alex <alex.zuo at intel.com>; Auld, Matthew <matthew.auld at intel.com>; Patelczyk, Maciej <maciej.patelczyk at intel.com>
>Subject: RE: [PATCH v2] drm/xe: Fix out-of-bounds field write in MI_STORE_DATA_IMM
>
>Hi maintainers:
>Just to avoid this patch be missed, could you please help review and merge it?
>Thanks.
>
>Best Regards
>Shuicheng
>
>On Wed, May 28, 2025 2:26 AM Brost, Matthew wrote:
>> On Tue, May 27, 2025 at 04:30:50PM +0000, Jia Yao wrote:
>> > According to Bspec, bits 0~9 of MI_STORE_DATA_IMM must not exceed 0x3FE.
>> > The macro MI_SDI_NUM_QW(x) evaluates to 2 * x + 1, which means the
>> > condition 2 * x + 1 <= 0x3FE must be satisfied. Therefore, the
>> > maximum valid value for x is 0x1FE, not 0x1FF.
>> >
>> > v2
>> >  - Replace 0x1fe with macro MAX_PTE_PER_SDI (Auld, Matthew &
>> > Patelczyk, Maciej)
>> >
>> > Bspec: 60246
>> >
>> > Fixes: 9c44fd5f6e8a ("drm/xe: Add migrate layer functions for SVM
>> > support")
>> > Cc: Matthew Brost <matthew.brost at intel.com>
>>
>> Thanks for the fix:
>> Reviewed-by: Matthew Brost <matthew.brost at intel.com>
>>
>> > Cc: Brian3 Nguyen <brian3.nguyen at intel.com>
>> > Cc: Alex Zuo <alex.zuo at intel.com>
>> > Cc: Matthew Auld <matthew.auld at intel.com>
>> > Cc: Maciej Patelczyk <maciej.patelczyk at intel.com>
>> > Suggested-by: Shuicheng Lin <shuicheng.lin at intel.com>
>> > Signed-off-by: Jia Yao <jia.yao at intel.com>
>> > ---
>> >  drivers/gpu/drm/xe/xe_migrate.c | 8 ++++----
>> >  1 file changed, 4 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/xe/xe_migrate.c
>> > b/drivers/gpu/drm/xe/xe_migrate.c index 8f8e9fdfb2a8..1ede50780a14
>> > 100644
>> > --- a/drivers/gpu/drm/xe/xe_migrate.c
>> > +++ b/drivers/gpu/drm/xe/xe_migrate.c
>> > @@ -1555,13 +1555,13 @@ static u32 pte_update_cmd_size(u64 size)
>> >  	XE_WARN_ON(size > MAX_PREEMPTDISABLE_TRANSFER);
>> >  	/*
>> >  	 * MI_STORE_DATA_IMM command is used to update page table. Each
>> > -	 * instruction can update maximumly 0x1ff pte entries. To update
>> > -	 * n (n <= 0x1ff) pte entries, we need:
>> > +	 * instruction can update maximumly 0x1fe pte entries. To update
>> > +	 * n (n <= 0x1fe) pte entries, we need:
>> >  	 * 1 dword for the MI_STORE_DATA_IMM command header (opcode etc)
>> >  	 * 2 dword for the page table's physical location
>> >  	 * 2*n dword for value of pte to fill (each pte entry is 2 dwords)
>> >  	 */
>> > -	num_dword = (1 + 2) * DIV_U64_ROUND_UP(entries, 0x1ff);
>> > +	num_dword = (1 + 2) * DIV_U64_ROUND_UP(entries,
>> MAX_PTE_PER_SDI);
>> >  	num_dword += entries * 2;
>> >
>> >  	return num_dword;
>> > @@ -1577,7 +1577,7 @@ static void build_pt_update_batch_sram(struct
>> > xe_migrate *m,
>> >
>> >  	ptes = DIV_ROUND_UP(size, XE_PAGE_SIZE);
>> >  	while (ptes) {
>> > -		u32 chunk = min(0x1ffU, ptes);
>> > +		u32 chunk = min(MAX_PTE_PER_SDI, ptes);
>> >
>> >  		bb->cs[bb->len++] = MI_STORE_DATA_IMM |
>> MI_SDI_NUM_QW(chunk);
>> >  		bb->cs[bb->len++] = pt_offset;
>> > --
>> > 2.34.1
>> >