Re: [PATCH v1] drm/xe: Register drm device at the end of probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] drm/xe: drm_dev_unplug() before xe_display_unregister() causes panel UAF
- [High] drm/xe: xe_oa_register() fails before drm_dev_register() adds minor kdev
- [High] drm/xe: xe_debugfs_register() uses NULL debugfs_root before drm_dev_register()
- [High] drm/xe: xe_display_register() runs before drm_dev_register() in probe
- [High] drm/xe: drm_dev_unplug() runs on unregistered device in xe_pci_remove()
--

commit 59e43ed89ab7b80b15dcc4432f5bc6716fd8b83a
Author: Raag Jadav <raag.jadav@intel.com>

drm/xe: Register drm device at the end of probe

This commit moves drm_dev_register() from xe_device_probe() to the end of
__xe_pci_probe() and moves drm_dev_unplug() from xe_device_remove() to the
beginning of xe_pci_remove().

> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index e1979bbcf7bc6..ad25e9833283b 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -1049,10 +1049,6 @@ int xe_device_probe(struct xe_device *xe)
>  	if (err)
>  		return err;
>  
> -	err = drm_dev_register(&xe->drm, 0);
> -	if (err)
> -		return err;
> -
>  	xe_display_register(xe);

[Severity: High]
With drm_dev_register() moved to the end of __xe_pci_probe(),
xe_display_register() in drivers/gpu/drm/xe/xe_device.c now runs before
drm_dev_register(). Can xe_display_register() safely execute before the DRM
device and its connectors are registered?

In drivers/gpu/drm/i915/display/intel_display_driver.c,
xe_display_register() -> intel_display_driver_register() calls both
intel_acpi_video_register() and intel_fbdev_setup():

    intel_opregion_register(display);
    intel_acpi_video_register(display);
    ...
    intel_fbdev_setup(display);

In drivers/gpu/drm/clients/drm_fbdev_client.c, intel_fbdev_setup() ->
drm_client_setup_with_color_mode() -> drm_fbdev_client_setup() checks
whether the DRM device has been registered:

    drm_WARN(dev, !dev->registered, "Device has not been registered.\n");

Because dev->registered is only set in drm_dev_register(), this triggers a
warning during probe and runs fbdev setup before drm_modeset_register_all()
has invoked the connector late_register callbacks (which initialize panels,
DP AUX, and LSPCON).

In addition, intel_acpi_video_register() in
drivers/gpu/drm/i915/display/intel_acpi.c checks whether a native backlight
device was registered:

    drm_for_each_connector_iter(connector, &conn_iter) {
        struct intel_panel *panel = &to_intel_connector(connector)->panel;

        if (panel->backlight.funcs && !panel->backlight.device) {
            acpi_video_register_backlight();
            break;
        }
    }

Because panel->backlight.device is only populated during
drm_dev_register() -> drm_modeset_register_all() ->
intel_connector_register() -> intel_panel_register(), running
intel_acpi_video_register() first finds panel->backlight.device == NULL and
registers acpi_video_register_backlight() even when a native panel backlight
is present.

Furthermore, later in xe_device_probe(), xe_ras_process_errors() can call
xe_device_declare_wedged() -> drm_dev_wedged_event() when boot-time RAS
errors are present, which fails with -EINVAL in kobject_uevent_env() on
&dev->primary->kdev->kobj before drm_dev_register() has added primary->kdev.

>  
>  	err = xe_oa_register(xe);

[Severity: High]
Does xe_oa_register() fail with -ENOMEM during probe when called before
drm_dev_register()?

In drivers/gpu/drm/xe/xe_oa.c, xe_oa_register() creates a sysfs subdirectory
under xe->drm.primary->kdev->kobj:

    oa->metrics_kobj = kobject_create_and_add("metrics",
                                              &xe->drm.primary->kdev->kobj);
    if (!oa->metrics_kobj)
        return -ENOMEM;

During xe_device_create() -> drm_dev_init() -> drm_minor_alloc() ->
drm_sysfs_minor_alloc(), xe->drm.primary->kdev is only initialized with
device_initialize(), so primary->kdev->kobj.sd is NULL until
drm_dev_register() -> drm_minor_register() calls device_add(minor->kdev) in
drivers/gpu/drm/drm_drv.c.

When xe_oa_register() runs in xe_device_probe() prior to drm_dev_register(),
sysfs_create_dir_ns() sees a NULL parent kernfs_node and fails with -ENOENT,
causing kobject_create_and_add() to return NULL and aborting probe on any
OA-capable device.

[ ... ]

> @@ -1116,8 +1110,6 @@ void xe_device_remove(struct xe_device *xe)
>  {
>  	xe_display_unregister(xe);
>  
> -	drm_dev_unplug(&xe->drm);
> -
>  	xe_bo_pci_dev_remove_all(xe);
>  }
>  

[Severity: Critical]
By moving drm_dev_unplug(&xe->drm) out of xe_device_remove() to the start of
xe_pci_remove(), drm_dev_unplug() now runs before xe_display_unregister().
Does this cause a use-after-free on panel->base during display shutdown on
devices with an active eDP or DSI panel?

During drm_dev_register() -> drm_modeset_register_all(), eDP and DSI
connectors allocate panel->base using __devm_drm_panel_alloc() bound to
connector->base.kdev.

When xe_pci_remove() calls drm_dev_unplug() -> drm_dev_unregister() ->
drm_modeset_unregister_all() -> drm_connector_unregister(),
drm_sysfs_connector_remove() calls device_unregister(connector->kdev). That
releases devres on connector->kdev and frees the struct drm_panel while
panel->base remains non-NULL.

Subsequently, xe_pci_remove() calls xe_device_remove() ->
xe_display_unregister() -> intel_display_driver_unregister() in
drivers/gpu/drm/i915/display/intel_display_driver.c:

    drm_kms_helper_poll_fini(display->drm);

    intel_display_driver_disable_user_access(display);

    intel_audio_deinit(display);

    drm_atomic_helper_shutdown(display->drm);

When drm_atomic_helper_shutdown() disables an active eDP or DSI output,
intel_panel_unprepare() in drivers/gpu/drm/i915/display/intel_panel.c calls
drm_panel_unprepare(panel->base), which in drivers/gpu/drm/drm_panel.c
dereferences the freed panel and locks panel->follower_lock:

    if (!panel->prepared) {
        dev_warn(panel->dev, "Skipping unprepare of already unprepared panel\n");
        return;
    }

    mutex_lock(&panel->follower_lock);

> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index 50e4920e8e08c..a549f05e150c9 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -1113,6 +1113,8 @@ static void xe_pci_remove(struct pci_dev *pdev)
>  {
>  	struct xe_device *xe = pdev_to_xe_device(pdev);
>  
> +	drm_dev_unplug(&xe->drm);
> +
>  	if (IS_SRIOV_PF(xe))
>  		xe_pci_sriov_configure(pdev, 0);

[Severity: High]
What happens in xe_pci_remove() when the device was probed in Boot
Survivability mode, or when __xe_pci_probe() jumps to err_driver_cleanup
before drm_dev_register() succeeds?

In __xe_pci_probe() in drivers/gpu/drm/xe/xe_pci.c, if Boot Survivability
mode is enabled during xe_device_probe_early(), probe returns 0 early:

    err = xe_device_probe_early(xe);
    if (xe_survivability_mode_is_boot_enabled(xe))
        return 0;

At that point xe->drm has DRIVER_MODESET set by xe_device_create(), but
xe_display_probe(), drmm_mode_config_init(), and drm_dev_register() have not
been called.

Because drm_dev_unplug(&xe->drm) is placed before the
xe_survivability_mode_is_boot_enabled(xe) check in xe_pci_remove(), removing
the device calls drm_dev_unplug() -> drm_dev_unregister() ->
drm_modeset_unregister_all() -> drm_connector_unregister_all() ->
drm_connector_list_iter_next() in drivers/gpu/drm/drm_connector.c:

    spin_lock_irqsave(&config->connector_list_lock, flags);
    lhead = old_conn ? &old_conn->head : &config->connector_list;

    do {
        if (lhead->next == &config->connector_list) {

This locks the uninitialized connector_list_lock spinlock and dereferences a
NULL connector_list.next pointer.

Similarly, if xe_pm_init() or drm_dev_register() fails at the end of
__xe_pci_probe(), jumping to err_driver_cleanup calls xe_pci_remove() ->
drm_dev_unplug(&xe->drm) on a drm_device that was never registered.

> @@ -1291,6 +1293,11 @@ static int __xe_pci_probe(struct pci_dev *pdev, const struct xe_device_desc *des
>  	if (err)
>  		goto err_driver_cleanup;
>  
> +	/* Must be last */
> +	err = drm_dev_register(&xe->drm, 0);
> +	if (err)
> +		goto err_driver_cleanup;

[Severity: High]
When drm_dev_register() is deferred until after xe_device_probe(), what
happens to xe_debugfs_register(xe) inside xe_device_probe()?

In drivers/gpu/drm/xe/xe_debugfs.c, xe_debugfs_register() retrieves the
debugfs root from the primary minor:

    struct drm_minor *minor = xe->drm.primary;
    struct dentry *root = minor->debugfs_root;

However, minor->debugfs_root is only populated when drm_dev_register() ->
drm_minor_register() calls drm_debugfs_register() in
drivers/gpu/drm/drm_debugfs.c:

    minor->debugfs_root = dev->debugfs_root;

When xe_debugfs_register() executes in xe_device_probe() before
drm_dev_register(), minor->debugfs_root is still NULL:

1. In drivers/gpu/drm/xe/xe_gt_debugfs.c, xe_gt_debugfs_register() hits
   xe_gt_assert(gt, minor->debugfs_root) on every GT.
2. In drivers/gpu/drm/xe/xe_pxp_debugfs.c and drivers/gpu/drm/xe/xe_psmi.c,
   xe_pxp_debugfs_register() and xe_psmi_debugfs_register() check
   if (!minor->debugfs_root) and return early without registering their
   debugfs entries.
3. xe_debugfs_register() and xe_tile_debugfs_register() pass root (NULL) as
   the parent dentry to debugfs_create_file(), debugfs_create_dir(),
   drm_debugfs_create_files(), and ttm_resource_manager_create_debugfs(),
   creating the driver's debugfs entries in the top-level /sys/kernel/debug/
   directory where they are not cleaned up on device unbind and retain
   dangling pointers to freed driver structures.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023030446.337222-1-raag.jadav@intel.com?part=1