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

[Daniel Vetter]

On Fri, 14 Apr 2023 at 09:34, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
>
> Hi
>
> Am 14.04.23 um 07:36 schrieb Daniel Vetter:
> > On Fri, 14 Apr 2023 at 06:24, Sui Jingfeng <15330273260@xxxxxx> 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
> 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.

Try it and be amazed :-) 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.
-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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch