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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1b44a3b5-5053-f121-ee62-de83d505759e@189.cn>
Date:   Fri, 14 Apr 2023 12:23:57 +0800
From:   Sui Jingfeng <15330273260@....cn>
To:     Thomas Zimmermann <tzimmermann@...e.de>,
        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,

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?

> 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