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?
Cheers,
Tom
