| Message ID | 20220701202352.559808-4-deller@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fbcon: Fixes for screen resolution changes - round 2 | expand |
Hi Helge, On Fri, Jul 1, 2022 at 10:23 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. > > Give a kernel WARNing and show the driver name to help locating the buggy > driver. > > 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 > @@ -1016,6 +1016,18 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) > if (ret) > return ret; > > + /* make sure virtual resolution >= physical resolution */ > + if (WARN_ON(var->xres_virtual < var->xres)) { WARN_ON_ONCE()? This does mean we would miss two or more buggy drivers in a single system. > + pr_warn("fbcon: Fix up invalid xres %d for %s\n", xres_virtual? > + var->xres_virtual, info->fix.id); > + var->xres_virtual = var->xres; I think it's better to not fix this up, but return -EINVAL instead. After all if we get here, we have a buggy driver that needs to be fixed. > + } > + if (WARN_ON(var->yres_virtual < var->yres)) { > + pr_warn("fbcon: Fix up invalid yres %d for %s\n", > + var->yres_virtual, info->fix.id); > + var->yres_virtual = var->yres; > + } Same for yres. > + > if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW) > return 0; > 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 <geert@linux-m68k.org>: > > --- a/drivers/video/fbdev/core/fbmem.c > > +++ b/drivers/video/fbdev/core/fbmem.c > > @@ -1016,6 +1016,18 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) > > if (ret) > > return ret; > > > > + /* make sure virtual resolution >= physical resolution */ > > + if (WARN_ON(var->xres_virtual < var->xres)) { > > WARN_ON_ONCE()? > This does mean we would miss two or more buggy drivers in a single system. > > > + pr_warn("fbcon: Fix up invalid xres %d for %s\n", > > xres_virtual? > > > + var->xres_virtual, info->fix.id); > > + var->xres_virtual = var->xres; > > I think it's better to not fix this up, but return -EINVAL instead. > After all if we get here, we have a buggy driver that needs to be fixed. > > > + } > > + if (WARN_ON(var->yres_virtual < var->yres)) { > > + pr_warn("fbcon: Fix up invalid yres %d for %s\n", > > + var->yres_virtual, info->fix.id); > > + var->yres_virtual = var->yres; > > + } > > Same for yres. Geert, thanks for your valuable feedback! In general I don't like for this case any of the WARN_ON* functions. They don't provide any useful info for us, dumps unneccessarily the stack backtrace and would annoy existing users. We know, that DRM drivers can't change the resolution. If we would leave in any kind of warning, all DRM users will ask back - and we don't have a solution for them. It's also no regression, because it didn't worked before either. But we want to detect fbdev drivers which behave badly - so we should print that info with the driver name. Below is a new patch which addresses this. The search for drm drivers looks somewhat hackish - maybe someone can propose a better approach? Thoughts? Helge From 0f33e2a3730342ab85df372f80b4f61762fb1130 Mon Sep 17 00:00:00 2001 From: Helge Deller <deller@gmx.de> Date: Wed, 29 Jun 2022 15:53:55 +0200 Subject: [PATCH] fbmem: Check virtual screen sizes in fb_set_var() Verify that the fbdev or drm driver correctly adjusted the virtual screen sizes. On failure report (in case of fbdev drivers) the failing driver. Signed-off-by: Helge Deller <deller@gmx.de> Cc: stable@vger.kernel.org # v5.4+ diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 160389365a36..9b75aa4405ee 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1016,6 +1016,21 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) if (ret) return ret; + /* verify that virtual resolution >= physical resolution */ + if (var->xres_virtual < var->xres || + var->yres_virtual < var->yres) { + /* drm drivers don't support mode changes yet. Do not report. */ + if (strstr(info->fix.id, "drm")) + return -ENOTSUPP; + + pr_warn("WARNING: fbcon: Driver %s missed to adjust virtual" + " screen size (%dx%d vs. %dx%d)\n", + info->fix.id, + var->xres_virtual, var->yres_virtual, + var->xres, var->yres); + return -EINVAL; + } + if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW) return 0;
Hi Helge, On Sun, Jul 3, 2022 at 3:54 PM Helge Deller <deller@gmx.de> wrote: > * Geert Uytterhoeven <geert@linux-m68k.org>: > > > --- a/drivers/video/fbdev/core/fbmem.c > > > +++ b/drivers/video/fbdev/core/fbmem.c > > > @@ -1016,6 +1016,18 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) > > > if (ret) > > > return ret; > > > > > > + /* make sure virtual resolution >= physical resolution */ > > > + if (WARN_ON(var->xres_virtual < var->xres)) { > > > > WARN_ON_ONCE()? > > This does mean we would miss two or more buggy drivers in a single system. > > > > > + pr_warn("fbcon: Fix up invalid xres %d for %s\n", > > > > xres_virtual? > > > > > + var->xres_virtual, info->fix.id); > > > + var->xres_virtual = var->xres; > > > > I think it's better to not fix this up, but return -EINVAL instead. > > After all if we get here, we have a buggy driver that needs to be fixed. > > > > > + } > > > + if (WARN_ON(var->yres_virtual < var->yres)) { > > > + pr_warn("fbcon: Fix up invalid yres %d for %s\n", > > > + var->yres_virtual, info->fix.id); > > > + var->yres_virtual = var->yres; > > > + } > > > > Same for yres. > > Geert, thanks for your valuable feedback! > > In general I don't like for this case any of the WARN_ON* functions. > They don't provide any useful info for us, dumps unneccessarily the > stack backtrace and would annoy existing users. Without the stack trace, most people won't notice... > We know, that DRM drivers can't change the resolution. If we would leave > in any kind of warning, all DRM users will ask back - and we don't have > a solution for them. It's also no regression, because it didn't worked > before either. The warning will only be triggered when the requested virtual resolution is smaller than the requested physical resolution. As long as the requested virtual and physical resolution match what was returned by FBIOGET_VSCREENINFO before, the warning won't be triggered. So in normal use cases, the user won't be impacted. Fuzzers will see the warning, but the kernel will no longer crash on invalid values. > But we want to detect fbdev drivers which behave badly - so we should > print that info with the driver name. > > Below is a new patch which addresses this. The search for drm drivers > looks somewhat hackish - maybe someone can propose a better approach? > > Thoughts? > > Helge > > > From 0f33e2a3730342ab85df372f80b4f61762fb1130 Mon Sep 17 00:00:00 2001 > From: Helge Deller <deller@gmx.de> > Date: Wed, 29 Jun 2022 15:53:55 +0200 > Subject: [PATCH] fbmem: Check virtual screen sizes in fb_set_var() > > Verify that the fbdev or drm driver correctly adjusted the virtual screen > sizes. On failure report (in case of fbdev drivers) the failing driver. > > Signed-off-by: Helge Deller <deller@gmx.de> > Cc: stable@vger.kernel.org # v5.4+ > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > index 160389365a36..9b75aa4405ee 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -1016,6 +1016,21 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) > if (ret) > return ret; > > + /* verify that virtual resolution >= physical resolution */ > + if (var->xres_virtual < var->xres || > + var->yres_virtual < var->yres) { > + /* drm drivers don't support mode changes yet. Do not report. */ > + if (strstr(info->fix.id, "drm")) > + return -ENOTSUPP; > + > + pr_warn("WARNING: fbcon: Driver %s missed to adjust virtual" > + " screen size (%dx%d vs. %dx%d)\n", > + info->fix.id, > + var->xres_virtual, var->yres_virtual, > + var->xres, var->yres); > + return -EINVAL; > + } > + > if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW) > return 0; Hence I think there is no need for ignoring drm. 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
Hi Daniel & DRM developers, could you please comment on the change below? On 7/3/22 16:41, Geert Uytterhoeven wrote: > Hi Helge, > > On Sun, Jul 3, 2022 at 3:54 PM Helge Deller <deller@gmx.de> wrote: >> * Geert Uytterhoeven <geert@linux-m68k.org>: >>>> --- a/drivers/video/fbdev/core/fbmem.c >>>> +++ b/drivers/video/fbdev/core/fbmem.c >>>> @@ -1016,6 +1016,18 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) >>>> if (ret) >>>> return ret; >>>> >>>> + /* make sure virtual resolution >= physical resolution */ >>>> + if (WARN_ON(var->xres_virtual < var->xres)) { >>> >>> WARN_ON_ONCE()? >>> This does mean we would miss two or more buggy drivers in a single system. >>> >>>> + pr_warn("fbcon: Fix up invalid xres %d for %s\n", >>> >>> xres_virtual? >>> >>>> + var->xres_virtual, info->fix.id); >>>> + var->xres_virtual = var->xres; >>> >>> I think it's better to not fix this up, but return -EINVAL instead. >>> After all if we get here, we have a buggy driver that needs to be fixed. >>> >>>> + } >>>> + if (WARN_ON(var->yres_virtual < var->yres)) { >>>> + pr_warn("fbcon: Fix up invalid yres %d for %s\n", >>>> + var->yres_virtual, info->fix.id); >>>> + var->yres_virtual = var->yres; >>>> + } >>> >>> Same for yres. >> >> Geert, thanks for your valuable feedback! >> >> In general I don't like for this case any of the WARN_ON* functions. >> They don't provide any useful info for us, dumps unneccessarily the >> stack backtrace and would annoy existing users. > > Without the stack trace, most people won't notice... > >> We know, that DRM drivers can't change the resolution. If we would leave >> in any kind of warning, all DRM users will ask back - and we don't have >> a solution for them. It's also no regression, because it didn't worked >> before either. > > The warning will only be triggered when the requested virtual > resolution is smaller than the requested physical resolution. As > long as the requested virtual and physical resolution match what > was returned by FBIOGET_VSCREENINFO before, the warning won't > be triggered. So in normal use cases, the user won't be impacted. > Fuzzers will see the warning, but the kernel will no longer crash > on invalid values. Still, fuzzers will crash if they have panic_on_warn enabled, which was the case for the reproducer I got. >> But we want to detect fbdev drivers which behave badly - so we should >> print that info with the driver name. >> >> Below is a new patch which addresses this. The search for drm drivers >> looks somewhat hackish - maybe someone can propose a better approach? >> >> Thoughts? >> >> Helge >> >> >> From 0f33e2a3730342ab85df372f80b4f61762fb1130 Mon Sep 17 00:00:00 2001 >> From: Helge Deller <deller@gmx.de> >> Date: Wed, 29 Jun 2022 15:53:55 +0200 >> Subject: [PATCH] fbmem: Check virtual screen sizes in fb_set_var() >> >> Verify that the fbdev or drm driver correctly adjusted the virtual screen >> sizes. On failure report (in case of fbdev drivers) the failing driver. >> >> Signed-off-by: Helge Deller <deller@gmx.de> >> Cc: stable@vger.kernel.org # v5.4+ >> >> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c >> index 160389365a36..9b75aa4405ee 100644 >> --- a/drivers/video/fbdev/core/fbmem.c >> +++ b/drivers/video/fbdev/core/fbmem.c >> @@ -1016,6 +1016,21 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) >> if (ret) >> return ret; >> >> + /* verify that virtual resolution >= physical resolution */ >> + if (var->xres_virtual < var->xres || >> + var->yres_virtual < var->yres) { This is the part I'd like to get feedback from DRM on: Shall we leave that in, or drop it as Geert suggested? >> + /* drm drivers don't support mode changes yet. Do not report. */ >> + if (strstr(info->fix.id, "drm")) >> + return -ENOTSUPP; >> + >> + pr_warn("WARNING: fbcon: Driver %s missed to adjust virtual" >> + " screen size (%dx%d vs. %dx%d)\n", >> + info->fix.id, >> + var->xres_virtual, var->yres_virtual, >> + var->xres, var->yres); >> + return -EINVAL; >> + } >> + >> if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW) >> return 0; > > Hence I think there is no need for ignoring drm.
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 160389365a36..e8f06d26803c 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1016,6 +1016,18 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) if (ret) return ret; + /* make sure virtual resolution >= physical resolution */ + if (WARN_ON(var->xres_virtual < var->xres)) { + pr_warn("fbcon: Fix up invalid xres %d for %s\n", + var->xres_virtual, info->fix.id); + var->xres_virtual = var->xres; + } + if (WARN_ON(var->yres_virtual < var->yres)) { + pr_warn("fbcon: Fix up invalid yres %d for %s\n", + var->yres_virtual, info->fix.id); + var->yres_virtual = var->yres; + } + if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW) return 0;
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. Give a kernel WARNing and show the driver name to help locating the buggy driver. Signed-off-by: Helge Deller <deller@gmx.de> Cc: stable@vger.kernel.org # v5.4+ --- drivers/video/fbdev/core/fbmem.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) -- 2.35.3