Skip to content

ipc3: helper: clear component pipeline pointers before freeing pipeline #10852

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tmleman wants to merge 1 commit into
base: main
Choose a base branch
from
+15 −0
Open

ipc3: helper: clear component pipeline pointers before freeing pipeline #10852

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/ipc/ipc3/helper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,8 @@ int ipc_pipeline_new(struct ipc *ipc, ipc_pipe_new *_pipe_desc)
int ipc_pipeline_free(struct ipc *ipc, uint32_t comp_id)
{
struct ipc_comp_dev *ipc_pipe;
struct ipc_comp_dev *icd;
struct list_item *clist;
int ret;

Comment thread
tmleman marked this conversation as resolved.
/* check whether pipeline exists */
Expand All @@ -457,6 +459,19 @@ int ipc_pipeline_free(struct ipc *ipc, uint32_t comp_id)
if (!cpu_is_me(ipc_pipe->core))
return ipc_process_on_core(ipc_pipe->core, false);

/* Clear stale cd->pipeline pointers on all components and buffers
* 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.

@wjablon1 wjablon1 Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@tmleman tmleman Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@wjablon1 wjablon1 Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@tmleman tmleman Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@wjablon1 wjablon1 Jun 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the sequence is legal, then the comment is confusing (at least to me):

  1. The phrase "well-behaved" is vague because the opposite might be interpreted as erroneous (misbehaved). I would use something like "conventional" instead.
  2. "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?

*/
list_for_item(clist, &ipc->comp_list) {
icd = container_of(clist, struct ipc_comp_dev, list);
if (icd->type == COMP_TYPE_COMPONENT &&
icd->cd && icd->cd->pipeline == ipc_pipe->pipeline)
icd->cd->pipeline = NULL;
}

/* free buffer and remove from list */
ret = pipeline_free(ipc_pipe->pipeline);
if (ret < 0) {
Expand Down
Loading