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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 6875 bytes --]



Am 14.04.23 um 09:34 schrieb Thomas Zimmermann:
> Hi
> 
> Am 14.04.23 um 07:36 schrieb Daniel Vetter:
>> On Fri, 14 Apr 2023 at 06:24, Sui Jingfeng <15330273260@189.cn> wrote:
>>>
>>> Hi,
>>>
>>> On 2023/4/14 04:01, Daniel Vetter wrote:
>>>> On Thu, Apr 13, 2023 at 09:20:23PM +0200, Thomas Zimmermann wrote:
>>>>> Hi
>>>>>
>>>>> Am 13.04.23 um 20:56 schrieb Daniel Vetter:
>>>>> [...]
>>>>>> This should switch the existing code over to using drm_framebuffer 
>>>>>> instead
>>>>>> of fbdev:
>>>>>>
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c 
>>>>>> b/drivers/gpu/drm/drm_fb_helper.c
>>>>>> index ef4eb8b12766..99ca69dd432f 100644
>>>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>>>>> @@ -647,22 +647,26 @@ static void drm_fb_helper_damage(struct 
>>>>>> drm_fb_helper *helper, u32 x, u32 y,
>>>>>>     static void drm_fb_helper_memory_range_to_clip(struct fb_info 
>>>>>> *info, off_t off, size_t len,
>>>>>>                                             struct drm_rect *clip)
>>>>>>     {
>>>>>> +   struct drm_fb_helper *helper = info->par;
>>>>>> +
>>>>>>      off_t end = off + len;
>>>>>>      u32 x1 = 0;
>>>>>>      u32 y1 = off / info->fix.line_length;
>>>>>> -   u32 x2 = info->var.xres;
>>>>>> -   u32 y2 = DIV_ROUND_UP(end, info->fix.line_length);
>>>>>> +   u32 x2 = helper->fb->height;
>>>>>> +   unsigned stride = helper->fb->pitches[0];
>>>>>> +   u32 y2 = DIV_ROUND_UP(end, stride);
>>>>>> +   int bpp = drm_format_info_bpp(helper->fb->format, 0);
>>>>> Please DONT do that. The code here is fbdev code and shouldn't 
>>>>> bother about
>>>>> DRM data structures. Actually, it shouldn't be here: a number of fbdev
>>>>> drivers with deferred I/O contain similar code and the fbdev module 
>>>>> should
>>>>> provide us with a helper. (I think I even had some patches somewhere.)
>>>> Well my thinking is that it's a drm driver,
>>>
>>> Yes, I actually agree with this, and the code looks quite good. And I
>>> really want to adopt it.
>>>
>>> Because here is DRM, we should emulate the fbdev in the DRM's way.
>>>
>>> The DRM is really the big brother on the behind of emulated fbdev,
>>>
>>> who provide the real displayable backing memory and scant out engine to
>>> display such a buffer.
>>>
>>>
>>> But currently, the fact is,  drm_fb_helper.c still initializes lots of
>>> data structure come from fbdev world.
>>>
>>> For example, the drm_fb_helper_fill_fix() and drm_fb_helper_fill_var()
>>> in drm_fb_helper_fill_info() function.
>>>
>>> This saying implicitly that the fbdev-generic is a glue layer which copy
>>> damage frame buffer data
>>>
>>> from the front end(fbdev layer) to its drm backend.  It's not a 100%
>>> replacement its fbdev front end,
>>>
>>> rather, it relay on it.
>>>
>>>
>>>> so if we have issue with limit
>>>> checks blowing up it makes more sense to check them against drm limits.
>>>> Plus a lot more people understand those than fbdev. They should all 
>>>> match
>>>> anyway, or if they dont, we have a bug.
>>>
>>> Yes, this is really what I'm worry about.
>>>
>>> I see that  members of struct fb_var_screeninfo can be changed by
>>> user-space. For example, xoffset and yoffset.
>>>
>>> There is no change about 'info->var.xres' and 'info->var.yres' from the
>>> userspace,
>>>
>>> therefore, they should all match in practice.
>>>
>>>
>>> User-space can choose a use a smaller dispaly area than 'var.xres x
>>> var.yres',
>>>
>>> but that is implemented with 'var.xoffset' and 'var.xoffset'.
>>>
>>> But consider that the name 'var', which may stand for 'variation' or
>>> 'vary'. This is terrifying.
>>>
>>> I imagine, with a shadow buffer, the front end can do any modeset on the
>>> runtime freely,
>>>
>>> including change the format of frame buffer on the runtime.  Just do not
>>> out-of-bound.
>>>
>>> The middle do the conversion on the fly.
>>>
>>>
>>> We might also create double shadow buffer size to allow the front end to
>>> pan?
>>
>> Yeah so the front should be able to pan. And the front-end can
>> actually make xres/yres smalle than drm_fb->height/width, so we _have_
>> to use the fb side of things. Otherwise it's a bug I just realized.
> 
> What are you talking about?  The GEM buffer is a full fbdev framebuffer, 
> which is screen resolution multiplied by the overall factor.  The shadow 

s/overall/overalloc'

> buffer is exactly the same size. You can already easily pan within these 
> buffers.
> 
> 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.
> 
> 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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]