On Wed, Jun 24, 2026 at 09:30:26PM +0000, [email protected] wrote:
> 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():
This is a seperate bug that I'm also working on a patch for (although it
is proving a bit more difficult to find the correct place to make the
change). The NULL mode that fbcon_switch() reaches through
display_to_var() is set by var_to_display() at
drivers/video/fbdev/core/fbcon.c:988, which assigns disp->mode the result of
fb_match_mode(). On the no-match path that result is NULL, independent of
this patch. The p->mode = NULL added to fbcon_deinit() only applies to a
console that has been deinitialised and has no vc_data. fbcon_new_modelist()
skips such a console through its existing mode check, and the console cannot
be switched to without first being re-initialised, which re-runs
var_to_display() and overwrites the mode. So this patch does not add a path
that reaches display_to_var() with a NULL mode. The dereference is reached
when the modelist is replaced from userspace while fbcon is unbound.
>
> 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?
>
The block quoted sits below the hunk, and this patch only moves the vc_data
check ahead of var_to_display(). That block is reached only after
var_to_display() succeeds and vc is non-NULL, which holds in both the old and
the new order, so its reachability is unchanged with this patch.
Ian
> 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
