possible use-after-free in amdgpu_dm

[Andrey Grodzovsky]

On 10/17/2017 03:43 PM, Harry Wentland wrote:
> On 2017-10-17 02:15 PM, Christian König wrote:
>> Am 17.10.2017 um 19:58 schrieb Felix Kuehling:
>>> On 2017-10-17 01:25 PM, Tom St Denis wrote:
>>>> On 17/10/17 01:23 PM, Tom St Denis wrote:
>>>>> On 17/10/17 01:18 PM, Christian König wrote:
>>>>>> Am 17.10.2017 um 16:10 schrieb Tom St Denis:
>>>>>>> In this block of code:
>>>>>>>
>>>>>>> void amdgpu_dm_connector_funcs_reset(struct drm_connector *connector)
>>>>>>> {
>>>>>>>       struct dm_connector_state *state =
>>>>>>>           to_dm_connector_state(connector->state);
>>>>>>>
>>>>>>>       kfree(state);
>>>>>>>
>>>>>>>       state = kzalloc(sizeof(*state), GFP_KERNEL);
>>>>>>>
>>>>>>>
>>>>>>> The value of state is never compared with NULL and moreso the value
>>>>>>> of connector->state is never written to if NULL. Wouldn't this mean
>>>>>>> the pointer points to freed memory?
>>>> Sorry I think I might be explaining this poorly.
>>>>
>>>> In the case the alloc succeeds the pointer is updated and everything
>>>> is fine.
>>>>
>>>> IF the alloc fails the pointer (connector->state) is not updated and
>>>> the value points to freed memory.
>>> I'm wondering why the function frees, and then reallocates the memory.
>>> Does its size change? If not, why not just memset it to 0?
>> Yeah, I was just thinking the same thing.
>>
>> And no from the code snippet Tom posted it just frees and reallocated the same structure.
>>
>> I think that should rather look like this:
>>
>> if (!state)
>>       state = kzalloc(...);
>> else
>>      memset(...);
>>
> I don't expect connector->state to ever be NULL here which seems to be confirmed by
> code that would have blown up if it ever was. How about we do a BUG_ON on this condition
> as has been suggested?

Tom already sent a patch for that and i rb it.
>
> Otherwise I like the memset idea. No need for free/alloc here.

I suggest first to align this hook with the dm_plane and dm_crtc hooks.

Thanks,
Andrey

>
> Harry
>
>> Regards,
>> Christian.
>>
>>> Regards,
>>>     Felix
>>>
>>>> Tom
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx