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.
Tom
