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

[Weiming Shi]

Helge Deller <deller@xxxxxx> 于2026年10月25日周日 12:38写道：
>
> 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@xxxxxxxxxxxxxxx
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Reported-by: <co+b0df61d49532b2ee@xxxxxxx>
> > Assisted-by: LLM
> > Signed-off-by: Weiming Shi <bestswngs@xxxxxxxxx>
> > ---
> >   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

Thanks for your review. v2 sent.