Re: [PATCH RESEND 34/83] drm/qxl: Switch to drm_mode_config_create_initial_state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/qxl: missing monitors object cleanup and UAF in qxl_modeset_init()
--

commit c9a17dafeda120436479a0975b3829acefce4188
Author: Maxime Ripard <mripard@kernel.org>

drm/qxl: Switch to drm_mode_config_create_initial_state

This commit replaces drm_mode_config_reset() with
drm_mode_config_create_initial_state() in qxl_modeset_init() to handle initial
KMS state creation at probe time.

> diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
> index 204c47d613732..7f719e200d317 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -1312,8 +1312,7 @@ int qxl_modeset_init(struct qxl_device *qdev)
>  	if (ret)
>  		return ret;
>  
> -	drm_mode_config_reset(&qdev->ddev);
> -	return 0;
> +	return drm_mode_config_create_initial_state(&qdev->ddev);
>  }

[Severity: High]
If drm_mode_config_create_initial_state() fails and returns an error here, can
this cause a use-after-free of qdev->monitors_config_bo and an unmapped MMIO
write to qdev->ram_header->monitors_config during probe teardown?

Earlier in qxl_modeset_init(), qxl_create_monitors_object() allocates, pins,
and vmaps qdev->monitors_config_bo, sets qdev->ram_header->monitors_config,
and allocates qdev->dumb_heads.

When qxl_modeset_init() returns an error to qxl_pci_probe(), the error path
jumps past qxl_modeset_fini() directly to the unload label:

drivers/gpu/drm/qxl/qxl_drv.c:qxl_pci_probe() {
	...
	ret = qxl_modeset_init(qdev);
	if (ret)
		goto unload;
	...
poll_fini:
	drm_kms_helper_poll_fini(&qdev->ddev);
	qxl_modeset_fini(qdev);
unload:
	qxl_device_fini(qdev);
}

In qxl_device_fini(), qxl_gem_fini() -> qxl_bo_force_delete() force-frees
qdev->monitors_config_bo while it is still pinned and vmapped (triggering
WARN_ON_ONCE(bo->map_count > 0) in qxl_ttm_bo_destroy() and leaving the
qdev->monitors_config_bo pointer non-NULL), and iounmap() unmaps
qdev->ram_header.

When qxl_pci_probe() then returns the error code, devres cleanup for
devm_drm_dev_alloc() runs qxl_drm_release() -> qxl_modeset_fini() ->
qxl_destroy_monitors_object():

drivers/gpu/drm/qxl/qxl_display.c:qxl_destroy_monitors_object() {
	if (!qdev->monitors_config_bo)
		return 0;
	...
	qdev->ram_header->monitors_config = 0;

	ret = qxl_bo_vunmap_and_unpin(qdev->monitors_config_bo);
	if (ret)
		return ret;

	qxl_bo_unref(&qdev->monitors_config_bo);
	return 0;
}

Because qdev->monitors_config_bo is still non-NULL, should qxl_modeset_init()
call qxl_destroy_monitors_object(qdev) on failure before returning?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-drm-no-more-config-reset-v1-0-1f58d1e33380@kernel.org?part=34