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 <[email protected]> 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/[email protected]?part=1
