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

[sashiko-bot@kernel.org]

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 <icb@fastmail.org>

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