[PATCH] firmware/sysfb: Fix device reference count leak in sysfb_disable()

[PATCH] firmware/sysfb: Fix device reference count leak in sysfb_disable()

[Wentao Liang]

In sysfb_disable(), parent is obtained via sysfb_parent_dev(si), which
calls screen_info_pci_dev(si) and returns a pointer to &pdev->dev with
an acquired reference count. However, sysfb_disable() does not release
this reference before returning, leading to a device reference leak.

Fix this by checking if parent is not an ERR_PTR and calling
put_device(parent) before releasing the mutex.

Fixes: b49420d6a1ae ("video/aperture: optionally match the device in sysfb_disable()")
Cc: stable@vger.kernel.org
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
 drivers/firmware/sysfb.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
index 8833582c1883..11c5ce128a6c 100644
--- a/drivers/firmware/sysfb.c
+++ b/drivers/firmware/sysfb.c
@@ -71,9 +71,12 @@ void sysfb_disable(struct device *dev)
 
 	mutex_lock(&disable_lock);
 	parent = sysfb_parent_dev(si);
-	if (!dev || !parent || dev == parent) {
-		sysfb_unregister();
-		disabled = true;
+	if (!IS_ERR(parent)) {
+		if (!dev || !parent || dev == parent) {
+			sysfb_unregister();
+			disabled = true;
+		}
+		put_device(parent);
 	}
 	mutex_unlock(&disable_lock);
 }
-- 
2.34.1

Re: [PATCH] firmware/sysfb: Fix device reference count leak in sysfb_disable()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] sysfb: kill-switch bypass when parent device query returns an error
--

commit cea5d6870e1a5b4277264d29fe9056b6df3310b7
Author: Wentao Liang <vulab@iscas.ac.cn>

firmware/sysfb: Fix device reference count leak in sysfb_disable()

This commit addresses a device reference count leak in sysfb_disable(). It
adds a check to ensure the parent device is not an error pointer and releases
the reference using put_device() before returning.

> diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
> --- a/drivers/firmware/sysfb.c
> +++ b/drivers/firmware/sysfb.c
> @@ -71,9 +71,12 @@ void sysfb_disable(struct device *dev)
>  
>  	mutex_lock(&disable_lock);
>  	parent = sysfb_parent_dev(si);
> -	if (!dev || !parent || dev == parent) {
> -		sysfb_unregister();
> -		disabled = true;
> +	if (!IS_ERR(parent)) {

[Severity: High]
Will wrapping the unregistration logic in !IS_ERR(parent) bypass the global
kill-switch when the parent device query returns an error?

Graphics drivers use sysfb_disable(NULL) as a global kill-switch to
unconditionally disable the generic firmware framebuffer before native DRM
drivers take over.

sysfb_parent_dev() can return an ERR_PTR (like -ENODEV) if the PCI device
exists but its memory decoding is not enabled. If this happens during early
boot or driver probe, this new check skips the block entirely, silently
bypassing the kill-switch and leaving the disabled flag as false.

If a built-in native DRM driver disables sysfb and subsequently enables PCI
memory decoding, could a later execution of sysfb_init() see disabled == false
and incorrectly register the generic framebuffer on top of the already-running
native driver? This concurrent access could lead to resource conflicts or kernel
panics.

> +		if (!dev || !parent || dev == parent) {
> +			sysfb_unregister();
> +			disabled = true;
> +		}
> +		put_device(parent);
>  	}
>  	mutex_unlock(&disable_lock);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916073815.1972045-1-vulab@iscas.ac.cn?part=1

RE: [PATCH] firmware/sysfb: Fix device reference count leak in sysfb_disable()

[Deucher, Alexander]

Public

> -----Original Message-----
> From: Wentao Liang <vulab@iscas.ac.cn>
> Sent: Wednesday, September 16, 2026 3:38 AM
> To: Deucher, Alexander <Alexander.Deucher@amd.com>
> Cc: dri-devel@lists.freedesktop.org; javierm@redhat.com; linux-
> kernel@vger.kernel.org; tzimmermann@suse.de; Wentao Liang
> <vulab@iscas.ac.cn>; stable@vger.kernel.org
> Subject: [PATCH] firmware/sysfb: Fix device reference count leak in
> sysfb_disable()
>
> In sysfb_disable(), parent is obtained via sysfb_parent_dev(si), which calls
> screen_info_pci_dev(si) and returns a pointer to &pdev->dev with an acquired
> reference count. However, sysfb_disable() does not release this reference
> before returning, leading to a device reference leak.
>
> Fix this by checking if parent is not an ERR_PTR and calling
> put_device(parent) before releasing the mutex.
>
> Fixes: b49420d6a1ae ("video/aperture: optionally match the device in
> sysfb_disable()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
> ---
>  drivers/firmware/sysfb.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c index
> 8833582c1883..11c5ce128a6c 100644
> --- a/drivers/firmware/sysfb.c
> +++ b/drivers/firmware/sysfb.c
> @@ -71,9 +71,12 @@ void sysfb_disable(struct device *dev)
>
>       mutex_lock(&disable_lock);
>       parent = sysfb_parent_dev(si);
> -     if (!dev || !parent || dev == parent) {
> -             sysfb_unregister();
> -             disabled = true;
> +     if (!IS_ERR(parent)) {

Are there cases where sysfb_unregister() would need to be called even if parent returns an error?

Alex

> +             if (!dev || !parent || dev == parent) {
> +                     sysfb_unregister();
> +                     disabled = true;
> +             }
> +             put_device(parent);
>       }
>       mutex_unlock(&disable_lock);
>  }
> --
> 2.34.1

Re: [PATCH] firmware/sysfb: Fix device reference count leak in sysfb_disable()

[Thomas Zimmermann]

Hi

Am 16.09.26 um 23:06 schrieb Deucher, Alexander:
> Public
>
>> -----Original Message-----
>> From: Wentao Liang <vulab@iscas.ac.cn>
>> Sent: Wednesday, September 16, 2026 3:38 AM
>> To: Deucher, Alexander <Alexander.Deucher@amd.com>
>> Cc: dri-devel@lists.freedesktop.org; javierm@redhat.com; linux-
>> kernel@vger.kernel.org; tzimmermann@suse.de; Wentao Liang
>> <vulab@iscas.ac.cn>; stable@vger.kernel.org
>> Subject: [PATCH] firmware/sysfb: Fix device reference count leak in
>> sysfb_disable()
>>
>> In sysfb_disable(), parent is obtained via sysfb_parent_dev(si), which calls
>> screen_info_pci_dev(si) and returns a pointer to &pdev->dev with an acquired
>> reference count. However, sysfb_disable() does not release this reference
>> before returning, leading to a device reference leak.
>>
>> Fix this by checking if parent is not an ERR_PTR and calling
>> put_device(parent) before releasing the mutex.
>>
>> Fixes: b49420d6a1ae ("video/aperture: optionally match the device in
>> sysfb_disable()")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
>> ---
>>   drivers/firmware/sysfb.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c index
>> 8833582c1883..11c5ce128a6c 100644
>> --- a/drivers/firmware/sysfb.c
>> +++ b/drivers/firmware/sysfb.c
>> @@ -71,9 +71,12 @@ void sysfb_disable(struct device *dev)
>>
>>        mutex_lock(&disable_lock);
>>        parent = sysfb_parent_dev(si);
>> -     if (!dev || !parent || dev == parent) {
>> -             sysfb_unregister();
>> -             disabled = true;
>> +     if (!IS_ERR(parent)) {
> Are there cases where sysfb_unregister() would need to be called even if parent returns an error?

It could be seen as a defensive measure. Usually we want to unregister 
the system framebuffer only if the native driver's device equals the 
parent. But if we fail to retrieve a parent, it might be better to 
unregister unconditionally.  Otherwise the sysfb driver might interfere 
with the native driver.

I think that would be a good idea.

Best regards
Thomas


>
> Alex
>
>> +             if (!dev || !parent || dev == parent) {
>> +                     sysfb_unregister();
>> +                     disabled = true;
>> +             }
>> +             put_device(parent);
>>        }
>>        mutex_unlock(&disable_lock);
>>   }
>> --
>> 2.34.1

-- 
--
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] firmware/sysfb: Fix device reference count leak in sysfb_disable()

[Wentao Liang]

Hi Thomas, Alex,

Sorry for the very late reply, and thanks for the review.

Alex, you asked whether sysfb_unregister() would still need to be called
when the parent device lookup returns an error. It does: without a known
parent we cannot tell whether the system framebuffer belongs to the
calling device, and leaving it registered could make sysfb interfere with
the native driver. Thomas, that is the defensive behaviour you suggested,
so v2 unregisters unconditionally when sysfb_parent_dev() fails and only
drops the reference on the non-error path.

The reference leak fix itself is unchanged.

From 8c5f084941a3b6aca5d3ae8cff0c3bd1d4ae1e6f Mon Sep 17 00:00:00 2001
From: Wentao Liang <vulab@iscas.ac.cn>
Date: Sat, 7 Nov 2026 17:02:27 +0000
Subject: [PATCH v2] firmware/sysfb: Fix device reference count leak in
 sysfb_disable()

sysfb_parent_dev() returns a pointer to &pdev->dev with an acquired
reference when it finds the PCI device that contains the system
framebuffer. sysfb_disable() never drops that reference, so a reference
to the PCI device is leaked on every call that resolves a parent.

Release the reference with put_device() before leaving the critical
section.

If the parent device cannot be determined, unregister the system
framebuffer unconditionally, as it might still belong to the calling
device. Leaving sysfb registered in that case can make it interfere
with the native driver.

Fixes: b49420d6a1ae ("video/aperture: optionally match the device in sysfb_disable()")
Cc: stable@vger.kernel.org
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
Changes in v2:
- Sorry for the late reply.
- Unregister the system framebuffer when the parent device lookup fails,
  as pointed out by Alex Deucher and Thomas Zimmermann.

Disclosure: the candidate was found with our static analysis tooling, and
the audit and the fix were assisted by DeepSeek and GPT 6.1 sol.
---
 drivers/firmware/sysfb.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
index 8833582c1883..4cd6e9f86962 100644
--- a/drivers/firmware/sysfb.c
+++ b/drivers/firmware/sysfb.c
@@ -71,10 +71,20 @@ void sysfb_disable(struct device *dev)
 
 	mutex_lock(&disable_lock);
 	parent = sysfb_parent_dev(si);
-	if (!dev || !parent || dev == parent) {
+
+	/*
+	 * If the parent device cannot be determined, unregister the system
+	 * framebuffer unconditionally. It might belong to the calling device
+	 * and would otherwise interfere with the native driver.
+	 */
+	if (IS_ERR_OR_NULL(parent) || !dev || dev == parent) {
 		sysfb_unregister();
 		disabled = true;
 	}
+
+	if (!IS_ERR(parent))
+		put_device(parent);
+
 	mutex_unlock(&disable_lock);
 }
 EXPORT_SYMBOL_GPL(sysfb_disable);
-- 
2.34.1

Re: [PATCH v2] firmware/sysfb: Fix device reference count leak in sysfb_disable()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] firmware/sysfb: conversational reply and mbox headers in commit
  message
--

commit 138aa0cb748b38d7396e354631327333b8c26fd3
Author: Wentao Liang <vulab@iscas.ac.cn>

> firmware/sysfb: Fix device reference count leak in sysfb_disable()
>
> Hi Thomas, Alex,
>
> Sorry for the very late reply, and thanks for the review.
>
> Alex, you asked whether sysfb_unregister() would still need to be called
> when the parent device lookup returns an error. It does: without a known
> parent we cannot tell whether the system framebuffer belongs to the
> calling device, and leaving it registered could make sysfb interfere with
> the native driver. Thomas, that is the defensive behaviour you suggested,
> so v2 unregisters unconditionally when sysfb_parent_dev() fails and only
> drops the reference on the non-error path.
>
> The reference leak fix itself is unchanged.
>
> From 8c5f084941a3b6aca5d3ae8cff0c3bd1d4ae1e6f Mon Sep 17 00:00:00 2001
> From: Wentao Liang <vulab@iscas.ac.cn>
> Date: Sat, 7 Nov 2026 17:02:27 +0000
> Subject: [PATCH v2] firmware/sysfb: Fix device reference count leak in
>  sysfb_disable()
>
> sysfb_parent_dev() returns a pointer to &pdev->dev with an acquired
> reference when it finds the PCI device that contains the system
> framebuffer. sysfb_disable() never drops that reference, so a reference
> to the PCI device is leaked on every call that resolves a parent.
>
> Release the reference with put_device() before leaving the critical
> section.
>
> If the parent device cannot be determined, unregister the system
> framebuffer unconditionally, as it might still belong to the calling
> device. Leaving sysfb registered in that case can make it interfere
> with the native driver.
>
> Fixes: b49420d6a1ae ("video/aperture: optionally match the device in sysfb_disable()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>

[Severity: Low]
The commit message includes a conversational reply and a duplicate
git-format-patch header block above the "---" tear line without a
scissors line ("-- >8 --"), so git am records them in the commit log.
The reply notes belong below "---" (or above a scissors line), and the
extra mbox headers should be removed from the changelog body.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/1794071765.2669957.4378216502938475@iscas.ac.cn?part=1