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

[Michel Dänzer]

On 2021-06-18 10:46 a.m., Łukasz Bartosik wrote:
> 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.

Fair enough.

FWIW, I went the "call drm_framebuffer_init last" route because it matches what all other drivers do AFAICT.


-- 
Earthling Michel Dänzer               |               https://redhat.com
Libre software enthusiast             |             Mesa and X developer