possible use-after-free in amdgpu_dm

[Andrey Grodzovsky]

On 10/17/2017 01:36 PM, Tom St Denis wrote:
> On 17/10/17 01:34 PM, Andrey Grodzovsky wrote:
>>
>>
>> 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.
>
>
> I'm not sure if that's a WARN_ON or simply a BUG_ON since I don't get 
> how the dm layer continues at this point.
>
> Alternatively, we could put a WARN_ON and set the pointer to NULL and 
> let it handle it higher in the call stack.
>
> Would you like me to draft a commit for the latter?

IMHO It's more of a BUG_ON situation, this failure is not recoverable 
since the framework assumes a connector has a fresh state to start 
working on.

Andrey

>
> Cheers,
> Tom