diff mbox series

[4/5] fbmem: Prevent invalid virtual screen sizes in fb_set_var()

Message ID 20220629200024.187187-5-deller@gmx.de (mailing list archive)
State Superseded
Headers show
Series fbcon: Fixes for screen resolution changes - round 2 | expand

Commit Message

Helge Deller June 29, 2022, 8 p.m. UTC 
Prevent that drivers configure a virtual screen resolution smaller than
the physical screen resolution.  This is important, because otherwise we
may access memory outside of the graphics memory area.

Signed-off-by: Helge Deller <deller@gmx.de>
Cc: stable@vger.kernel.org # v5.4+
---
 drivers/video/fbdev/core/fbmem.c | 6 ++++++
 1 file changed, 6 insertions(+)

--
2.35.3

Comments

Geert Uytterhoeven June 30, 2022, 7:11 p.m. UTC | #1 
Hi Helge,

On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@gmx.de> wrote:
> Prevent that drivers configure a virtual screen resolution smaller than
> the physical screen resolution.  This is important, because otherwise we
> may access memory outside of the graphics memory area.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
> Cc: stable@vger.kernel.org # v5.4+

Thanks for your patch!

> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1006,6 +1006,12 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
>         if (var->xres < 8 || var->yres < 8)
>                 return -EINVAL;
>
> +       /* make sure virtual resolution >= physical resolution */
> +       if (WARN_ON(var->xres_virtual < var->xres))
> +               var->xres_virtual = var->xres;
> +       if (WARN_ON(var->yres_virtual < var->yres))
> +               var->yres_virtual = var->yres;

This should be moved below the call to info->fbops->fb_check_var(),
so the WARN_ON() catches buggy fbdev drivers, not userspace fuzzers.

> +
>         /* Too huge resolution causes multiplication overflow. */
>         if (check_mul_overflow(var->xres, var->yres, &unused) ||
>             check_mul_overflow(var->xres_virtual, var->yres_virtual, &unused))

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
Helge Deller June 30, 2022, 7:16 p.m. UTC | #2 
On 6/30/22 21:11, Geert Uytterhoeven wrote:
> Hi Helge,
>
> On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@gmx.de> wrote:
>> Prevent that drivers configure a virtual screen resolution smaller than
>> the physical screen resolution.  This is important, because otherwise we
>> may access memory outside of the graphics memory area.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>> Cc: stable@vger.kernel.org # v5.4+
>
> Thanks for your patch!
>
>> --- a/drivers/video/fbdev/core/fbmem.c
>> +++ b/drivers/video/fbdev/core/fbmem.c
>> @@ -1006,6 +1006,12 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
>>         if (var->xres < 8 || var->yres < 8)
>>                 return -EINVAL;
>>
>> +       /* make sure virtual resolution >= physical resolution */
>> +       if (WARN_ON(var->xres_virtual < var->xres))
>> +               var->xres_virtual = var->xres;
>> +       if (WARN_ON(var->yres_virtual < var->yres))
>> +               var->yres_virtual = var->yres;
>
> This should be moved below the call to info->fbops->fb_check_var(),
> so the WARN_ON() catches buggy fbdev drivers, not userspace fuzzers.

Yes, makes sense.

THX,
Helge


>> +
>>         /* Too huge resolution causes multiplication overflow. */
>>         if (check_mul_overflow(var->xres, var->yres, &unused) ||
>>             check_mul_overflow(var->xres_virtual, var->yres_virtual, &unused))
>
> 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
Geert Uytterhoeven June 30, 2022, 7:38 p.m. UTC | #3 
Hi Helge,

On Thu, Jun 30, 2022 at 9:17 PM Helge Deller <deller@gmx.de> wrote:
> On 6/30/22 21:11, Geert Uytterhoeven wrote:
> > On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@gmx.de> wrote:
> >> Prevent that drivers configure a virtual screen resolution smaller than
> >> the physical screen resolution.  This is important, because otherwise we
> >> may access memory outside of the graphics memory area.
> >>
> >> Signed-off-by: Helge Deller <deller@gmx.de>
> >> Cc: stable@vger.kernel.org # v5.4+
> >
> > Thanks for your patch!
> >
> >> --- a/drivers/video/fbdev/core/fbmem.c
> >> +++ b/drivers/video/fbdev/core/fbmem.c
> >> @@ -1006,6 +1006,12 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
> >>         if (var->xres < 8 || var->yres < 8)
> >>                 return -EINVAL;
> >>
> >> +       /* make sure virtual resolution >= physical resolution */
> >> +       if (WARN_ON(var->xres_virtual < var->xres))
> >> +               var->xres_virtual = var->xres;
> >> +       if (WARN_ON(var->yres_virtual < var->yres))
> >> +               var->yres_virtual = var->yres;
> >
> > This should be moved below the call to info->fbops->fb_check_var(),
> > so the WARN_ON() catches buggy fbdev drivers, not userspace fuzzers.
>
> Yes, makes sense.

And print the name of the frame buffer device driver, so people know
who to blame.

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
Geert Uytterhoeven July 1, 2022, 2:49 p.m. UTC | #4 
Hi Helge,

On Thu, Jun 30, 2022 at 9:38 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Thu, Jun 30, 2022 at 9:17 PM Helge Deller <deller@gmx.de> wrote:
> > On 6/30/22 21:11, Geert Uytterhoeven wrote:
> > > On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@gmx.de> wrote:
> > >> Prevent that drivers configure a virtual screen resolution smaller than
> > >> the physical screen resolution.  This is important, because otherwise we
> > >> may access memory outside of the graphics memory area.
> > >>
> > >> Signed-off-by: Helge Deller <deller@gmx.de>
> > >> Cc: stable@vger.kernel.org # v5.4+
> > >
> > > Thanks for your patch!
> > >
> > >> --- a/drivers/video/fbdev/core/fbmem.c
> > >> +++ b/drivers/video/fbdev/core/fbmem.c
> > >> @@ -1006,6 +1006,12 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
> > >>         if (var->xres < 8 || var->yres < 8)
> > >>                 return -EINVAL;
> > >>
> > >> +       /* make sure virtual resolution >= physical resolution */
> > >> +       if (WARN_ON(var->xres_virtual < var->xres))
> > >> +               var->xres_virtual = var->xres;
> > >> +       if (WARN_ON(var->yres_virtual < var->yres))
> > >> +               var->yres_virtual = var->yres;
> > >
> > > This should be moved below the call to info->fbops->fb_check_var(),
> > > so the WARN_ON() catches buggy fbdev drivers, not userspace fuzzers.
> >
> > Yes, makes sense.
>
> And print the name of the frame buffer device driver, so people know
> who to blame.

Or better, do not continue, but return with a failure:

    if (WARN(var->xres_virtual < var->xres || var->yres_virtual < var->yres,
        "%ps for %s is broken\n", info->fbops->fb_check_var, info->fix.id)
            return -EINVAL;

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
Michel Dänzer July 2, 2022, 12:05 p.m. UTC | #5 
On 2022-07-01 16:49, Geert Uytterhoeven wrote:
> On Thu, Jun 30, 2022 at 9:38 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Thu, Jun 30, 2022 at 9:17 PM Helge Deller <deller@gmx.de> wrote:
>>> On 6/30/22 21:11, Geert Uytterhoeven wrote:
>>>> On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@gmx.de> wrote:
>>>>> Prevent that drivers configure a virtual screen resolution smaller than
>>>>> the physical screen resolution.  This is important, because otherwise we
>>>>> may access memory outside of the graphics memory area.
>>>>>
>>>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>>> Cc: stable@vger.kernel.org # v5.4+
>>>>
>>>> Thanks for your patch!
>>>>
>>>>> --- a/drivers/video/fbdev/core/fbmem.c
>>>>> +++ b/drivers/video/fbdev/core/fbmem.c
>>>>> @@ -1006,6 +1006,12 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
>>>>>         if (var->xres < 8 || var->yres < 8)
>>>>>                 return -EINVAL;
>>>>>
>>>>> +       /* make sure virtual resolution >= physical resolution */
>>>>> +       if (WARN_ON(var->xres_virtual < var->xres))
>>>>> +               var->xres_virtual = var->xres;
>>>>> +       if (WARN_ON(var->yres_virtual < var->yres))
>>>>> +               var->yres_virtual = var->yres;
>>>>
>>>> This should be moved below the call to info->fbops->fb_check_var(),
>>>> so the WARN_ON() catches buggy fbdev drivers, not userspace fuzzers.
>>>
>>> Yes, makes sense.
>>
>> And print the name of the frame buffer device driver, so people know
>> who to blame.
> 
> Or better, do not continue, but return with a failure:
> 
>     if (WARN(var->xres_virtual < var->xres || var->yres_virtual < var->yres,
>         "%ps for %s is broken\n", info->fbops->fb_check_var, info->fix.id)
>             return -EINVAL;

I'd also recommend WARN(_ON)_ONCE, or users with a broken driver might get spammed.
Helge Deller July 2, 2022, 4:26 p.m. UTC | #6 
On 7/2/22 14:05, Michel Dänzer wrote:
> On 2022-07-01 16:49, Geert Uytterhoeven wrote:
>> On Thu, Jun 30, 2022 at 9:38 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Thu, Jun 30, 2022 at 9:17 PM Helge Deller <deller@gmx.de> wrote:
>>>> On 6/30/22 21:11, Geert Uytterhoeven wrote:
>>>>> On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@gmx.de> wrote:
>>>>>> Prevent that drivers configure a virtual screen resolution smaller than
>>>>>> the physical screen resolution.  This is important, because otherwise we
>>>>>> may access memory outside of the graphics memory area.
>>>>>>
>>>>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>>>> Cc: stable@vger.kernel.org # v5.4+
>>>>>
>>>>> Thanks for your patch!
>>>>>
>>>>>> --- a/drivers/video/fbdev/core/fbmem.c
>>>>>> +++ b/drivers/video/fbdev/core/fbmem.c
>>>>>> @@ -1006,6 +1006,12 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
>>>>>>         if (var->xres < 8 || var->yres < 8)
>>>>>>                 return -EINVAL;
>>>>>>
>>>>>> +       /* make sure virtual resolution >= physical resolution */
>>>>>> +       if (WARN_ON(var->xres_virtual < var->xres))
>>>>>> +               var->xres_virtual = var->xres;
>>>>>> +       if (WARN_ON(var->yres_virtual < var->yres))
>>>>>> +               var->yres_virtual = var->yres;
>>>>>
>>>>> This should be moved below the call to info->fbops->fb_check_var(),
>>>>> so the WARN_ON() catches buggy fbdev drivers, not userspace fuzzers.
>>>>
>>>> Yes, makes sense.
>>>
>>> And print the name of the frame buffer device driver, so people know
>>> who to blame.
>>
>> Or better, do not continue, but return with a failure:
>>
>>     if (WARN(var->xres_virtual < var->xres || var->yres_virtual < var->yres,
>>         "%ps for %s is broken\n", info->fbops->fb_check_var, info->fix.id)
>>             return -EINVAL;
>
> I'd also recommend WARN(_ON)_ONCE, or users with a broken driver might get spammed.

Yes, that's probably better. Will do.

Thanks!
Helge
diff mbox series

Patch

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 324f726739c4..222d94e2e0a2 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1006,6 +1006,12 @@  fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
 	if (var->xres < 8 || var->yres < 8)
 		return -EINVAL;

+	/* make sure virtual resolution >= physical resolution */
+	if (WARN_ON(var->xres_virtual < var->xres))
+		var->xres_virtual = var->xres;
+	if (WARN_ON(var->yres_virtual < var->yres))
+		var->yres_virtual = var->yres;
+
 	/* Too huge resolution causes multiplication overflow. */
 	if (check_mul_overflow(var->xres, var->yres, &unused) ||
 	    check_mul_overflow(var->xres_virtual, var->yres_virtual, &unused))