[PATCH] fbdev: protect mode sysfs reads with lock_fb_info()

[PATCH] fbdev: protect mode sysfs reads with lock_fb_info()

[Melbin K Mathew]

show_mode() dereferences fb_info->mode and show_modes() walks
fb_info->modelist without holding lock_fb_info(). store_modes() takes
lock_fb_info() while replacing the modelist and freeing the old one.

A concurrent reader can load a pointer to an old modelist entry before
store_modes() frees it, then dereference freed memory in mode_string().

Take lock_fb_info() in both show_mode() and show_modes() to serialize
with store_modes(). In show_mode(), copy the mode to the stack and
format the stack copy after dropping the lock.

Impact: local kernel UAF read when a privileged writer races with
sysfs readers of /sys/class/graphics/fb*/mode and modes.

Cc: stable@vger.kernel.org
Signed-off-by: Melbin K Mathew <mlbnkm1@gmail.com>
---
A userspace reproducer triggering the race is available to maintainers on request.

 drivers/video/fbdev/core/fbsysfs.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
index ea196603c7..6bdb25f7be 100644
--- a/drivers/video/fbdev/core/fbsysfs.c
+++ b/drivers/video/fbdev/core/fbsysfs.c
@@ -82,11 +82,20 @@ static ssize_t show_mode(struct device *device, struct device_attribute *attr,
 			 char *buf)
 {
 	struct fb_info *fb_info = dev_get_drvdata(device);
+	struct fb_videomode mode;
+	bool have_mode = false;
 
-	if (!fb_info->mode)
+	lock_fb_info(fb_info);
+	if (fb_info->mode) {
+		mode = *fb_info->mode;
+		have_mode = true;
+	}
+	unlock_fb_info(fb_info);
+
+	if (!have_mode)
 		return 0;
 
-	return mode_string(buf, 0, fb_info->mode);
+	return mode_string(buf, 0, &mode);
 }
 
 static ssize_t store_modes(struct device *device,
@@ -134,10 +143,13 @@ static ssize_t show_modes(struct device *device, struct device_attribute *attr,
 	const struct fb_videomode *mode;
 
 	i = 0;
+	lock_fb_info(fb_info);
 	list_for_each_entry(modelist, &fb_info->modelist, list) {
 		mode = &modelist->mode;
 		i += mode_string(buf, i, mode);
 	}
+	unlock_fb_info(fb_info);
+
 	return i;
 }
 
-- 
2.39.5

Re: [PATCH] fbdev: protect mode sysfs reads with lock_fb_info()

[sashiko-bot]

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

Pre-existing issues:
- [High] The patch fails to resolve the local kernel Use-After-Free because it misses lockless traversal and assignment in `store_mode()`.
- [High] Unprivileged local DoS via WARN_ON_ONCE triggered by snprintf size underflow in show_modes().
--

commit 781c0f45188865f8a46896496380e20ca20b9af8
Author: Melbin K Mathew <mlbnkm1@gmail.com>

fbdev: protect mode sysfs reads with lock_fb_info()

This commit adds lock_fb_info() around mode sysfs reads to protect
against concurrent modifications to the modelist by store_modes().

> diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
> index ea196603c7a87..6bdb25f7be018 100644
> --- a/drivers/video/fbdev/core/fbsysfs.c
> +++ b/drivers/video/fbdev/core/fbsysfs.c

[ ... ]

> @@ -82,11 +82,20 @@ static ssize_t show_mode(struct device *device, struct device_attribute *attr,
>  			 char *buf)
>  {
>  	struct fb_info *fb_info = dev_get_drvdata(device);
> +	struct fb_videomode mode;
> +	bool have_mode = false;
>  
> -	if (!fb_info->mode)
> +	lock_fb_info(fb_info);
> +	if (fb_info->mode) {
> +		mode = *fb_info->mode;
> +		have_mode = true;
> +	}
> +	unlock_fb_info(fb_info);

[Severity: High]
This is a pre-existing issue, but does store_mode() also need similar locking
protection to fully prevent the use-after-free?

While this patch protects show_mode() and show_modes(), store_mode() still
traverses the modelist locklessly:

drivers/video/fbdev/core/fbsysfs.c:store_mode() {
    ...
    list_for_each_entry(modelist, &fb_info->modelist, list) {
    ...
}

If store_modes() concurrently clears and frees the modelist, could
store_mode() traverse freed list elements?

Additionally, store_mode() locklessly assigns the mode pointer:

drivers/video/fbdev/core/fbsysfs.c:store_mode() {
    ...
    fb_info->mode = mode;
    ...
}

If store_modes() freed the old modelist in the interim, could this resurrect
a dangling pointer into fb_info->mode and bypass the fix for subsequent
readers?

[ ... ]

> @@ -134,10 +143,13 @@ static ssize_t show_modes(struct device *device, struct device_attribute *attr,
>  	const struct fb_videomode *mode;
>  
>  	i = 0;
> +	lock_fb_info(fb_info);
>  	list_for_each_entry(modelist, &fb_info->modelist, list) {
>  		mode = &modelist->mode;
>  		i += mode_string(buf, i, mode);
>  	}

[Severity: High]
This is a pre-existing issue, but can this loop trigger a WARN_ON_ONCE
due to integer underflow if the modelist is large?

If the accumulated length in i exceeds PAGE_SIZE (for example, with a very
large modelist), the size calculation in mode_string() appears to underflow:

drivers/video/fbdev/core/fbsysfs.c:mode_string() {
    ...
    return snprintf(&buf[offset], PAGE_SIZE - offset, "%c:%dx%d%c-%d\n",
                    m, mode->xres, mode->yres, v, mode->refresh);
}

Since offset is unsigned, PAGE_SIZE - offset would become a massive positive
value, which vsnprintf() detects as exceeding INT_MAX and triggers a
WARN_ON_ONCE. Could an unprivileged local user trigger a kernel panic on
systems with panic_on_warn=1 by repeatedly reading the modes sysfs file?

> +	unlock_fb_info(fb_info);
> +
>  	return i;
>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701221757.231490-1-mlbnkm1@gmail.com?part=1

[PATCH v2 0/2] fbdev: fix mode sysfs lifetime and bounds issues

[Melbin K Mathew]

This v2 addresses the remaining fbdev mode sysfs lifetime issues noted
during review.

Patch 1 bounds mode sysfs output so show_modes() cannot advance the
sysfs buffer offset past PAGE_SIZE. mode_string() is given a size
parameter and switched to scnprintf().

Patch 2 serializes mode sysfs access with lock_fb_info(), including
store_mode(), show_mode(), and show_modes(), so these paths cannot race
with store_modes() while it replaces and frees the old modelist.

Changes in v2:
- Add bounds handling for mode_string()/show_modes().
- Extend locking to store_mode() via activate_locked().
- Keep show_mode() using a stack copy after dropping lock_fb_info().
- Avoid overclaiming unprivileged impact in the commit text.


Melbin K Mathew (2):
  fbdev: bound mode sysfs output to the sysfs buffer
  fbdev: serialize mode sysfs access with lock_fb_info()

 drivers/video/fbdev/core/fbsysfs.c | 59 ++++++++++++++++++++++++------
 1 file changed, 47 insertions(+), 12 deletions(-)

-- 
2.39.5

[PATCH v3 0/3] fbdev: fix mode sysfs lifetime and bounds issues

[Melbin K Mathew]

This v3 adds a patch to clear fb_info->mode before deleting a
videomode through FBIOPUT_VSCREENINFO with FB_ACTIVATE_INV_MODE.

Patch 1 bounds mode sysfs output so show_modes() cannot advance the
sysfs buffer offset past PAGE_SIZE.

Patch 2 clears fb_info->mode before fb_delete_videomode() when it
matches the mode being removed via the FBIOPUT_VSCREENINFO ioctl.

Patch 3 serializes mode sysfs access with lock_fb_info(), including
store_mode(), show_mode(), and show_modes().

Changes in v3:
- Add patch to clear fb_info->mode in fb_set_var() INV_MODE path.


Melbin K Mathew (3):
  fbdev: bound mode sysfs output to the sysfs buffer
  fbdev: clear fb_info->mode before deleting a videomode
  fbdev: serialize mode sysfs access with lock_fb_info()

 drivers/video/fbdev/core/fbmem.c   |  5 ++-
 drivers/video/fbdev/core/fbsysfs.c | 59 ++++++++++++++++++++++++------
 2 files changed, 51 insertions(+), 13 deletions(-)

-- 
2.39.5

[PATCH v3 1/3] fbdev: bound mode sysfs output to the sysfs buffer

[Melbin K Mathew]

mode_string() uses snprintf() which can return a value larger than the
remaining buffer space. show_modes() accumulates the return value into i
without checking whether i has reached PAGE_SIZE, causing the offset to
advance past the sysfs buffer if the modelist is long enough.

Add a size parameter to mode_string() and use scnprintf() to return
only the bytes actually written. Add an early return when offset
already exceeds the buffer. In show_modes(), stop accumulating once
the buffer is full.

Cc: stable@vger.kernel.org
Signed-off-by: Melbin K Mathew <mlbnkm1@gmail.com>
---
 drivers/video/fbdev/core/fbsysfs.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
index ea196603c7..af21dc5052 100644
--- a/drivers/video/fbdev/core/fbsysfs.c
+++ b/drivers/video/fbdev/core/fbsysfs.c
@@ -27,12 +27,15 @@ static int activate(struct fb_info *fb_info, struct fb_var_screeninfo *var)
 	return 0;
 }
 
-static int mode_string(char *buf, unsigned int offset,
+static int mode_string(char *buf, size_t size, unsigned int offset,
 		       const struct fb_videomode *mode)
 {
 	char m = 'U';
 	char v = 'p';
 
+	if (offset >= size)
+		return 0;
+
 	if (mode->flag & FB_MODE_IS_DETAILED)
 		m = 'D';
 	if (mode->flag & FB_MODE_IS_VESA)
@@ -45,7 +48,7 @@ static int mode_string(char *buf, unsigned int offset,
 	if (mode->vmode & FB_VMODE_DOUBLE)
 		v = 'd';
 
-	return snprintf(&buf[offset], PAGE_SIZE - offset, "%c:%dx%d%c-%d\n",
+	return scnprintf(&buf[offset], size - offset, "%c:%dx%d%c-%d\n",
 	                m, mode->xres, mode->yres, v, mode->refresh);
 }
 
@@ -64,7 +67,7 @@ static ssize_t store_mode(struct device *device, struct device_attribute *attr,
 
 	list_for_each_entry(modelist, &fb_info->modelist, list) {
 		mode = &modelist->mode;
-		i = mode_string(mstr, 0, mode);
+		i = mode_string(mstr, sizeof(mstr), 0, mode);
 		if (strncmp(mstr, buf, max(count, i)) == 0) {
 
 			var = fb_info->var;
@@ -86,7 +89,7 @@ static ssize_t show_mode(struct device *device, struct device_attribute *attr,
 	if (!fb_info->mode)
 		return 0;
 
-	return mode_string(buf, 0, fb_info->mode);
+	return mode_string(buf, PAGE_SIZE, 0, fb_info->mode);
 }
 
 static ssize_t store_modes(struct device *device,
@@ -136,7 +139,9 @@ static ssize_t show_modes(struct device *device, struct device_attribute *attr,
 	i = 0;
 	list_for_each_entry(modelist, &fb_info->modelist, list) {
 		mode = &modelist->mode;
-		i += mode_string(buf, i, mode);
+		i += mode_string(buf, PAGE_SIZE, i, mode);
+		if (i >= PAGE_SIZE - 1)
+			break;
 	}
 	return i;
 }
-- 
2.39.5

Re: [PATCH v3 1/3] fbdev: bound mode sysfs output to the sysfs buffer

[sashiko-bot]

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

Pre-existing issues:
- [High] Unprotected value copy of large `fb_info->var` struct leads to torn reads and state corruption in sysfs store functions.
--

commit 03248859e392cd00678961d93e5c27717ac23498
Author: Melbin K Mathew <mlbnkm1@gmail.com>

fbdev: bound mode sysfs output to the sysfs buffer

This commit updates mode_string() to take a size parameter and use scnprintf()
to prevent buffer overruns when generating sysfs output. It also modifies
show_modes() to stop accumulating mode strings once the sysfs buffer is full.

> diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
> index ea196603c7a87..af21dc5052df3 100644
> --- a/drivers/video/fbdev/core/fbsysfs.c
> +++ b/drivers/video/fbdev/core/fbsysfs.c
[ ... ]
> @@ -64,7 +67,7 @@ static ssize_t store_mode(struct device *device, struct device_attribute *attr,
>  
>  	list_for_each_entry(modelist, &fb_info->modelist, list) {
>  		mode = &modelist->mode;
> -		i = mode_string(mstr, 0, mode);
> +		i = mode_string(mstr, sizeof(mstr), 0, mode);
>  		if (strncmp(mstr, buf, max(count, i)) == 0) {
>  
>  			var = fb_info->var;

[Severity: High]
This is a pre-existing issue, but seeing the struct copy here prompted a
question about other sysfs store functions in this file.

In store_mode(), var = fb_info->var; is protected by lock_fb_info(fb_info)
acquired earlier in the function. However, other store functions in this file
perform this same large struct copy locklessly before acquiring the lock:

drivers/video/fbdev/core/fbsysfs.c:store_bpp() {
    ...
    var = fb_info->var;
    ...
}

Similar unprotected copies occur in store_rotate(), store_virtual(), and
store_pan().

Can these unprotected copies lead to torn reads if executed concurrently with
an ioctl or another sysfs write that modifies the display state?

If a torn read occurs and a single field is updated before passing the torn
structure to activate(), could this commit corrupted state back to the
hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701231706.234715-1-mlbnkm1@gmail.com?part=1

[PATCH v3 2/3] fbdev: clear fb_info->mode before deleting a videomode

[Melbin K Mathew]

fb_set_var() can delete a mode from info->modelist when userspace
passes FB_ACTIVATE_INV_MODE through FBIOPUT_VSCREENINFO. The code
checks that the mode being deleted is not the current info->var and
that fbcon is not using it, but it does not check fb_info->mode.

fb_info->mode may still point into the modelist entry being deleted.
If the entry is freed, later mode sysfs reads through show_mode() can
dereference a stale pointer.

Clear fb_info->mode before calling fb_delete_videomode() when it
matches the mode being removed.

Cc: stable@vger.kernel.org
Signed-off-by: Melbin K Mathew <mlbnkm1@gmail.com>
---
 drivers/video/fbdev/core/fbmem.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 2f1c56e5a7..c8aa163b0e 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -246,8 +246,11 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
 		ret = fb_mode_is_equal(&mode1, &mode2);
 		if (!ret) {
 			ret = fbcon_mode_deleted(info, &mode1);
-			if (!ret)
+			if (!ret) {
+				if (info->mode && fb_mode_is_equal(info->mode, &mode1))
+					info->mode = NULL;
 				fb_delete_videomode(&mode1, &info->modelist);
+			}
 		}
 
 		return ret ? -EINVAL : 0;
-- 
2.39.5

[PATCH v3 3/3] fbdev: serialize mode sysfs access with lock_fb_info()

[Melbin K Mathew]

show_mode(), show_modes(), and store_mode() access fb_info->modelist
and fb_info->mode without holding lock_fb_info(). store_modes() takes
lock_fb_info() while replacing the modelist and freeing the old one.

A concurrent reader or writer can load a pointer to an old modelist
entry before store_modes() frees it, then dereference freed memory or
store a stale freed pointer in fb_info->mode.

Take lock_fb_info() in show_mode(), show_modes(), and store_mode() to
serialize with store_modes(). In show_mode(), copy the mode to the
stack and format after dropping the lock. In store_mode(), split
activate() into a _locked variant to avoid double-locking, and hold
the locks for the modelist walk, mode conversion, activation, and
fb_info->mode assignment together.

Cc: stable@vger.kernel.org
Signed-off-by: Melbin K Mathew <mlbnkm1@gmail.com>
---
 drivers/video/fbdev/core/fbsysfs.c | 46 ++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 8 deletions(-)

diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
index af21dc5052..d3d60c555b 100644
--- a/drivers/video/fbdev/core/fbsysfs.c
+++ b/drivers/video/fbdev/core/fbsysfs.c
@@ -12,19 +12,24 @@
 #include "fb_internal.h"
 #include "fbcon.h"
 
+static int activate_locked(struct fb_info *fb_info,
+			    struct fb_var_screeninfo *var)
+{
+	var->activate |= FB_ACTIVATE_FORCE;
+	return fb_set_var_from_user(fb_info, var);
+}
+
 static int activate(struct fb_info *fb_info, struct fb_var_screeninfo *var)
 {
 	int err;
 
-	var->activate |= FB_ACTIVATE_FORCE;
 	console_lock();
 	lock_fb_info(fb_info);
-	err = fb_set_var_from_user(fb_info, var);
+	err = activate_locked(fb_info, var);
 	unlock_fb_info(fb_info);
 	console_unlock();
-	if (err)
-		return err;
-	return 0;
+
+	return err;
 }
 
 static int mode_string(char *buf, size_t size, unsigned int offset,
@@ -65,6 +70,9 @@ static ssize_t store_mode(struct device *device, struct device_attribute *attr,
 
 	memset(&var, 0, sizeof(var));
 
+	console_lock();
+	lock_fb_info(fb_info);
+
 	list_for_each_entry(modelist, &fb_info->modelist, list) {
 		mode = &modelist->mode;
 		i = mode_string(mstr, sizeof(mstr), 0, mode);
@@ -72,12 +80,22 @@ static ssize_t store_mode(struct device *device, struct device_attribute *attr,
 
 			var = fb_info->var;
 			fb_videomode_to_var(&var, mode);
-			if ((err = activate(fb_info, &var)))
+			err = activate_locked(fb_info, &var);
+			if (err) {
+				unlock_fb_info(fb_info);
+				console_unlock();
 				return err;
+			}
 			fb_info->mode = mode;
+			unlock_fb_info(fb_info);
+			console_unlock();
 			return count;
 		}
 	}
+
+	unlock_fb_info(fb_info);
+	console_unlock();
+
 	return -EINVAL;
 }
 
@@ -85,11 +103,20 @@ static ssize_t show_mode(struct device *device, struct device_attribute *attr,
 			 char *buf)
 {
 	struct fb_info *fb_info = dev_get_drvdata(device);
+	struct fb_videomode mode;
+	bool have_mode = false;
 
-	if (!fb_info->mode)
+	lock_fb_info(fb_info);
+	if (fb_info->mode) {
+		mode = *fb_info->mode;
+		have_mode = true;
+	}
+	unlock_fb_info(fb_info);
+
+	if (!have_mode)
 		return 0;
 
-	return mode_string(buf, PAGE_SIZE, 0, fb_info->mode);
+	return mode_string(buf, PAGE_SIZE, 0, &mode);
 }
 
 static ssize_t store_modes(struct device *device,
@@ -137,12 +164,15 @@ static ssize_t show_modes(struct device *device, struct device_attribute *attr,
 	const struct fb_videomode *mode;
 
 	i = 0;
+	lock_fb_info(fb_info);
 	list_for_each_entry(modelist, &fb_info->modelist, list) {
 		mode = &modelist->mode;
 		i += mode_string(buf, PAGE_SIZE, i, mode);
 		if (i >= PAGE_SIZE - 1)
 			break;
 	}
+	unlock_fb_info(fb_info);
+
 	return i;
 }
 
-- 
2.39.5

Re: [PATCH v3 3/3] fbdev: serialize mode sysfs access with lock_fb_info()

[sashiko-bot]

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

Pre-existing issues:
- [High] show_modes() iterates fb_info->modelist without holding console_lock(), which is required to protect against concurrent list modifications by fbcon.
- [High] Sysfs store handlers (store_bpp, store_rotate, store_virtual, store_pan, store_blank) bypass lock_fb_info(), risking torn reads and concurrent driver execution.
--

commit 52388e2c8c0d0f6bb72f9bedf44a38f798ee348d
Author: Melbin K Mathew <mlbnkm1@gmail.com>

fbdev: serialize mode sysfs access with lock_fb_info()

show_mode(), show_modes(), and store_mode() access fb_info->modelist
and fb_info->mode without holding lock_fb_info(). This commit adds
lock_fb_info() in these functions to serialize with store_modes() and
avoid data races.

Cc: stable@vger.kernel.org

> diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
> index af21dc5052df3..d3d60c555bb86 100644
> --- a/drivers/video/fbdev/core/fbsysfs.c
> +++ b/drivers/video/fbdev/core/fbsysfs.c

[ ... ]

> @@ -137,12 +164,15 @@ static ssize_t show_modes(struct device *device, struct device_attribute *attr,
>  	const struct fb_videomode *mode;
>  
>  	i = 0;
> +	lock_fb_info(fb_info);
>  	list_for_each_entry(modelist, &fb_info->modelist, list) {

[Severity: High]
This is a pre-existing issue, but show_modes() iterates fb_info->modelist
without holding console_lock(), which is required to protect against
concurrent list modifications by fbcon.

Can concurrent VT switches call fbcon_switch() -> fb_set_var() ->
fb_add_videomode() and modify fb_info->modelist under console_lock()
without holding lock_fb_info()? Would an unprivileged user reading
this sysfs attribute risk a torn pointer or out-of-bounds access
since console_lock() is missing here?

>  		mode = &modelist->mode;
>  		i += mode_string(buf, PAGE_SIZE, i, mode);
>  		if (i >= PAGE_SIZE - 1)
>  			break;
>  	}
> +	unlock_fb_info(fb_info);
> +
>  	return i;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but other sysfs store handlers
(store_bpp, store_rotate, store_virtual, store_pan, store_blank) bypass
lock_fb_info(), risking torn reads and concurrent driver execution.

For example, in store_bpp(), store_rotate(), and store_virtual(),
fb_info->var is read locklessly before activating it. In store_pan()
and store_blank(), driver functions like fb_pan_display() or
fb_blank_from_user() are invoked without taking lock_fb_info().

Does this bypass the mutual exclusion that drivers rely on in the
equivalent ioctl paths? Could this result in applying corrupt
configurations to hardware or concurrent execution of non-reentrant
driver code?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701231706.234715-1-mlbnkm1@gmail.com?part=3

Re: [PATCH v3 3/3] fbdev: serialize mode sysfs access with lock_fb_info()

[Melbin K Mathew]

Thanks for the review.

I think these are broader pre-existing fbdev sysfs locking observations 
rather than blockers for this focused series.

This series addresses the mode/modelist lifetime issue and the mode 
sysfs output bounds issue. The modelist lifetime paths touched here are 
now serialized with `lock_fb_info()`, including `store_mode()`, 
`show_mode()`, and `show_modes()`, and the `FB_ACTIVATE_INV_MODE` 
deletion path clears `fb_info->mode` before deleting a matching mode.

For `show_modes()`, `lock_fb_info()` serializes against `store_modes()` 
replacing and freeing `fb_info->modelist`. I do not see fbcon itself 
mutating `fb_info->modelist` in the same way `store_modes()` does, so I 
am not adding `console_lock()` there unless maintainers prefer that 
locking discipline.

The other `fb_info->var` accesses in sysfs store handlers look like a 
separate wider cleanup, which I can look at as a follow-up if 
maintainers prefer.


On 02/07/2026 00:56, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] show_modes() iterates fb_info->modelist without holding console_lock(), which is required to protect against concurrent list modifications by fbcon.
> - [High] Sysfs store handlers (store_bpp, store_rotate, store_virtual, store_pan, store_blank) bypass lock_fb_info(), risking torn reads and concurrent driver execution.
> --
> 
> commit 52388e2c8c0d0f6bb72f9bedf44a38f798ee348d
> Author: Melbin K Mathew <mlbnkm1@gmail.com>
> 
> fbdev: serialize mode sysfs access with lock_fb_info()
> 
> show_mode(), show_modes(), and store_mode() access fb_info->modelist
> and fb_info->mode without holding lock_fb_info(). This commit adds
> lock_fb_info() in these functions to serialize with store_modes() and
> avoid data races.
> 
> Cc: stable@vger.kernel.org
> 
>> diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
>> index af21dc5052df3..d3d60c555bb86 100644
>> --- a/drivers/video/fbdev/core/fbsysfs.c
>> +++ b/drivers/video/fbdev/core/fbsysfs.c
> 
> [ ... ]
> 
>> @@ -137,12 +164,15 @@ static ssize_t show_modes(struct device *device, struct device_attribute *attr,
>>   	const struct fb_videomode *mode;
>>   
>>   	i = 0;
>> +	lock_fb_info(fb_info);
>>   	list_for_each_entry(modelist, &fb_info->modelist, list) {
> 
> [Severity: High]
> This is a pre-existing issue, but show_modes() iterates fb_info->modelist
> without holding console_lock(), which is required to protect against
> concurrent list modifications by fbcon.
> 
> Can concurrent VT switches call fbcon_switch() -> fb_set_var() ->
> fb_add_videomode() and modify fb_info->modelist under console_lock()
> without holding lock_fb_info()? Would an unprivileged user reading
> this sysfs attribute risk a torn pointer or out-of-bounds access
> since console_lock() is missing here?
> 
>>   		mode = &modelist->mode;
>>   		i += mode_string(buf, PAGE_SIZE, i, mode);
>>   		if (i >= PAGE_SIZE - 1)
>>   			break;
>>   	}
>> +	unlock_fb_info(fb_info);
>> +
>>   	return i;
>>   }
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but other sysfs store handlers
> (store_bpp, store_rotate, store_virtual, store_pan, store_blank) bypass
> lock_fb_info(), risking torn reads and concurrent driver execution.
> 
> For example, in store_bpp(), store_rotate(), and store_virtual(),
> fb_info->var is read locklessly before activating it. In store_pan()
> and store_blank(), driver functions like fb_pan_display() or
> fb_blank_from_user() are invoked without taking lock_fb_info().
> 
> Does this bypass the mutual exclusion that drivers rely on in the
> equivalent ioctl paths? Could this result in applying corrupt
> configurations to hardware or concurrent execution of non-reentrant
> driver code?
> 

[PATCH v2 1/2] fbdev: bound mode sysfs output to the sysfs buffer

[Melbin K Mathew]

mode_string() uses snprintf() which can return a value larger than the
remaining buffer space. show_modes() accumulates the return value into i
without checking whether i has reached PAGE_SIZE, causing the offset to
advance past the sysfs buffer if the modelist is long enough.

Add a size parameter to mode_string() and use scnprintf() to return
only the bytes actually written. Add an early return when offset
already exceeds the buffer. In show_modes(), stop accumulating once
the buffer is full.

Cc: stable@vger.kernel.org
Signed-off-by: Melbin K Mathew <mlbnkm1@gmail.com>
---
 drivers/video/fbdev/core/fbsysfs.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
index ea196603c7..af21dc5052 100644
--- a/drivers/video/fbdev/core/fbsysfs.c
+++ b/drivers/video/fbdev/core/fbsysfs.c
@@ -27,12 +27,15 @@ static int activate(struct fb_info *fb_info, struct fb_var_screeninfo *var)
 	return 0;
 }
 
-static int mode_string(char *buf, unsigned int offset,
+static int mode_string(char *buf, size_t size, unsigned int offset,
 		       const struct fb_videomode *mode)
 {
 	char m = 'U';
 	char v = 'p';
 
+	if (offset >= size)
+		return 0;
+
 	if (mode->flag & FB_MODE_IS_DETAILED)
 		m = 'D';
 	if (mode->flag & FB_MODE_IS_VESA)
@@ -45,7 +48,7 @@ static int mode_string(char *buf, unsigned int offset,
 	if (mode->vmode & FB_VMODE_DOUBLE)
 		v = 'd';
 
-	return snprintf(&buf[offset], PAGE_SIZE - offset, "%c:%dx%d%c-%d\n",
+	return scnprintf(&buf[offset], size - offset, "%c:%dx%d%c-%d\n",
 	                m, mode->xres, mode->yres, v, mode->refresh);
 }
 
@@ -64,7 +67,7 @@ static ssize_t store_mode(struct device *device, struct device_attribute *attr,
 
 	list_for_each_entry(modelist, &fb_info->modelist, list) {
 		mode = &modelist->mode;
-		i = mode_string(mstr, 0, mode);
+		i = mode_string(mstr, sizeof(mstr), 0, mode);
 		if (strncmp(mstr, buf, max(count, i)) == 0) {
 
 			var = fb_info->var;
@@ -86,7 +89,7 @@ static ssize_t show_mode(struct device *device, struct device_attribute *attr,
 	if (!fb_info->mode)
 		return 0;
 
-	return mode_string(buf, 0, fb_info->mode);
+	return mode_string(buf, PAGE_SIZE, 0, fb_info->mode);
 }
 
 static ssize_t store_modes(struct device *device,
@@ -136,7 +139,9 @@ static ssize_t show_modes(struct device *device, struct device_attribute *attr,
 	i = 0;
 	list_for_each_entry(modelist, &fb_info->modelist, list) {
 		mode = &modelist->mode;
-		i += mode_string(buf, i, mode);
+		i += mode_string(buf, PAGE_SIZE, i, mode);
+		if (i >= PAGE_SIZE - 1)
+			break;
 	}
 	return i;
 }
-- 
2.39.5

Re: [PATCH v2 1/2] fbdev: bound mode sysfs output to the sysfs buffer

[sashiko-bot]

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

Pre-existing issues:
- [High] Lockless reads of `fb_info->var` cause torn reads and state corruption.
--

commit b5317fd1a6886d886e344587e27b5ae1721b2542
Author: Melbin K Mathew <mlbnkm1@gmail.com>

fbdev: bound mode sysfs output to the sysfs buffer
    
mode_string() uses snprintf() which can return a value larger than the
remaining buffer space. show_modes() accumulates the return value into i
without checking whether i has reached PAGE_SIZE. This commit adds a size
parameter to mode_string() and uses scnprintf() to return only the bytes
actually written.
    
Cc: stable@vger.kernel.org

> diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
> index ea196603c7a87..af21dc5052df3 100644
> --- a/drivers/video/fbdev/core/fbsysfs.c
> +++ b/drivers/video/fbdev/core/fbsysfs.c

[ ... ]

> @@ -64,7 +67,7 @@ static ssize_t store_mode(struct device *device, struct device_attribute *attr,
>  
>  	list_for_each_entry(modelist, &fb_info->modelist, list) {
>  		mode = &modelist->mode;
> -		i = mode_string(mstr, 0, mode);
> +		i = mode_string(mstr, sizeof(mstr), 0, mode);
>  		if (strncmp(mstr, buf, max(count, i)) == 0) {
>  
>  			var = fb_info->var;

[Severity: High]
This isn't a bug introduced by this patch, but while looking at fbsysfs.c,
I noticed several other functions perform a lockless copy of the
fb_info->var struct:

    var = fb_info->var;

This happens in store_bpp(), store_rotate(), store_virtual(), and store_pan().

If this lockless sysfs read races with an ioctl(FBIOPUT_VSCREENINFO)
executing fb_set_var() (which modifies info->var under fb_info->lock),
could it tear the large struct fb_var_screeninfo?

Modifying and writing this corrupted state back to the hardware via activate()
or fb_pan_display() might configure the display controller with invalid 
settings, potentially causing display corruption or crashes in low-level 
graphics drivers.

Should these other functions also serialize their access with lock_fb_info()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701221757.231490-1-mlbnkm1@gmail.com?part=1

[PATCH v2 2/2] fbdev: serialize mode sysfs access with lock_fb_info()

[Melbin K Mathew]

show_mode(), show_modes(), and store_mode() access fb_info->modelist
and fb_info->mode without holding lock_fb_info(). store_modes() takes
lock_fb_info() while replacing the modelist and freeing the old one.

A concurrent reader or writer can load a pointer to an old modelist
entry before store_modes() frees it, then dereference freed memory or
store a stale freed pointer in fb_info->mode.

Take lock_fb_info() in show_mode(), show_modes(), and store_mode() to
serialize with store_modes(). In show_mode(), copy the mode to the
stack and format after dropping the lock. In store_mode(), split
activate() into a _locked variant to avoid double-locking, and hold
the locks for the modelist walk, mode conversion, activation, and
fb_info->mode assignment together.

Cc: stable@vger.kernel.org
Signed-off-by: Melbin K Mathew <mlbnkm1@gmail.com>
---
 drivers/video/fbdev/core/fbsysfs.c | 46 ++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 8 deletions(-)

diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
index af21dc5052..d3d60c555b 100644
--- a/drivers/video/fbdev/core/fbsysfs.c
+++ b/drivers/video/fbdev/core/fbsysfs.c
@@ -12,19 +12,24 @@
 #include "fb_internal.h"
 #include "fbcon.h"
 
+static int activate_locked(struct fb_info *fb_info,
+			    struct fb_var_screeninfo *var)
+{
+	var->activate |= FB_ACTIVATE_FORCE;
+	return fb_set_var_from_user(fb_info, var);
+}
+
 static int activate(struct fb_info *fb_info, struct fb_var_screeninfo *var)
 {
 	int err;
 
-	var->activate |= FB_ACTIVATE_FORCE;
 	console_lock();
 	lock_fb_info(fb_info);
-	err = fb_set_var_from_user(fb_info, var);
+	err = activate_locked(fb_info, var);
 	unlock_fb_info(fb_info);
 	console_unlock();
-	if (err)
-		return err;
-	return 0;
+
+	return err;
 }
 
 static int mode_string(char *buf, size_t size, unsigned int offset,
@@ -65,6 +70,9 @@ static ssize_t store_mode(struct device *device, struct device_attribute *attr,
 
 	memset(&var, 0, sizeof(var));
 
+	console_lock();
+	lock_fb_info(fb_info);
+
 	list_for_each_entry(modelist, &fb_info->modelist, list) {
 		mode = &modelist->mode;
 		i = mode_string(mstr, sizeof(mstr), 0, mode);
@@ -72,12 +80,22 @@ static ssize_t store_mode(struct device *device, struct device_attribute *attr,
 
 			var = fb_info->var;
 			fb_videomode_to_var(&var, mode);
-			if ((err = activate(fb_info, &var)))
+			err = activate_locked(fb_info, &var);
+			if (err) {
+				unlock_fb_info(fb_info);
+				console_unlock();
 				return err;
+			}
 			fb_info->mode = mode;
+			unlock_fb_info(fb_info);
+			console_unlock();
 			return count;
 		}
 	}
+
+	unlock_fb_info(fb_info);
+	console_unlock();
+
 	return -EINVAL;
 }
 
@@ -85,11 +103,20 @@ static ssize_t show_mode(struct device *device, struct device_attribute *attr,
 			 char *buf)
 {
 	struct fb_info *fb_info = dev_get_drvdata(device);
+	struct fb_videomode mode;
+	bool have_mode = false;
 
-	if (!fb_info->mode)
+	lock_fb_info(fb_info);
+	if (fb_info->mode) {
+		mode = *fb_info->mode;
+		have_mode = true;
+	}
+	unlock_fb_info(fb_info);
+
+	if (!have_mode)
 		return 0;
 
-	return mode_string(buf, PAGE_SIZE, 0, fb_info->mode);
+	return mode_string(buf, PAGE_SIZE, 0, &mode);
 }
 
 static ssize_t store_modes(struct device *device,
@@ -137,12 +164,15 @@ static ssize_t show_modes(struct device *device, struct device_attribute *attr,
 	const struct fb_videomode *mode;
 
 	i = 0;
+	lock_fb_info(fb_info);
 	list_for_each_entry(modelist, &fb_info->modelist, list) {
 		mode = &modelist->mode;
 		i += mode_string(buf, PAGE_SIZE, i, mode);
 		if (i >= PAGE_SIZE - 1)
 			break;
 	}
+	unlock_fb_info(fb_info);
+
 	return i;
 }
 
-- 
2.39.5

Re: [PATCH v2 2/2] fbdev: serialize mode sysfs access with lock_fb_info()

[sashiko-bot]

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

Pre-existing issues:
- [High] fb_info->mode can become a dangling pointer if the active mode is deleted via ioctl(FBIOPUT_VSCREENINFO) with FB_ACTIVATE_INV_MODE, leading to a Use-After-Free in show_mode().
--

commit 243c82e865602245e21ae16b7cdd7738ccfa6a53
Author: Melbin K Mathew <mlbnkm1@gmail.com>

fbdev: serialize mode sysfs access with lock_fb_info()

Take lock_fb_info() in show_mode(), show_modes(), and store_mode() to
serialize with store_modes(). In show_mode(), copy the mode to the
stack and format after dropping the lock. In store_mode(), split
activate() into a _locked variant to avoid double-locking.

> diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
> index af21dc5052df3..d3d60c555bb86 100644
> --- a/drivers/video/fbdev/core/fbsysfs.c
> +++ b/drivers/video/fbdev/core/fbsysfs.c

[ ... ]

> @@ -85,11 +103,20 @@ static ssize_t show_mode(struct device *device, struct device_attribute *attr,
>  			 char *buf)
>  {
>  	struct fb_info *fb_info = dev_get_drvdata(device);
> +	struct fb_videomode mode;
> +	bool have_mode = false;
>  
> -	if (!fb_info->mode)
> +	lock_fb_info(fb_info);
> +	if (fb_info->mode) {
> +		mode = *fb_info->mode;

[Severity: High]
This is a pre-existing issue, but can fb_info->mode become a dangling pointer
and lead to a use-after-free here?

If the active mode is deleted via ioctl(FBIOPUT_VSCREENINFO) with
FB_ACTIVATE_INV_MODE, it appears fb_delete_videomode() unlinks and frees
the mode node:

drivers/video/fbdev/core/modedb.c:fb_delete_videomode() {
    ...
    list_del(pos);
    kfree(pos);
    ...
}

However, it does not seem to clear the fb_info->mode reference. If a user
subsequently reads /sys/class/graphics/fb0/mode, would this dereference the
freed memory even while holding the lock?

> +		have_mode = true;
> +	}
> +	unlock_fb_info(fb_info);
> +
> +	if (!have_mode)
>  		return 0;
>  
> -	return mode_string(buf, PAGE_SIZE, 0, fb_info->mode);
> +	return mode_string(buf, PAGE_SIZE, 0, &mode);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701221757.231490-1-mlbnkm1@gmail.com?part=2