Re: [PATCH] fbdev: vfb: Validate framebuffer geometry and memory size

[Helge Deller <deller@gmx.de>]

On 10/23/26 19:51, Weiming Shi wrote:
> get_line_length() multiplies two int operands before assigning the
> result to u_long. Setting xres_virtual to 0x8000000 at 32 bpp therefore

A xres_virtual value of 134217728 seems quite unrealistic IMHO...

> wraps the product to zero and bypasses vfb_check_var()'s video-memory
> limit. fbcon can then clear margins with a zero line stride and write
> past the vmalloc'ed framebuffer.
> 
> The xoffset + xres and yoffset + yres calculations can likewise wrap,
> leaving an invalid offset in info->var after fb_pan_display() rejects
> it. Subsequent fbcon drawing can then access far beyond the framebuffer.
> 
> Finally, PAGE_ALIGN(videomemorysize) was narrowed from unsigned long to
> unsigned int. Values near or above 4 GiB could therefore allocate a much
> smaller buffer than the size used for mode validation.
> 
> KASAN reports:
> 
>    BUG: KASAN: vmalloc-out-of-bounds in sys_fillrect
>      drivers/video/fbdev/core/sysfillrect.c:24
>    Write of size 8 at addr ffa0000001671000 by task poc/198
>    sys_fillrect
>      drivers/video/fbdev/core/sysfillrect.c:24
>    bit_clear_margins
>      drivers/video/fbdev/core/bitblit.c:247
>    fbcon_clear_margins.constprop.0
>      drivers/video/fbdev/core/fbcon.c:1410
>    fbcon_switch
>      drivers/video/fbdev/core/fbcon.c:2261
>    redraw_screen
>      drivers/tty/vt/vt.c:994
>    fbcon_modechanged
>      drivers/video/fbdev/core/fbcon.c:2693
>    fbcon_update_vcs
>      drivers/video/fbdev/core/fbcon.c:2732
> 
> Reject video-memory sizes whose aligned value is not representable by
> the fbdev metadata. Check the offset additions for overflow. Compute the
> line length and framebuffer size in u64, reject unrepresentable line
> lengths, and check the total-size multiplication before comparing it
> with the allocated video-memory size.
> 
> Cc: stable@vger.kernel.org
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: <co+b0df61d49532b2ee@bugs.sh>
> Assisted-by: LLM
> Signed-off-by: Weiming Shi <bestswngs@gmail.com>
> ---
>   drivers/video/fbdev/vfb.c | 31 +++++++++++++++++++++----------
>   1 file changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/video/fbdev/vfb.c b/drivers/video/fbdev/vfb.c
> index de137b2bd..84b0a937c 100644
> --- a/drivers/video/fbdev/vfb.c
> +++ b/drivers/video/fbdev/vfb.c
> @@ -101,11 +101,11 @@ static const struct fb_ops vfb_ops = {
>        *  Internal routines
>        */
>   
> -static u_long get_line_length(int xres_virtual, int bpp)
> +static u64 get_line_length(u32 xres_virtual, u32 bpp)


Instead of trying to extend to 64bit, I think we should simply
limit the maximum allowed screen sizes to e.g. 4096 x 4096
(or some other realistic values).


>   {
> -	u_long length;
> +	u64 length;
>   
> -	length = xres_virtual * bpp;
> +	length = (u64)xres_virtual * bpp;
>   	length = (length + 31) & ~31;
>   	length >>= 3;
>   	return (length);
> @@ -122,7 +122,8 @@ static u_long get_line_length(int xres_virtual, int bpp)
>   static int vfb_check_var(struct fb_var_screeninfo *var,
>   			 struct fb_info *info)
>   {
> -	u_long line_length;
> +	u64 line_length, size;
> +	u32 xres, yres;
>   
>   	/*
>   	 *  FB_VMODE_CONUPDATE and FB_VMODE_SMOOTH_XPAN are equal!
> @@ -159,17 +160,23 @@ static int vfb_check_var(struct fb_var_screeninfo *var,
>   	else
>   		return -EINVAL;
>   
> -	if (var->xres_virtual < var->xoffset + var->xres)
> -		var->xres_virtual = var->xoffset + var->xres;
> -	if (var->yres_virtual < var->yoffset + var->yres)
> -		var->yres_virtual = var->yoffset + var->yres;
> +	if (check_add_overflow(var->xoffset, var->xres, &xres) ||
> +	    check_add_overflow(var->yoffset, var->yres, &yres))
> +		return -EINVAL;
> +
> +	if (var->xres_virtual < xres)
> +		var->xres_virtual = xres;
> +	if (var->yres_virtual < yres)
> +		var->yres_virtual = yres;
>   
>   	/*
>   	 *  Memory limit
>   	 */
>   	line_length =
>   	    get_line_length(var->xres_virtual, var->bits_per_pixel);
> -	if (line_length * var->yres_virtual > videomemorysize)
> +	if (line_length > U32_MAX ||
> +	    check_mul_overflow(line_length, (u64)var->yres_virtual, &size) ||
> +	    size > videomemorysize)
>   		return -ENOMEM;
>   
>   	/*
> @@ -434,9 +441,13 @@ static int __init vfb_setup(char *options)
>   static int vfb_probe(struct platform_device *dev)
>   {
>   	struct fb_info *info;
> -	unsigned int size = PAGE_ALIGN(videomemorysize);
> +	unsigned int size;
>   	int retval = -ENOMEM;
>   
> +	if (videomemorysize > U32_MAX - (PAGE_SIZE - 1))
> +		return -EINVAL;
> +	size = PAGE_ALIGN(videomemorysize);

same here.
There doesn't seem to be a maximum limit for videomemorysize.
Maybe adding a check and allow a maximum of e.g. 512MB ?

Will you send a revised patch?

Helge