ipc3: helper: clear component pipeline pointers before freeing pipeline#10852
ipc3: helper: clear component pipeline pointers before freeing pipeline#10852tmleman wants to merge 1 commit into
Conversation
tmleman
requested review from
dbaluta,
kv2019i,
lbetlej,
lgirdwood,
mmaka1 and
plbossart
as code owners
There was a problem hiding this comment.
Pull request overview
Fixes a potential use-after-free in IPC3 pipeline teardown by ensuring components no longer hold dangling cd->pipeline pointers after ipc_pipeline_free() frees the pipeline object.
Changes:
- Iterate
ipc->comp_listduringipc_pipeline_free()and clearcd->pipelinefor components still referencing the pipeline being freed.
tmleman
force-pushed
the
topic/upstream/pr/fix/ipc3/clear_component_pointers
branch
from
e14ed5b to
9e1dd00
Compare
When ipc_pipeline_free() frees a pipeline, component devices that were connected to it retain stale cd->pipeline pointers. If an IPC (e.g. stream position request) later dereferences that pointer, it triggers a use-after-free. Fix this by iterating all components in the IPC comp_list and setting cd->pipeline = NULL for any component whose pipeline matches the one being freed. This makes the existing NULL checks in handler.c effective and prevents the dangling pointer dereference. Found by fuzzing with AddressSanitizer enabled. Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
| * that still belong to this pipeline. A well-behaved host driver | ||
| * frees components before freeing the pipeline, but if it does not | ||
| * (or in fuzz/error paths) the dangling pointer would be a | ||
| * use-after-free on any subsequent IPC referencing that component. |
There was a problem hiding this comment.
The description implies that the entire sequence is invalid and the pointers to the already freed pipeline are only indications of this. If this is really a non-critical bug and you are 100% certain that there are no other side effects, I would at least issue a warning; Else, return an error.
There was a problem hiding this comment.
Where? In the place where I set pointers to NULL that's normal flow. Do you mean that instead of setting the pointer to NULL I should insert a warning that there's a component in memory that was part of a pipeline that was deleted and now holds a pointer to freed memory?
There was a problem hiding this comment.
If this is an error then I think you could indeed skip NULL setting. In case of warning (or non-critical error), you would need to keep this code and just add a log.
But if this is the normal flow, then maybe I misinterpreted the phrase "well-behaved host driver" ;) I thought "well-behaved host driver" equals "driver without bugs"... but it also can be interpreted as "minimalistic driver" that doesn't perform optional steps.
There was a problem hiding this comment.
I don't think I'll do it this way, it seems to me this is the best approach. The driver removes the pipeline and then the module, and this flow is fine. If the driver sends an IPC to the module (in the case that triggered a crash in the fuzzer, this is a stream position request) whose pipeline has already been removed, it will dereference a pointer to memory that has been freed. After this change, the driver will get an error because the module will no longer hold a pointer to the pipeline. I assume it's too late for IPC3 to introduce restrictions on the IPC sending order (remove components first, then the pipeline).
It seems logical to me: you trigger an action on a component that is no longer part of the pipeline and you get an error in return.
There was a problem hiding this comment.
If the sequence is legal, then the comment is confusing (at least to me):
- The phrase "well-behaved" is vague because the opposite might be interpreted as erroneous (misbehaved). I would use something like "conventional" instead.
- "any subsequent IPC referencing that component" -> "any subsequent IPC referencing that pipeline" OR "any subsequent IPC referencing that component except COMP_FREE". Because COMP_FREE IPC would still be legal according to your clarification, right?
When ipc_pipeline_free() frees a pipeline, component devices that were connected to it retain stale cd->pipeline pointers. If an IPC (e.g. stream position request) later dereferences that pointer, it triggers a use-after-free.
Fix this by iterating all components in the IPC comp_list and setting cd->pipeline = NULL for any component whose pipeline matches the one being freed. This makes the existing NULL checks in handler.c effective and prevents the dangling pointer dereference.
Found by fuzzing with AddressSanitizer enabled.