Re: [PATCH v5 3/6] drm/xe/xe_ras: Add support to query page offline queue and list

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] xe: pages silently skipped if firmware returns pages_returned > XE_RAS_NUM_PAGES
--

commit 9d21f3b18f4dc78f0d5de890ccb72a1c2ba87da4
Author: Riana Tauro <riana.tauro@intel.com>

drm/xe/xe_ras: Add support to query page offline queue and list

This commit adds support to query the page offline list and queue from
firmware during module load. The page offline list command retrieves pages
already offlined by the firmware, while the queue command retrieves pages
pending to be offlined.

> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> index a3464857e84cc..de97e5341f944 100644
> --- a/drivers/gpu/drm/xe/xe_ras.c
> +++ b/drivers/gpu/drm/xe/xe_ras.c
[ ... ]
> @@ -343,6 +343,89 @@ static bool ras_counter_is_valid(struct xe_device *xe, struct xe_ras_error_class
> +static void process_offline_pages(struct xe_device *xe, u32 cmd, void *req, size_t req_size,
> +				  void *resp, size_t resp_size,
> +				  struct xe_ras_offline_common *common, bool notify_fw)
> +{
[ ... ]
> +	do {
> +		memset(resp, 0, resp_size);
> +
> +		if (list_req)
> +			list_req->index = count;
> +
> +		ret = xe_sysctrl_send_command(&xe->sc, &command, &rlen);
[ ... ]
> +		if (rlen != resp_size) {
> +			xe_log_err(xe, SYSCTRL, -EINVAL,
> +				   "unexpected page offline response length %zu (expected %zu), cmd=%#x\n",
> +				   rlen, resp_size, cmd);
> +			return;
> +		}
> +
> +		for (i = 0; i < common->pages_returned && i < XE_RAS_NUM_PAGES; i++)
> +			handle_page_offline(xe, common->page_addresses[i], notify_fw);
> +
> +		count += common->pages_returned;

[Severity: Medium]
Will this silently skip pages if the firmware returns a value for
common->pages_returned that is greater than XE_RAS_NUM_PAGES?

The driver correctly bounds the for loop array access with i < XE_RAS_NUM_PAGES
to avoid out-of-bounds reads if the firmware provides a maliciously or
erroneously high pages_returned value.

However, the driver then unconditionally adds the unbounded pages_returned
value to count. For list commands, the next iteration requests
list_req->index = count.

If pages_returned was greater than XE_RAS_NUM_PAGES, count advances too far,
silently skipping the pages that were omitted by the array bound check. Should
the driver either reject the response entirely with -EINVAL, or cap the count
increment to XE_RAS_NUM_PAGES?

> +		if (!common->pages_returned)
> +			break;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012054056.2272869-8-riana.tauro@intel.com?part=3