possible use-after-free in amdgpu_dm

[Andrey Grodzovsky]

On 10/17/2017 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?
>>>
>>> Why should we compare the value of state to NULL? What's done here 
>>> is just to get the size of the type state points to.
>>>
>>> Not sure if that is really covered by the C standard, but in 
>>> practice it works fine even when state is NULL.
>>
>> Hi Christian,
>>
>>
>> Oh sorry no the issue isn't with the sizeof (that's perfectly fine 
>> since the standard says the pointer won't be dereferenced).
>>
>> The issue is later on in the function there's this statement:
>>
>>          connector->state = &state->base;
>>
>> Where "base" is first item in the struct so it's effectively just 
>> "connector->state = &state".
>>
>> This means that the value of "connector->state" is stale since the 
>> pointer was kfree'ed right (if the alloc fails) which could lead to a 
>> use-after-free error (I don't know where this function lies in the 
>> rest of the code paths but it seems wrong either way).
>
>
> 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.

Indeed an issue, there should be a big WARN_ON or error here for such a 
case. In general this hook is called from drm_mode_config_reset which is 
used to reset SW and HW states on loading or resetting due to GPU reset 
or resume from suspend. I think we only use it on load.

Andrey

>
> Tom
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx