[PATCH] drm/client: fix restore of partially initialized client

[PATCH] drm/client: fix restore of partially initialized client

[shechenglong]

I got a null-ptr-deref report when closing a DRM file descriptor:

WARNING: drivers/gpu/drm/drm_atomic.c:2031 at
__drm_atomic_helper_set_config+0x18e/0x1b0 [drm]

Call Trace:
drm_client_modeset_commit_atomic+0x16b/0x220 [drm]
drm_client_modeset_commit_locked+0x56/0x160 [drm]
drm_client_modeset_commit+0x21/0x40 [drm]
__drm_fb_helper_restore_fbdev_mode_unlocked.part.0+0x7b/0x80
drm_fbdev_client_restore+0xe/0x20 [drm_client_lib]
drm_client_dev_restore+0x9f/0xc0 [drm]
drm_release+0xc5/0xe0 [drm]

The warning is followed by a NULL pointer dereference:

BUG: kernel NULL pointer dereference, address: 0000000000000008

RIP:
__drm_fb_helper_restore_fbdev_mode_unlocked.part.0+0x41/0x80
[drm_kms_helper]

Call Trace:
drm_fbdev_client_restore+0xe/0x20 [drm_client_lib]
drm_client_dev_restore+0x9f/0xc0 [drm]
drm_release+0xc5/0xe0 [drm]
__fput+0xdc/0x2b0
__x64_sys_close+0x39/0x80
do_syscall_64+0x8d/0x460
entry_SYSCALL_64_after_hwframe+0x76/0x7e

drm_client_register() adds the DRM client to the device client list
before invoking the initial hotplug callback. If the hotplug callback
fails, the client remains registered.

For the fbdev client, a failure during drm_fb_helper_initial_config()
causes the partially initialized fbdev helper to be cleaned up.
drm_fb_helper_fini() releases fb_helper->info and leaves it NULL.

The fbdev client therefore remains registered even though there is no
fully initialized framebuffer device.

Later, when userspace closes the DRM file descriptor, drm_release()
can invoke the restore callbacks of registered DRM clients:

drm_release()
drm_client_dev_restore()
drm_fbdev_client_restore()
drm_fb_helper_restore_fbdev_mode_unlocked()

drm_fbdev_client_restore() currently restores the fbdev state
unconditionally. For a partially initialized fbdev client this can
submit an incomplete modeset state and subsequently access fbdev
state which has not been initialized, resulting in the warning and
NULL pointer dereference above.

drm_fbdev_client_unregister() already uses fb_helper->info to
distinguish a fully probed framebuffer device from a partially
initialized client.

Use the same condition in drm_fbdev_client_restore() and skip restore
if no framebuffer device has been successfully initialized.

Signed-off-by: shechenglong <shechenglong@xfusion.com>
---
 drivers/gpu/drm/clients/drm_fbdev_client.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/clients/drm_fbdev_client.c b/drivers/gpu/drm/clients/drm_fbdev_client.c
index 91d196a397cf..1c16bc1084c4 100644
--- a/drivers/gpu/drm/clients/drm_fbdev_client.c
+++ b/drivers/gpu/drm/clients/drm_fbdev_client.c
@@ -42,6 +42,14 @@ static int drm_fbdev_client_restore(struct drm_client_dev *client, bool force)
 {
 	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
 
+	/*
+	 * The client is registered before the initial fbdev probe.
+	 * If probing failed, the client remains registered but there
+	 * is no valid fbdev framebuffer to restore.
+	 */
+	if (!fb_helper->info || !fb_helper->fb)
+		return 0;
+
 	drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper, force);
 
 	return 0;
-- 
2.33.0

Re: [PATCH] drm/client: fix restore of partially initialized client

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Memory leak of drm_client_buffer and its virtual memory mapping if register_framebuffer() fails.
--

commit 99c42165eff4a5954aaf41ad682bb169178e6b09
Author: shechenglong <shechenglong@xfusion.com>

drm/client: fix restore of partially initialized client

This commit fixes a NULL pointer dereference that occurs when closing a
DRM file descriptor for a partially initialized fbdev client. It skips
the restore process in drm_fbdev_client_restore() if no valid fbdev
framebuffer is found.

> diff --git a/drivers/gpu/drm/clients/drm_fbdev_client.c b/drivers/gpu/drm/clients/drm_fbdev_client.c
> index 91d196a397cf2..1c16bc1084c40 100644
> --- a/drivers/gpu/drm/clients/drm_fbdev_client.c
> +++ b/drivers/gpu/drm/clients/drm_fbdev_client.c
> @@ -42,6 +42,14 @@ static int drm_fbdev_client_restore(struct drm_client_dev *client, bool force)
>  {
>  	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
>  
> +	/*
> +	 * The client is registered before the initial fbdev probe.
> +	 * If probing failed, the client remains registered but there
> +	 * is no valid fbdev framebuffer to restore.
> +	 */
> +	if (!fb_helper->info || !fb_helper->fb)
> +		return 0;
> +

[Severity: High]
This is a pre-existing issue, but since the commit message mentions mirroring
the logic from drm_fbdev_client_unregister(), does the current cleanup path
leak the drm_client_buffer when probing fails?

In drivers/gpu/drm/drm_fb_helper.c:__drm_fb_helper_initial_config_and_unlock(),
drm_fb_helper_single_fb_probe() allocates fb_helper->buffer (which creates the
framebuffer and maps memory). If the subsequent call to register_framebuffer()
fails, it returns without cleaning up the buffer:

drivers/gpu/drm/drm_fb_helper.c:__drm_fb_helper_initial_config_and_unlock() {
    ...
    ret = register_framebuffer(info);
    if (ret < 0)
        return ret;
    ...
}

The caller handles this error by calling drm_fb_helper_fini(), which frees
fb_helper->info via framebuffer_release() and sets fb_helper->info to NULL.
Importantly, framebuffer_release() only frees the struct without invoking
the fb_destroy callback.

Later, when the client is unregistered, drm_fbdev_client_unregister() takes
the else branch because fb_helper->info is NULL:

drivers/gpu/drm/clients/drm_fbdev_client.c:drm_fbdev_client_unregister() {
    if (fb_helper->info) {
        ...
    } else {
        /*
         * Partially initialized client, no framebuffer device yet
         */
        drm_client_release(&fb_helper->client);
    }
}

This skips drm_fb_helper_unregister_info() and the associated fb_destroy()
callback, permanently leaking fb_helper->buffer, its backing GEM object,
and its virtual memory mapping.

This leak can be triggered by resource exhaustion, limit reached (FB_MAX), or
other failures during register_framebuffer(). Could the error handling path
be updated to ensure the buffer is properly destroyed when this occurs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907035147.1339-1-shechenglong@xfusion.com?part=1

Re: [PATCH] drm/client: fix restore of partially initialized client

[shechenglong]

> Could the error handling path be updated to ensure the buffer is properly
> destroyed when this occurs?

Thanks for pointing this out.

This appears to be a pre-existing issue and is not introduced by this
patch. The purpose of the current patch is limited to preventing the
restore callback from dereferencing a partially initialized fbdev
client.

The cleanup concern looks valid. I will inspect the buffer ownership and
error-unwind paths around register_framebuffer() failure. If additional
cleanup is required, I will handle it separately so that the restore fix
remains focused.

Thanks,
Chenglong She

[PATCH RESEND] drm/client: fix restore of partially initialized client

[shechenglong]

I got a null-ptr-deref report when closing a DRM file descriptor:

WARNING: drivers/gpu/drm/drm_atomic.c:2031 at
__drm_atomic_helper_set_config+0x18e/0x1b0 [drm]

Call Trace:
drm_client_modeset_commit_atomic+0x16b/0x220 [drm]
drm_client_modeset_commit_locked+0x56/0x160 [drm]
drm_client_modeset_commit+0x21/0x40 [drm]
__drm_fb_helper_restore_fbdev_mode_unlocked.part.0+0x7b/0x80
drm_fbdev_client_restore+0xe/0x20 [drm_client_lib]
drm_client_dev_restore+0x9f/0xc0 [drm]
drm_release+0xc5/0xe0 [drm]

The warning is followed by a NULL pointer dereference:

BUG: kernel NULL pointer dereference, address: 0000000000000008

RIP:
__drm_fb_helper_restore_fbdev_mode_unlocked.part.0+0x41/0x80
[drm_kms_helper]

Call Trace:
drm_fbdev_client_restore+0xe/0x20 [drm_client_lib]
drm_client_dev_restore+0x9f/0xc0 [drm]
drm_release+0xc5/0xe0 [drm]
__fput+0xdc/0x2b0
__x64_sys_close+0x39/0x80
do_syscall_64+0x8d/0x460
entry_SYSCALL_64_after_hwframe+0x76/0x7e

drm_client_register() adds the DRM client to the device client list
before invoking the initial hotplug callback. If the hotplug callback
fails, the client remains registered.

For the fbdev client, a failure during drm_fb_helper_initial_config()
causes the partially initialized fbdev helper to be cleaned up.
drm_fb_helper_fini() releases fb_helper->info and leaves it NULL.

The fbdev client therefore remains registered even though there is no
fully initialized framebuffer device.

Later, when userspace closes the DRM file descriptor, drm_release()
can invoke the restore callbacks of registered DRM clients:

drm_release()
drm_client_dev_restore()
drm_fbdev_client_restore()
drm_fb_helper_restore_fbdev_mode_unlocked()

drm_fbdev_client_restore() currently restores the fbdev state
unconditionally. For a partially initialized fbdev client this can
submit an incomplete modeset state and subsequently access fbdev
state which has not been initialized, resulting in the warning and
NULL pointer dereference above.

drm_fbdev_client_unregister() already uses fb_helper->info to
distinguish a fully probed framebuffer device from a partially
initialized client.

Use the same condition in drm_fbdev_client_restore() and skip restore
if no framebuffer device has been successfully initialized.

Signed-off-by: shechenglong <shechenglong@xfusion.com>
---
 drivers/gpu/drm/clients/drm_fbdev_client.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/clients/drm_fbdev_client.c b/drivers/gpu/drm/clients/drm_fbdev_client.c
index 91d196a397cf..1c16bc1084c4 100644
--- a/drivers/gpu/drm/clients/drm_fbdev_client.c
+++ b/drivers/gpu/drm/clients/drm_fbdev_client.c
@@ -42,6 +42,14 @@ static int drm_fbdev_client_restore(struct drm_client_dev *client, bool force)
 {
 	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
 
+	/*
+	 * The client is registered before the initial fbdev probe.
+	 * If probing failed, the client remains registered but there
+	 * is no valid fbdev framebuffer to restore.
+	 */
+	if (!fb_helper->info || !fb_helper->fb)
+		return 0;
+
 	drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper, force);
 
 	return 0;
-- 
2.33.0

Re: [PATCH] drm/client: fix restore of partially initialized client

[Thomas Zimmermann]

Am 07.09.26 um 05:51 schrieb shechenglong:
> I got a null-ptr-deref report when closing a DRM file descriptor:
>
> WARNING: drivers/gpu/drm/drm_atomic.c:2031 at
> __drm_atomic_helper_set_config+0x18e/0x1b0 [drm]
>
> Call Trace:
> drm_client_modeset_commit_atomic+0x16b/0x220 [drm]
> drm_client_modeset_commit_locked+0x56/0x160 [drm]
> drm_client_modeset_commit+0x21/0x40 [drm]
> __drm_fb_helper_restore_fbdev_mode_unlocked.part.0+0x7b/0x80
> drm_fbdev_client_restore+0xe/0x20 [drm_client_lib]
> drm_client_dev_restore+0x9f/0xc0 [drm]
> drm_release+0xc5/0xe0 [drm]
>
> The warning is followed by a NULL pointer dereference:
>
> BUG: kernel NULL pointer dereference, address: 0000000000000008
>
> RIP:
> __drm_fb_helper_restore_fbdev_mode_unlocked.part.0+0x41/0x80
> [drm_kms_helper]
>
> Call Trace:
> drm_fbdev_client_restore+0xe/0x20 [drm_client_lib]
> drm_client_dev_restore+0x9f/0xc0 [drm]
> drm_release+0xc5/0xe0 [drm]
> __fput+0xdc/0x2b0
> __x64_sys_close+0x39/0x80
> do_syscall_64+0x8d/0x460
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
>
> drm_client_register() adds the DRM client to the device client list
> before invoking the initial hotplug callback. If the hotplug callback
> fails, the client remains registered.
>
> For the fbdev client, a failure during drm_fb_helper_initial_config()
> causes the partially initialized fbdev helper to be cleaned up.
> drm_fb_helper_fini() releases fb_helper->info and leaves it NULL.
>
> The fbdev client therefore remains registered even though there is no
> fully initialized framebuffer device.
>
> Later, when userspace closes the DRM file descriptor, drm_release()
> can invoke the restore callbacks of registered DRM clients:
>
> drm_release()
> drm_client_dev_restore()
> drm_fbdev_client_restore()
> drm_fb_helper_restore_fbdev_mode_unlocked()
>
> drm_fbdev_client_restore() currently restores the fbdev state
> unconditionally. For a partially initialized fbdev client this can
> submit an incomplete modeset state and subsequently access fbdev
> state which has not been initialized, resulting in the warning and
> NULL pointer dereference above.
>
> drm_fbdev_client_unregister() already uses fb_helper->info to
> distinguish a fully probed framebuffer device from a partially
> initialized client.
>
> Use the same condition in drm_fbdev_client_restore() and skip restore
> if no framebuffer device has been successfully initialized.
>
> Signed-off-by: shechenglong <shechenglong@xfusion.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

Thanks for fixing this bug.

Fixes: 5d08c44e47b9 ("drm/fbdev: Add memory-agnostic fbdev client")
Cc: <stable@vger.kernel.org> # v6.13+


> ---
>   drivers/gpu/drm/clients/drm_fbdev_client.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/clients/drm_fbdev_client.c b/drivers/gpu/drm/clients/drm_fbdev_client.c
> index 91d196a397cf..1c16bc1084c4 100644
> --- a/drivers/gpu/drm/clients/drm_fbdev_client.c
> +++ b/drivers/gpu/drm/clients/drm_fbdev_client.c
> @@ -42,6 +42,14 @@ static int drm_fbdev_client_restore(struct drm_client_dev *client, bool force)
>   {
>   	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
>   
> +	/*
> +	 * The client is registered before the initial fbdev probe.
> +	 * If probing failed, the client remains registered but there
> +	 * is no valid fbdev framebuffer to restore.
> +	 */
> +	if (!fb_helper->info || !fb_helper->fb)
> +		return 0;
> +
>   	drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper, force);
>   
>   	return 0;

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)

[PATCH v2] drm/client: fix restore of partially initialized client

[shechenglong]

I got a null-ptr-deref report when closing a DRM file descriptor:

WARNING: drivers/gpu/drm/drm_atomic.c:2031 at
__drm_atomic_helper_set_config+0x18e/0x1b0 [drm]

Call Trace:
drm_client_modeset_commit_atomic+0x16b/0x220 [drm]
drm_client_modeset_commit_locked+0x56/0x160 [drm]
drm_client_modeset_commit+0x21/0x40 [drm]
__drm_fb_helper_restore_fbdev_mode_unlocked.part.0+0x7b/0x80
drm_fbdev_client_restore+0xe/0x20 [drm_client_lib]
drm_client_dev_restore+0x9f/0xc0 [drm]
drm_release+0xc5/0xe0 [drm]

The warning is followed by a NULL pointer dereference:

BUG: kernel NULL pointer dereference, address: 0000000000000008

RIP:
__drm_fb_helper_restore_fbdev_mode_unlocked.part.0+0x41/0x80
[drm_kms_helper]

Call Trace:
drm_fbdev_client_restore+0xe/0x20 [drm_client_lib]
drm_client_dev_restore+0x9f/0xc0 [drm]
drm_release+0xc5/0xe0 [drm]
__fput+0xdc/0x2b0
__x64_sys_close+0x39/0x80
do_syscall_64+0x8d/0x460
entry_SYSCALL_64_after_hwframe+0x76/0x7e

drm_client_register() adds the DRM client to the device client list
before invoking the initial hotplug callback. If the hotplug callback
fails, the client remains registered.

For the fbdev client, a failure during drm_fb_helper_initial_config()
causes the partially initialized fbdev helper to be cleaned up.
drm_fb_helper_fini() releases fb_helper->info and leaves it NULL.

The fbdev client therefore remains registered even though there is no
fully initialized framebuffer device.

Later, when userspace closes the DRM file descriptor, drm_release()
can invoke the restore callbacks of registered DRM clients:

drm_release()
drm_client_dev_restore()
drm_fbdev_client_restore()
drm_fb_helper_restore_fbdev_mode_unlocked()

drm_fbdev_client_restore() currently restores the fbdev state
unconditionally. For a partially initialized fbdev client this can
submit an incomplete modeset state and subsequently access fbdev
state which has not been initialized, resulting in the warning and
NULL pointer dereference above.

drm_fbdev_client_unregister() already uses fb_helper->info to
distinguish a fully probed framebuffer device from a partially
initialized client.

Use the same condition in drm_fbdev_client_restore() and skip restore
if no framebuffer device has been successfully initialized.

Fixes: 5d08c44e47b9 ("drm/fbdev: Add memory-agnostic fbdev client")
Cc: <stable@vger.kernel.org> # v6.13+

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: shechenglong <shechenglong@xfusion.com>
---
Changes in v2:
- Add the Fixes and stable tags suggested by Thomas Zimmermann.

 drivers/gpu/drm/clients/drm_fbdev_client.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/clients/drm_fbdev_client.c b/drivers/gpu/drm/clients/drm_fbdev_client.c
index 91d196a397cf..1c16bc1084c4 100644
--- a/drivers/gpu/drm/clients/drm_fbdev_client.c
+++ b/drivers/gpu/drm/clients/drm_fbdev_client.c
@@ -42,6 +42,14 @@ static int drm_fbdev_client_restore(struct drm_client_dev *client, bool force)
 {
 	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
 
+	/*
+	 * The client is registered before the initial fbdev probe.
+	 * If probing failed, the client remains registered but there
+	 * is no valid fbdev framebuffer to restore.
+	 */
+	if (!fb_helper->info || !fb_helper->fb)
+		return 0;
+
 	drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper, force);
 
 	return 0;
-- 
2.33.0

Re: [PATCH v2] drm/client: fix restore of partially initialized client

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/client: early return of 0 in fbdev restore aborts fallback loop
--

commit d167ca7eea0e08eeed4c9c5277344faccb69545c
Author: shechenglong <shechenglong@xfusion.com>

drm/client: fix restore of partially initialized client

This commit fixes a NULL pointer dereference that occurs when userspace
closes a DRM file descriptor and a partially initialized fbdev client is
restored. It adds a check to skip the restore operation if the framebuffer
device has not been fully initialized.

> diff --git a/drivers/gpu/drm/clients/drm_fbdev_client.c b/drivers/gpu/drm/clients/drm_fbdev_client.c
> index 827f326687140..7574bd36f45eb 100644
> --- a/drivers/gpu/drm/clients/drm_fbdev_client.c
> +++ b/drivers/gpu/drm/clients/drm_fbdev_client.c
> @@ -42,6 +42,14 @@ static int drm_fbdev_client_restore(struct drm_client_dev *client, bool force)
>  {
>  	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
>  
> +	/*
> +	 * The client is registered before the initial fbdev probe.
> +	 * If probing failed, the client remains registered but there
> +	 * is no valid fbdev framebuffer to restore.
> +	 */
> +	if (!fb_helper->info || !fb_helper->fb)
> +		return 0;

[Severity: Medium]
Does returning 0 here prematurely abort the fallback loop in the caller?

When the fbdev client is registered but fails probing, fb_helper->info is
NULL. The caller, drm_client_dev_restore(), iterates through clients and
calls their restore callbacks. Since drm_client_dev_restore() breaks the loop
on the first 0 return, this falsely signals success.

This means the display is not restored by other registered clients
(like drm_log) when userspace closes the DRM fd or exits. Should this return
a negative error code like -ENODEV to allow the loop to continue?

> +
>  	drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper, force);
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917154743.1621-1-shechenglong@xfusion.com?part=1

Re: [PATCH v2] drm/client: fix restore of partially initialized client

[Thomas Zimmermann]

Hi

Am 17.09.26 um 18:06 schrieb sashiko-bot@kernel.org:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] drm/client: early return of 0 in fbdev restore aborts fallback loop
> --
>
> commit d167ca7eea0e08eeed4c9c5277344faccb69545c
> Author: shechenglong <shechenglong@xfusion.com>
>
> drm/client: fix restore of partially initialized client
>
> This commit fixes a NULL pointer dereference that occurs when userspace
> closes a DRM file descriptor and a partially initialized fbdev client is
> restored. It adds a check to skip the restore operation if the framebuffer
> device has not been fully initialized.
>
>> diff --git a/drivers/gpu/drm/clients/drm_fbdev_client.c b/drivers/gpu/drm/clients/drm_fbdev_client.c
>> index 827f326687140..7574bd36f45eb 100644
>> --- a/drivers/gpu/drm/clients/drm_fbdev_client.c
>> +++ b/drivers/gpu/drm/clients/drm_fbdev_client.c
>> @@ -42,6 +42,14 @@ static int drm_fbdev_client_restore(struct drm_client_dev *client, bool force)
>>   {
>>   	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
>>   
>> +	/*
>> +	 * The client is registered before the initial fbdev probe.
>> +	 * If probing failed, the client remains registered but there
>> +	 * is no valid fbdev framebuffer to restore.
>> +	 */
>> +	if (!fb_helper->info || !fb_helper->fb)
>> +		return 0;
> [Severity: Medium]
> Does returning 0 here prematurely abort the fallback loop in the caller?
>
> When the fbdev client is registered but fails probing, fb_helper->info is
> NULL. The caller, drm_client_dev_restore(), iterates through clients and
> calls their restore callbacks. Since drm_client_dev_restore() breaks the loop
> on the first 0 return, this falsely signals success.
>
> This means the display is not restored by other registered clients
> (like drm_log) when userspace closes the DRM fd or exits. Should this return
> a negative error code like -ENODEV to allow the loop to continue?

We can ignore this problem for now.  Multi-client setups have never been 
used or even tested before.

Best regards
Thomas

>
>> +
>>   	drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper, force);
>>   
>>   	return 0;
>>   }

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)

Re: [PATCH v2] drm/client: fix restore of partially initialized client

[Thomas Zimmermann]

Hi,

I merged the patch into drm-misc-fixes.

Best regards
Thomas

Am 17.09.26 um 17:47 schrieb shechenglong:
> I got a null-ptr-deref report when closing a DRM file descriptor:
>
> WARNING: drivers/gpu/drm/drm_atomic.c:2031 at
> __drm_atomic_helper_set_config+0x18e/0x1b0 [drm]
>
> Call Trace:
> drm_client_modeset_commit_atomic+0x16b/0x220 [drm]
> drm_client_modeset_commit_locked+0x56/0x160 [drm]
> drm_client_modeset_commit+0x21/0x40 [drm]
> __drm_fb_helper_restore_fbdev_mode_unlocked.part.0+0x7b/0x80
> drm_fbdev_client_restore+0xe/0x20 [drm_client_lib]
> drm_client_dev_restore+0x9f/0xc0 [drm]
> drm_release+0xc5/0xe0 [drm]
>
> The warning is followed by a NULL pointer dereference:
>
> BUG: kernel NULL pointer dereference, address: 0000000000000008
>
> RIP:
> __drm_fb_helper_restore_fbdev_mode_unlocked.part.0+0x41/0x80
> [drm_kms_helper]
>
> Call Trace:
> drm_fbdev_client_restore+0xe/0x20 [drm_client_lib]
> drm_client_dev_restore+0x9f/0xc0 [drm]
> drm_release+0xc5/0xe0 [drm]
> __fput+0xdc/0x2b0
> __x64_sys_close+0x39/0x80
> do_syscall_64+0x8d/0x460
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
>
> drm_client_register() adds the DRM client to the device client list
> before invoking the initial hotplug callback. If the hotplug callback
> fails, the client remains registered.
>
> For the fbdev client, a failure during drm_fb_helper_initial_config()
> causes the partially initialized fbdev helper to be cleaned up.
> drm_fb_helper_fini() releases fb_helper->info and leaves it NULL.
>
> The fbdev client therefore remains registered even though there is no
> fully initialized framebuffer device.
>
> Later, when userspace closes the DRM file descriptor, drm_release()
> can invoke the restore callbacks of registered DRM clients:
>
> drm_release()
> drm_client_dev_restore()
> drm_fbdev_client_restore()
> drm_fb_helper_restore_fbdev_mode_unlocked()
>
> drm_fbdev_client_restore() currently restores the fbdev state
> unconditionally. For a partially initialized fbdev client this can
> submit an incomplete modeset state and subsequently access fbdev
> state which has not been initialized, resulting in the warning and
> NULL pointer dereference above.
>
> drm_fbdev_client_unregister() already uses fb_helper->info to
> distinguish a fully probed framebuffer device from a partially
> initialized client.
>
> Use the same condition in drm_fbdev_client_restore() and skip restore
> if no framebuffer device has been successfully initialized.
>
> Fixes: 5d08c44e47b9 ("drm/fbdev: Add memory-agnostic fbdev client")
> Cc: <stable@vger.kernel.org> # v6.13+
>
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: shechenglong <shechenglong@xfusion.com>
> ---
> Changes in v2:
> - Add the Fixes and stable tags suggested by Thomas Zimmermann.
>
>   drivers/gpu/drm/clients/drm_fbdev_client.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/clients/drm_fbdev_client.c b/drivers/gpu/drm/clients/drm_fbdev_client.c
> index 91d196a397cf..1c16bc1084c4 100644
> --- a/drivers/gpu/drm/clients/drm_fbdev_client.c
> +++ b/drivers/gpu/drm/clients/drm_fbdev_client.c
> @@ -42,6 +42,14 @@ static int drm_fbdev_client_restore(struct drm_client_dev *client, bool force)
>   {
>   	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
>   
> +	/*
> +	 * The client is registered before the initial fbdev probe.
> +	 * If probing failed, the client remains registered but there
> +	 * is no valid fbdev framebuffer to restore.
> +	 */
> +	if (!fb_helper->info || !fb_helper->fb)
> +		return 0;
> +
>   	drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper, force);
>   
>   	return 0;

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)