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