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

[Helge Deller <deller@gmx.de>]

On 10/29/26 09:09, Geert Uytterhoeven wrote:
> Hi Weiming,
> 
> On Tue, 27 Oct 2026 at 17:16, Weiming Shi <bestswngs@gmail.com> wrote:
>> FBIOPUT_VSCREENINFO can supply a virtual width large enough to
>> overflow get_line_length()'s int multiplication and bypass the
>> video-memory limit, allowing fbcon to write past the framebuffer. The
>> offset additions can also wrap, while an excessive videomemorysize
>> module parameter can be truncated when PAGE_ALIGN() is assigned to
>> unsigned int.
>>
>> Cap framebuffer geometry at 8192 x 8192 and video memory at 512 MiB,
>> keeping the stride, framebuffer-size, and generic fbdev drawing
>> calculations within their destination types.
>>
>> 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
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>> Reported-by: <co+b0df61d49532b2ee@bugs.sh>
>> Suggested-by: Helge Deller <deller@gmx.de>
>> Assisted-by: LLM
>> Signed-off-by: Weiming Shi <bestswngs@gmail.com>
> 
> Thanks for your patch, which is now commit f93dd31db3e23e61 ("fbdev:
> vfb: Validate framebuffer geometry and memory size") in fbdev/for-next.
> 
>> --- a/drivers/video/fbdev/vfb.c
>> +++ b/drivers/video/fbdev/vfb.c
>> @@ -31,6 +31,9 @@
>>        */
>>
>>   #define VIDEOMEMSIZE   (1*1024*1024)   /* 1 MB */
>> +#define MAX_VIDEOMEMSIZE       (512UL * 1024 * 1024)
> 
> Is there any point in having a larger limit than 256 MiB
> (i.e. MAX_XRES * MAX_YRES * 32 bpp)?
> 
>> +#define MAX_XRES               8192U
>> +#define MAX_YRES               8192U
>>
>>   static void *videomemory;
>>   static u_long videomemorysize = VIDEOMEMSIZE;
>> @@ -142,6 +145,10 @@ static int vfb_check_var(struct fb_var_screeninfo *var,
>>                  var->xres = 1;
>>          if (!var->yres)
>>                  var->yres = 1;
>> +       var->xres = min(var->xres, MAX_XRES);
>> +       var->yres = min(var->yres, MAX_YRES);
>> +       var->xres_virtual = min(var->xres_virtual, MAX_XRES);
>> +       var->yres_virtual = min(var->yres_virtual, MAX_YRES);
> 
> This violates fbdev API rounding rules:
>    1. If a value doesn't fit, it must be rounded up by the driver,
>    2. If a value is too large, -EINVAL must be returned.

Thanks for your review, Geert!
You are right. It's still documented like that in skeletonfb.c
Weiming changed it based on my feedback.
Weiming, no need to resend the patch.
I'll pick up one of your former patches.

Thanks!
Helge

> 
>>          if (var->xres > var->xres_virtual)
>>                  var->xres_virtual = var->xres;
>>          if (var->yres > var->yres_virtual)
>> @@ -159,6 +166,9 @@ static int vfb_check_var(struct fb_var_screeninfo *var,
>>          else
>>                  return -EINVAL;
>>
>> +       var->xoffset = min(var->xoffset, MAX_XRES - var->xres);
>> +       var->yoffset = min(var->yoffset, MAX_YRES - var->yres);
> 
> Likewise.
> 
>> +
>>          if (var->xres_virtual < var->xoffset + var->xres)
>>                  var->xres_virtual = var->xoffset + var->xres;
>>          if (var->yres_virtual < var->yoffset + var->yres)
>> @@ -434,9 +444,12 @@ 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;
>>
>> +       videomemorysize = min(videomemorysize, MAX_VIDEOMEMSIZE);
>> +       size = PAGE_ALIGN(videomemorysize);
> 
> Perhaps return -EINVAL, too?
> 
>> +
>>          /*
>>           * For real video cards we use ioremap.
>>           */
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds