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
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
