Re: [PATCH] fbcon: fix NULL pointer dereference for a console without vc_data

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