Skip to content

Commit efbc17b

Browse files
tmlemanabonislawski
authored andcommitted
pipeline: reject double-connect of already-attached buffer
pipeline_connect() had no guard against being called twice for the same buffer-component pair. Calling list_item_prepend() on a node that is already in a doubly-linked list corrupts the list by creating a self-loop where node->next points back to itself instead of to the list head. The corruption was discovered through IPC3 fuzzing in persistent mode. Without per-testcase topology teardown, components and buffers created by testcase N survive into testcase N+1. When N+1 sends a TPLG_COMP_CONNECT for IDs that N already connected, ipc_comp_connect() finds the surviving objects and calls pipeline_connect() a second time. The same sequence can also be triggered within a single testcase by two CONNECT messages for the same pair. The self-loop causes ipc_comp_free() to hang indefinitely. The function walks bsource_list / bsink_list with comp_dev_for_each_producer_safe() whose termination condition checks node->next == &comp->bsource_list. With the self-loop that check is always false. The walk runs inside irq_local_disable(), so the native_sim timer cannot preempt the thread and nsi_exec_for() never returns, making libFuzzer's max_total_time limit unreachable. A second failure mode arises when ipc_buffer_free() calls pipeline_disconnect() on the corrupted buffer. list_item_del() updates comp->bsource_list.next to node->next, which due to the self-loop is the node itself — leaving the component's list head pointing into the freed buffer memory. When that memory is reused by a later allocation and overwritten, the next walk of bsource_list dereferences an invalid pointer, crashing with a null dereference or a corrupt-pointer access. Move the list-consistency check down into buffer_attach() itself, where it belongs alongside the list_item_prepend() call it protects. The function is changed to return int: it inspects the buffer's list node for the given direction via buffer_comp_list() and returns -EALREADY when list_is_empty() reports the node is already linked (node->next != node), otherwise prepends and returns 0. pipeline_connect() propagates that error and emits the component-context log message; the existing single caller is the only one to update. Verified with -s address on the full IPC3 corpus (~95K runs, 41 s): zero crashes, zero hangs, ~2300 exec/s. The unfixed build stalled indefinitely on the same run. Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
1 parent 2b8fc70 commit efbc17b

3 files changed

Lines changed: 21 additions & 3 deletions

File tree

src/audio/buffers/comp_buffer.c

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -604,12 +604,20 @@ static inline struct list_item *buffer_comp_list(struct comp_buffer *buffer,
604604
* from racing attach / detach calls, but the scheduler can interrupt the IPC
605605
* thread and begin using the buffer for streaming. FIXME: this is still a
606606
* problem with different cores.
607+
*
608+
* Returns -EALREADY if the buffer is already attached in this direction.
609+
* Attaching the same buffer twice would corrupt the list (list_item_prepend
610+
* on an already-linked node creates a self-loop), so callers must propagate
611+
* the error.
607612
*/
608-
void buffer_attach(struct comp_buffer *buffer, struct list_item *head, int dir)
613+
int buffer_attach(struct comp_buffer *buffer, struct list_item *head, int dir)
609614
{
610615
struct list_item *list = buffer_comp_list(buffer, dir);
611616
CORE_CHECK_STRUCT(&buffer->audio_buffer);
617+
if (!list_is_empty(list))
618+
return -EALREADY;
612619
list_item_prepend(list, head);
620+
return 0;
613621
}
614622

615623
/*

src/audio/pipeline/pipeline-graph.c

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,7 @@ int pipeline_connect(struct comp_dev *comp, struct comp_buffer *buffer,
198198
int dir)
199199
{
200200
struct list_item *comp_list;
201+
int ret;
201202
PPL_LOCK_DECLARE;
202203

203204
if (dir == PPL_CONN_DIR_COMP_TO_BUFFER)
@@ -208,7 +209,13 @@ int pipeline_connect(struct comp_dev *comp, struct comp_buffer *buffer,
208209
PPL_LOCK();
209210

210211
comp_list = comp_buffer_list(comp, dir);
211-
buffer_attach(buffer, comp_list, dir);
212+
ret = buffer_attach(buffer, comp_list, dir);
213+
if (ret < 0) {
214+
comp_err(comp, "buffer %d already connected dir %d",
215+
buf_get_id(buffer), dir);
216+
PPL_UNLOCK();
217+
return ret;
218+
}
212219
buffer_set_comp(buffer, comp, dir);
213220

214221
PPL_UNLOCK();

src/include/sof/audio/buffer.h

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -289,8 +289,11 @@ static inline void buffer_stream_writeback(struct comp_buffer *buffer, uint32_t
289289
* really be the head of the list, not a list head within another buffer. We
290290
* don't synchronise its cache, so it must not be embedded in an object, using
291291
* the coherent API. The caller takes care to protect list heads.
292+
*
293+
* Returns -EINVAL if the buffer is already linked in this direction
294+
* (re-attaching would create a self-loop and corrupt the list).
292295
*/
293-
void buffer_attach(struct comp_buffer *buffer, struct list_item *head, int dir);
296+
int buffer_attach(struct comp_buffer *buffer, struct list_item *head, int dir);
294297

295298
/*
296299
* Detach a buffer from anywhere in the list. "head" is again the head of the

0 commit comments

Comments
 (0)