Re: [PATCH V2] drm/qxl: Fix out-of-bounds read in client monitors head access

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/qxl: use-after-free in qxl_output_get_client_head() due to concurrent reallocation
--

commit 81383457292a90ad6073bf06583b55f1415d8a86
Author: Julio Faracco <jcfaracco@gmail.com>

drm/qxl: Fix out-of-bounds read in client monitors head access

This commit factors out the client monitors configuration access into a new
qxl_output_get_client_head() helper function to prevent out-of-bounds reads
when the client reports fewer monitors than the default. It updates several
places, including the mode probe path, to use this new helper.

> diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
> index 63fc7c6a7f1a5..9be9ded8d1639 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -148,6 +148,21 @@ static int qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
>  	return status;
>  }
>  
> +/*
> + * Return the client monitors config head for @output, or NULL when the
> + * client monitors config is absent or does not cover this output's index.
> + */
> +static struct qxl_head *qxl_output_get_client_head(struct qxl_output *output)
> +{
> +	struct qxl_device *qdev = to_qxl(output->base.dev);
> +	struct qxl_monitors_config *cfg = qdev->client_monitors_config;
> +
> +	if (!cfg || output->index >= cfg->count)
> +		return NULL;
> +
> +	return &cfg->heads[output->index];
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this introduce a potential use-after-free?

If an unprivileged userspace process inside the guest repeatedly invokes the
DRM_IOCTL_MODE_GETCONNECTOR ioctl (which calls qxl_conn_detect() and
qxl_conn_get_modes()), it will read qdev->client_monitors_config into the
local cfg pointer here.

Meanwhile, a concurrent qxl_client_monitors_config_work_func worker thread
could be reallocating qdev->client_monitors_config in response to host
monitor configuration changes:

qxl_display_copy_rom_client_monitors_config() {
  ...
  if (qxl_alloc_client_monitors_config(qdev, num_monitors)) {
  ...
}

qxl_alloc_client_monitors_config() {
  ...
  kfree(qdev->client_monitors_config);
  qdev->client_monitors_config = NULL;
  ...
}

Since the worker thread frees the structure without holding
mode_config.connection_mutex or any other lock, could the worker free the
configuration after the reader fetches the pointer but before it accesses the
array?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008155034.55688-1-jcfaracco@gmail.com?part=1