[PATCH v1] drm/amdgpu: fix framebuffer memory use after free

[Łukasz Bartosik]

czw., 17 cze 2021 o 16:18 Michel Dänzer <michel at daenzer.net> napisał(a):
>
> On 2021-06-17 10:18 a.m., Lukasz Bartosik wrote:
> > With option CONFIG_DEBUG_LIST enabled the kernel logs show list_add
> > corruption warning. The warning originates from drm_framebuffer_init()
> > function which adds framebuffer to a framebuffers list and is called by
> > amdgpu_display_gem_fb_verify_and_init().
> > If amdgpu_display_gem_fb_verify_and_init() encounters an error after
> > calling drm_framebuffer_init() then framebuffer memory is released
> > in amdgpu_display_user_framebuffer_create() without removing framebuffer
> > from the list where it was added. Reuse of that memory by any other
> > party cause corruption of the framebuffers linked list. This fix removes
> > framebuffer from the linked list and unregisters it in case of failure.
> >
> > [...]
> >
> > Fixes: 6eed95b00b45 ("drm/amd/display: Store tiling_flags in the framebuffer.")
>
> I didn't realize there was already an issue before f258907fdd835e "drm/amdgpu: Verify bo size can fit framebuffer size on init.". Looking at
> the Git history again, I agree there's already at least a theoretical issue in 5.11, though I suspect it's harder to hit in practice.
>
>
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > index c13985fb35be..933190281b91 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > @@ -1085,14 +1085,17 @@ int amdgpu_display_gem_fb_verify_and_init(
> >                           mode_cmd->modifier[0]);
> >
> >               ret = -EINVAL;
> > -             goto err;
> > +             goto err_fb_cleanup;
> >       }
> >
> >       ret = amdgpu_display_framebuffer_init(dev, rfb, mode_cmd, obj);
> >       if (ret)
> > -             goto err;
> > +             goto err_fb_cleanup;
> >
> >       return 0;
> > +err_fb_cleanup:
> > +     drm_framebuffer_unregister_private(&rfb->base);
> > +     drm_framebuffer_cleanup(&rfb->base);
> >  err:
> >       drm_dbg_kms(dev, "Failed to verify and init gem fb: %d\n", ret);
> >       rfb->base.obj[0] = NULL;
> >
>
> There's a similar issue in amdgpu_display_gem_fb_init. https://patchwork.freedesktop.org/patch/439542/ fixes that as well, and seems simpler (though I'm biased obviously :).

I agree your patch is simpler and covers more cases, but IMHO my
approach with explicit framebuffer cleanup has the advantage
that it will be hard to miss in case of future code reorganizations in
that area.

>
>
> Neither patch can be trivially cherry picked for fixing the issue in 5.11/5.12 due to f258907fdd835e.
>
>
> --
> Earthling Michel Dänzer               |               https://redhat.com
> Libre software enthusiast             |             Mesa and X developer