Re: [PATCH] drm/fbdev-generic: fix potential out-of-bounds access

From: Thomas Zimmermann
Date: Mon Apr 17 2023 - 03:17:36 EST

Hi

Am 14.04.23 um 09:56 schrieb Daniel Vetter:
On Fri, 14 Apr 2023 at 09:34, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
[...]

There's also no need/way to change video modes or formats in the shadow
buffer. If we'd ever support that, it would be implemented in the DRM
driver's modesetting. The relationship between GEM buffer and shadow
buffer remains unaffected by this.

Try it and be amazed :-)

It's not supported. I don't know if we catch all the cases, but at least we try. And I don't think we will ever support changes to the video mode. The framebuffer width/height binds us to certain constrains during the damage handling. Figuring all this out is probably not worth the effort.

I've seen enough syzkaller bugs and screamed
at them that yes we do this. Also xres/yres is the wrong thing even if
you don't use fb ioctl to change things up in multi-monitor cases (we
allocate the drm_fb/fbdev virtual size to match the biggest
resolution, but then set fbinfo->var.x/yres to match the smallest to
make sure fbcon is fully visible everywhere).

I think you're confusion is the perfect case for why we really should
use fb->height/width/pitches[0] here.

I really don't see the point of building a DRM-only variant when there's the same code in fbdev drivers. Required information is all stored in the fb_info. The helper code should be seen as part of fbdev's deferred I/O.

This, however, is independent from the limitation where the memory size has to be a multiple of the framebuffer resolution. That's a limitation imposed by DRM. Please also note that this is only relevant for fbdev-generic. I intent to move some of those damage helpers there. I'd assume that will make the whole thing a bit more understandable. (Unfortunately, the fbdev emulation has been a victim of false starts and complexity. It takes time to fix all this.)

I'm not sure why you refer to xres/yres; I think, the smem_length and line_length is what we'd need in most cases.

Best regards
Thomas

-Daniel


Best regards
Thomas


The xres_virtual/yres_virtual should always match drm_fb sizes (but
we've had bugs in the past for that, only recently fixed all in
linux-next), because that's supposed to be the max size. And since we
never reallocate the fbdev emulation fb (at least with the current
code) this should never change.

But fundamentally you're bringing up a very good point, we've had
piles of bugs in the past with not properly validating the fbdev side
information in info->var, and a bunch of resulting bugs. So validating
against the drm side of things should be a bit more robust.

It's kinda the same we do for legacy kms ioctls: We translate that to
atomic kms as fast as possible, and then do the entire subsequent
validation with atomic kms data structures.
-Daniel

The thing is, if you change this
further to just pass the drm_framebuffer, then this 100% becomes a drm
function, which could be used by anything in drm really.

I agree with you.

If I use fb_width/fb_height directly and bypassing 'info->var.xres" and
"info->var.yres",

the code style diverged then. As far as I am understanding, the clip
happen on the front end,

the actual damage update happen at back end.

Using the data structure come with the front end is more reasonable for
current implement.

But also *shrug*.

I can convert this single function to 100% drm with another patch.

But, maybe there also have other functions are not 100% drm

I would like do something to help achieve this in the future,

let me help to fix this bug first?

-Daniel




--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev




--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature