[v6,3/4] fbcon: Prevent that screen size is smaller than font size

Commit Message

Helge Deller June 26, 2022, 10:28 a.m. UTC

We need to prevent that users configure a screen size which is smaller than the
currently selected font size. Otherwise rendering chars on the screen will
access memory outside the graphics memory region.

This patch adds a new function fbcon_modechange_possible() which
implements this check and which later may be extended with other checks
if necessary.  The new function is called from the FBIOPUT_VSCREENINFO
ioctl handler in fbmem.c, which will return -EINVAL if userspace asked
for a too small screen size.

Signed-off-by: Helge Deller <[e-mail]>
Reviewed-by: Daniel Vetter <[e-mail]>
Cc: [e-mail] # v5.4+
---
 drivers/video/fbdev/core/fbcon.c | 27 +++++++++++++++++++++++++++
 drivers/video/fbdev/core/fbmem.c |  4 +++-
 include/linux/fbcon.h            |  4 ++++
 3 files changed, 34 insertions(+), 1 deletion(-)

--
2.35.3

Comments

Geert Uytterhoeven June 28, 2022, 8:39 a.m. UTC | #1

Hi Helge,

On Sun, Jun 26, 2022 at 12:33 PM Helge Deller <[e-mail]> wrote:
> We need to prevent that users configure a screen size which is smaller than the
> currently selected font size. Otherwise rendering chars on the screen will
> access memory outside the graphics memory region.
>
> This patch adds a new function fbcon_modechange_possible() which
> implements this check and which later may be extended with other checks
> if necessary.  The new function is called from the FBIOPUT_VSCREENINFO
> ioctl handler in fbmem.c, which will return -EINVAL if userspace asked
> for a too small screen size.
>
> Signed-off-by: Helge Deller <[e-mail]>
> Reviewed-by: Daniel Vetter <[e-mail]>
> Cc: [e-mail] # v5.4+

Thanks for your patch, which is now commit f0b6a66d33ca6e7e ("fbcon:
Prevent that screen size is smaller than font size") in fbdev/for-next

> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c

> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1112,7 +1112,9 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
>                         return -EFAULT;
>                 console_lock();
>                 lock_fb_info(info);
> -               ret = fb_set_var(info, &var);
> +               ret = fbcon_modechange_possible(info, &var);

Again, this should be done (if done at all) after the call to
fb_ops.check_var(), as it breaks the FBIOPUT_VSCREENINFO rounding rule.

What if the user just wants to display graphics, not text?
Can't the text console be disabled instead?

> +               if (!ret)
> +                       ret = fb_set_var(info, &var);
>                 if (!ret)
>                         fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL);
>                 unlock_fb_info(info);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

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 28, 2022, 8:52 p.m. UTC | #2

On 6/28/22 10:39, Geert Uytterhoeven wrote:
> Hi Helge,
>
> On Sun, Jun 26, 2022 at 12:33 PM Helge Deller <[e-mail]> wrote:
>> We need to prevent that users configure a screen size which is smaller than the
>> currently selected font size. Otherwise rendering chars on the screen will
>> access memory outside the graphics memory region.
>>
>> This patch adds a new function fbcon_modechange_possible() which
>> implements this check and which later may be extended with other checks
>> if necessary.  The new function is called from the FBIOPUT_VSCREENINFO
>> ioctl handler in fbmem.c, which will return -EINVAL if userspace asked
>> for a too small screen size.
>>
>> Signed-off-by: Helge Deller <[e-mail]>
>> Reviewed-by: Daniel Vetter <[e-mail]>
>> Cc: [e-mail] # v5.4+
>
> Thanks for your patch, which is now commit f0b6a66d33ca6e7e ("fbcon:
> Prevent that screen size is smaller than font size") in fbdev/for-next
>
>> --- a/drivers/video/fbdev/core/fbcon.c
>> +++ b/drivers/video/fbdev/core/fbcon.c
>
>> --- a/drivers/video/fbdev/core/fbmem.c
>> +++ b/drivers/video/fbdev/core/fbmem.c
>> @@ -1112,7 +1112,9 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
>>                         return -EFAULT;
>>                 console_lock();
>>                 lock_fb_info(info);
>> -               ret = fb_set_var(info, &var);
>> +               ret = fbcon_modechange_possible(info, &var);
>
> Again, this should be done (if done at all) after the call to
> fb_ops.check_var(), as it breaks the FBIOPUT_VSCREENINFO rounding rule.
>
> What if the user just wants to display graphics, not text?

Yes, I need to go back to an older version here too and check that
the test is only run on text consoles.
That check was dropped, due feedback that you could switch
back from graphics (e.g. X11) to text console at any time....so the
check for text-only is not correct.

> Can't the text console be disabled instead?

I think the solution is to return failure if switching back to text mode isn't possible if
fonts are bigger than the screen resolution. That will be another patch.

Thanks!

Helge


>
>> +               if (!ret)
>> +                       ret = fb_set_var(info, &var);
>>                 if (!ret)
>>                         fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL);
>>                 unlock_fb_info(info);
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]
>
> 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 29, 2022, 7:03 a.m. UTC | #3

Hi Helge,

On Tue, Jun 28, 2022 at 10:52 PM Helge Deller <[e-mail]> wrote:
> On 6/28/22 10:39, Geert Uytterhoeven wrote:
> > On Sun, Jun 26, 2022 at 12:33 PM Helge Deller <[e-mail]> wrote:
> >> We need to prevent that users configure a screen size which is smaller than the
> >> currently selected font size. Otherwise rendering chars on the screen will
> >> access memory outside the graphics memory region.
> >>
> >> This patch adds a new function fbcon_modechange_possible() which
> >> implements this check and which later may be extended with other checks
> >> if necessary.  The new function is called from the FBIOPUT_VSCREENINFO
> >> ioctl handler in fbmem.c, which will return -EINVAL if userspace asked
> >> for a too small screen size.
> >>
> >> Signed-off-by: Helge Deller <[e-mail]>
> >> Reviewed-by: Daniel Vetter <[e-mail]>
> >> Cc: [e-mail] # v5.4+
> >
> > Thanks for your patch, which is now commit f0b6a66d33ca6e7e ("fbcon:
> > Prevent that screen size is smaller than font size") in fbdev/for-next

> >> --- a/drivers/video/fbdev/core/fbmem.c
> >> +++ b/drivers/video/fbdev/core/fbmem.c
> >> @@ -1112,7 +1112,9 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
> >>                         return -EFAULT;
> >>                 console_lock();
> >>                 lock_fb_info(info);
> >> -               ret = fb_set_var(info, &var);
> >> +               ret = fbcon_modechange_possible(info, &var);
> >
> > Again, this should be done (if done at all) after the call to
> > fb_ops.check_var(), as it breaks the FBIOPUT_VSCREENINFO rounding rule.
> >
> > What if the user just wants to display graphics, not text?
>
> Yes, I need to go back to an older version here too and check that
> the test is only run on text consoles.
> That check was dropped, due feedback that you could switch
> back from graphics (e.g. X11) to text console at any time....so the
> check for text-only is not correct.
>
> > Can't the text console be disabled instead?
>
> I think the solution is to return failure if switching back to text mode isn't possible if
> fonts are bigger than the screen resolution. That will be another patch.

Isn't the font a per-VC setting? Hence can't you change resolution,
switch to a different VC, and run into this?

I think the only real solution is to set the number of text columns
and/or rows to zero, and make sure that is handled correctly.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

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 29, 2022, 8:05 p.m. UTC | #4

On 6/29/22 09:03, Geert Uytterhoeven wrote:
> Hi Helge,
>
> On Tue, Jun 28, 2022 at 10:52 PM Helge Deller <[e-mail]> wrote:
>> On 6/28/22 10:39, Geert Uytterhoeven wrote:
>>> On Sun, Jun 26, 2022 at 12:33 PM Helge Deller <[e-mail]> wrote:
>>>> We need to prevent that users configure a screen size which is smaller than the
>>>> currently selected font size. Otherwise rendering chars on the screen will
>>>> access memory outside the graphics memory region.
>>>>
>>>> This patch adds a new function fbcon_modechange_possible() which
>>>> implements this check and which later may be extended with other checks
>>>> if necessary.  The new function is called from the FBIOPUT_VSCREENINFO
>>>> ioctl handler in fbmem.c, which will return -EINVAL if userspace asked
>>>> for a too small screen size.
>>>>
>>>> Signed-off-by: Helge Deller <[e-mail]>
>>>> Reviewed-by: Daniel Vetter <[e-mail]>
>>>> Cc: [e-mail] # v5.4+
>>>
>>> Thanks for your patch, which is now commit f0b6a66d33ca6e7e ("fbcon:
>>> Prevent that screen size is smaller than font size") in fbdev/for-next
>
>>>> --- a/drivers/video/fbdev/core/fbmem.c
>>>> +++ b/drivers/video/fbdev/core/fbmem.c
>>>> @@ -1112,7 +1112,9 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
>>>>                         return -EFAULT;
>>>>                 console_lock();
>>>>                 lock_fb_info(info);
>>>> -               ret = fb_set_var(info, &var);
>>>> +               ret = fbcon_modechange_possible(info, &var);
>>>
>>> Again, this should be done (if done at all) after the call to
>>> fb_ops.check_var(), as it breaks the FBIOPUT_VSCREENINFO rounding rule.
>>>
>>> What if the user just wants to display graphics, not text?
>>
>> Yes, I need to go back to an older version here too and check that
>> the test is only run on text consoles.
>> That check was dropped, due feedback that you could switch
>> back from graphics (e.g. X11) to text console at any time....so the
>> check for text-only is not correct.
>>
>>> Can't the text console be disabled instead?
>>
>> I think the solution is to return failure if switching back to text mode isn't possible if
>> fonts are bigger than the screen resolution. That will be another patch.
>
> Isn't the font a per-VC setting? Hence can't you change resolution,
> switch to a different VC, and run into this?
>
> I think the only real solution is to set the number of text columns
> and/or rows to zero, and make sure that is handled correctly.

I agree, there doesn't seem to be a simple solution.
On the other hand, such usecase seems very unlikely.
If you have a proposal for a pacth I'd welcome it.

Anyway, I've just sent out a new patch series. It does not include any patch
for this theoretical problem yet.

Helge

Patch

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index e162d5e753e5..69c7261ac334 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -2736,6 +2736,33 @@  void fbcon_update_vcs(struct fb_info *info, bool all)
 }
 EXPORT_SYMBOL(fbcon_update_vcs);

+/* let fbcon check if it supports a new screen resolution */
+int fbcon_modechange_possible(struct fb_info *info, struct fb_var_screeninfo *var)
+{
+	struct fbcon_ops *ops = info->fbcon_par;
+	struct vc_data *vc;
+	int i;
+
+	WARN_CONSOLE_UNLOCKED();
+
+	if (!ops || ops->currcon < 0)
+		return -EINVAL;
+
+	/* prevent setting a screen size which is smaller than font size */
+	for (i = first_fb_vc; i <= last_fb_vc; i++) {
+		vc = vc_cons[i].d;
+		if (!vc || registered_fb[con2fb_map[i]] != info)
+			continue;
+
+		if (vc->vc_font.width  > FBCON_SWAP(var->rotate, var->xres, var->yres) ||
+		    vc->vc_font.height > FBCON_SWAP(var->rotate, var->yres, var->xres))
+			return -EINVAL;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(fbcon_modechange_possible);
+
 int fbcon_mode_deleted(struct fb_info *info,
 		       struct fb_videomode *mode)
 {
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 5dfa4bbee642..b6e1d0f2b974 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1112,7 +1112,9 @@  static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
 			return -EFAULT;
 		console_lock();
 		lock_fb_info(info);
-		ret = fb_set_var(info, &var);
+		ret = fbcon_modechange_possible(info, &var);
+		if (!ret)
+			ret = fb_set_var(info, &var);
 		if (!ret)
 			fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL);
 		unlock_fb_info(info);
diff --git a/include/linux/fbcon.h b/include/linux/fbcon.h
index ff5596dd30f8..2382dec6d6ab 100644
--- a/include/linux/fbcon.h
+++ b/include/linux/fbcon.h
@@ -15,6 +15,8 @@  void fbcon_new_modelist(struct fb_info *info);
 void fbcon_get_requirement(struct fb_info *info,
 			   struct fb_blit_caps *caps);
 void fbcon_fb_blanked(struct fb_info *info, int blank);
+int  fbcon_modechange_possible(struct fb_info *info,
+			       struct fb_var_screeninfo *var);
 void fbcon_update_vcs(struct fb_info *info, bool all);
 void fbcon_remap_all(struct fb_info *info);
 int fbcon_set_con2fb_map_ioctl(void __user *argp);
@@ -33,6 +35,8 @@  static inline void fbcon_new_modelist(struct fb_info *info) {}
 static inline void fbcon_get_requirement(struct fb_info *info,
 					 struct fb_blit_caps *caps) {}
 static inline void fbcon_fb_blanked(struct fb_info *info, int blank) {}
+static inline int  fbcon_modechange_possible(struct fb_info *info,
+				struct fb_var_screeninfo *var) { return 0; }
 static inline void fbcon_update_vcs(struct fb_info *info, bool all) {}
 static inline void fbcon_remap_all(struct fb_info *info) {}
 static inline int fbcon_set_con2fb_map_ioctl(void __user *argp) { return 0; }