Re: [PATCH v6 5/7] drm/xe/vf: Add bounds checking for queried GGTT base and size

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Validation bypass and state corruption in `vf_get_ggtt_info()`.
- [Medium] Missing implementation of bounds checking.

Pre-existing issues:
- [High] Integer wrap-around vulnerability in GGTT bounds check.
--

--- Patch [5]: [PATCH v6 5/7] drm/xe/vf: Add bounds checking for queried GGTT base and size ---
commit 79b0ebbde860ecafbd9642c6e961750508666c39
Author: Satyanarayana K V P <[email protected]>

drm/xe/vf: Add bounds checking for queried GGTT base and size

This commit adds explicit alignment checks for the queried GGTT base
and size in the VF to protect against invalid configuration data from
the PF.

> drm/xe/vf: Add bounds checking for queried GGTT base and size
> 
> Add explicit bounds checks for GGTT base and size which can detect
> and reject invalid configuration data from a misconfigured or
> malfunctioning PF, preventing protocol violations and protecting VF
> initialization.

[Severity: Medium]
Does this patch implement bounds checking? 

The commit message states bounds checking is being added, but the code 
only appears to implement alignment checks using IS_ALIGNED(). It seems 
actual bounds verification (to ensure start and size do not exceed physical 
limits) is missing from this change.

> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> index 81d68fe5ad7cd..d805c064377c3 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
[ ... ]
> @@ -526,6 +527,18 @@ static int vf_get_ggtt_info(struct xe_gt *gt)
>  		return -EREMCHG;
>  	}
>  
> +	if (!IS_ALIGNED(start, alignment)) {
> +		xe_gt_sriov_err(gt, "Unaligned GGTT base: %llu, expected %llu\n",
> +				start, ALIGN(start, alignment));
> +		return -EINVAL;
> +	}
> +
> +	if (!IS_ALIGNED(size, alignment)) {
> +		xe_gt_sriov_err(gt, "Unaligned GGTT size %llu, expected %llu\n",
> +				size, ALIGN(size, alignment));
> +		return -EINVAL;
> +	}

[Severity: High]
Can untrusted input from the PF bypass these new alignment checks during
VF initialization?

Looking at vf_get_ggtt_info() earlier in this function:

	xe_tile_sriov_vf_ggtt_base_store(tile, start);
	ggtt_size = xe_tile_sriov_vf_ggtt(tile);
	if (!ggtt_size) {
		/*
		 * This function is called once during xe_guc_init_noalloc(),
		 * at which point ggtt_size = 0 and we have to initialize everything,
		 * and GGTT is not yet initialized.
		 *
		 * Return early as there's nothing to fixup.
		 */
		xe_tile_sriov_vf_ggtt_store(tile, size);
		return 0;
	}

Because ggtt_size is initially 0 during xe_guc_init_noalloc(), the function
will take this early return path and completely skip the newly added 
IS_ALIGNED() checks.

Additionally, if invalid parameters are sent during a post-migration update,
will the unaligned start address be cached in the tile state by the call to
xe_tile_sriov_vf_ggtt_base_store(tile, start) before these checks can reject
it?

[Severity: High]
This isn't a bug introduced by this patch, but does the bounds check in
xe_ggtt_init_early() have an integer wrap-around vulnerability?

If a misconfigured or malicious PF returns an extremely large ggtt_size
via the GuC KLV query (for example, 0xFFFFFFFFFFF00000), it looks like it
could bypass the checks:

drivers/gpu/drm/xe/xe_ggtt.c:xe_ggtt_init_early() {
    ...
	if (ggtt_start < wopcm ||
	    ggtt_start + ggtt_size > GUC_GGTT_TOP) {
    ...
}

Could the unprotected addition ggtt_start + ggtt_size wrap around the 64-bit
limit and evaluate to a small value? 

This would bypass the GUC_GGTT_TOP check and pass the massive size to 
drm_mm_init(), potentially allowing future allocations to generate offsets 
far beyond the valid MMIO region.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5