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 <[email protected]> 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: [email protected] > 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/[email protected]?part=3
