Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Pre-existing NULL pointer dereference via `font_data_get(NULL)` in `fbcon_set_disp()` if the foreground console's font is uninitialized. - [High] Pre-existing NULL pointer dereference in `fb_videomode_to_var()` when a console is switched to or resized while its `mode` is NULL. -- commit 6f0719a9bd0a2231fe53bfe57ca1abf9ddec2785 Author: Ian Bridges <[email protected]> fbcon: fix NULL pointer dereference for a console without vc_data This commit fixes a NULL pointer dereference in fbcon_new_modelist(). It does this by ensuring a console's mode is only set while it has a valid vc_data structure. Closes: https://syzkaller.appspot.com/bug?extid=42525d636f430fd5d983 > diff --git a/drivers/video/fbdev/core/fbcon.c > b/drivers/video/fbdev/core/fbcon.c > index b0e3e765360d1..7187be0eea872 100644 > --- a/drivers/video/fbdev/core/fbcon.c > +++ b/drivers/video/fbdev/core/fbcon.c > @@ -1274,6 +1274,7 @@ static void fbcon_deinit(struct vc_data *vc) > int idx; > > fbcon_free_font(p); > + p->mode = NULL; [Severity: High] This is a pre-existing issue, but does setting p->mode to NULL here expose us to a crash during a VT switch? If a subsequent fbcon_init() fails early (e.g. var_to_display() fails to find a matching mode), p->mode will remain NULL. Later, if a user switches to this VT, the VT layer calls fbcon_switch(), which unconditionally passes p to display_to_var(): drivers/video/fbdev/core/fbcon.c:fbcon_switch() { ... memset(&var, 0, sizeof(struct fb_var_screeninfo)); display_to_var(&var, p); ... } display_to_var() then calls fb_videomode_to_var(): drivers/video/fbdev/core/fbcon.c:display_to_var() { ... fb_videomode_to_var(var, disp->mode); ... } And fb_videomode_to_var() immediately dereferences the pointer: drivers/video/fbdev/core/modedb.c:fb_videomode_to_var() { ... var->xres = mode->xres; ... } Can this lead to a NULL pointer dereference if the modelist is altered from userspace? > idx = con2fb_map[vc->vc_num]; > > if (idx == -1) [ ... ] > @@ -1445,14 +1446,14 @@ static void fbcon_set_disp(struct fb_info *info, > struct fb_var_screeninfo *var, > > p = &fb_display[unit]; > > - if (var_to_display(p, var, info)) > - return; > - > vc = vc_cons[unit].d; > > if (!vc) > return; > > + if (var_to_display(p, var, info)) > + return; > + > default_mode = vc->vc_display_fg; > svc = *default_mode; > t = &fb_display[svc->vc_num]; [Severity: High] This is a pre-existing issue, but can this lead to a NULL pointer dereference in font_data_get() if the foreground console's font is uninitialized? Just below this hunk in fbcon_set_disp(), the code does: if (!p->fontdata) { ... p->fontdata = t->fontdata; font_data_get(p->fontdata); } If fbcon_init() previously failed early for the foreground console, its t->fontdata could be NULL. This assigns NULL to p->fontdata and then passes it to font_data_get(): lib/fonts/fonts.c:font_data_get() { if (font_data_is_internal(fd)) return; /* never ref static data */ if (WARN_ON(!REFCOUNT(fd))) return; /* should never be 0 */ ++REFCOUNT(fd); } The REFCOUNT() macro and font_data_is_internal() will unconditionally dereference fd, causing a crash if it is NULL. Does this need an extra NULL check? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/ajxICA0v-_5NrqhS@dev?part=1
