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-...@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 <mlbn...@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: sta...@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?