Message-ID: <2061666b-3978-d117-5acd-71d0d10b0af6@suse.de>
Date:   Mon, 17 Apr 2023 09:17:24 +0200
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Daniel Vetter <daniel@...ll.ch>
Cc:     Sui Jingfeng <15330273260@....cn>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        David Airlie <airlied@...il.com>, Li Yi <liyi@...ngson.cn>,
        Helge Deller <deller@....de>,
        Lucas De Marchi <lucas.demarchi@...el.com>,
        linux-kernel@...r.kernel.org, linux-fbdev@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, loongson-kernel@...ts.loongnix.cn
Subject: Re: [PATCH] drm/fbdev-generic: fix potential out-of-bounds access

Hi

Am 14.04.23 um 09:56 schrieb Daniel Vetter:
> On Fri, 14 Apr 2023 at 09:34, Thomas Zimmermann <tzimmermann@...e.de> 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

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)

Powered by blists - more mailing lists