[PATCH 3/3] drm/fb-helper: fix input validation gaps in check_var

[Daniel Vetter]

On Wed, Apr 05, 2023 at 06:27:17PM +0200, Javier Martinez Canillas wrote:
> Daniel Vetter <daniel at ffwll.ch> writes:
> 
> [...]
> 
> >> 
> >> but only the 'var->xres > fb->width || var->yres > fb->height' from the
> >> conditions checked could be false after your __fill_var() call above.
> >> 
> >> You should drop the 'var->bits_per_pixel > bpp', 'var->xres_virtual >
> >> fb->width' and 'var->yres_virtual > fb->height' checks I believe since
> >> those will always be true.
> >
> > The __fill_var is after this. I'm honestly not sure what the exact
> 
> Ah, your patch adds it after that indeed. Please ignore my comment then.

So rb: you?

> > semantics are supposed to be, but essentially if userspace asks for too
> > big virtual size, we reject it. And for anything else we then tell it
> > (with __fill_var) how big the actually available space is.
> >
> > What I'm wondering now is whether too small x/yres won't lead to problems
> > of some sorts ... For multi-screen we set the virtual size to be big
> > enough for all crtc, and then just set x/yres to be the smallest output.
> > That way fbcon knows to only draw as much as is visible on all screens.
> > But if you then pan that too much, the bigger screens might not have a big
> > enough buffer anymore and things fail (but shouldn't).
> >
> > Not sure how to fix that tbh.
> 
> Would this be a problem in practice?

I'm frankly not sure. You'd get a black screen for fbcon/fbdev across all
outputs, but only if you have userspace doing this intentionally.

In a way it's just another artifact of the drm fbdev emulation not using
ATOMIC_TEST_ONLY in the various places where it should, and so doesn't
really know whether a configuration change will work out.

We already have this in obscure mulit-monitor cases where adding another
screen kills fbcon, because the display hw is running out of fifo or
clocks or whatever, and because the drm fbdev code doesn't check but just
blindly commits the entire thing as an atomic commit, the overall commit
fails.

This worked "better" with legacy kms because there we commit per-crtc, so
if any specific crtc runs into a limit check, only that one fails to light
up.

Imo given that no one cared enough yet to write up atomic TEST_ONLY
support for fbdev emulation I think we can continue to just ignore this
problem.

What should not happen is that fbcon code blows up drawing out of bounds
or something like that, resulting in a kernel crash. So from that pov I
think it's "safe" :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch