From 3e3d0cdeab7aded526fea237290f38e471dfd30d Mon Sep 17 00:00:00 2001 From: Guennadi Liakhovetski Date: Thu, 15 Jun 2023 15:40:43 +0200 Subject: [PATCH] buffer: prevent cache corruption Recent research discovered a set of potential issues related with cache prefetch. Specifically it seems like uncached access to memory can cause cache prefetch. This can cause problems in buffer_attach() and buffer_detach() where buffers are added to or removed from lists respectively via uncached addresses, after which they can be used via cached addresses. Add proper cache synchronisation and interrupt locking to protect against such memory corruption. Signed-off-by: Guennadi Liakhovetski --- src/audio/buffer.c | 17 ++++++++++++++++- src/audio/module_adapter/module_adapter.c | 11 +++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/audio/buffer.c b/src/audio/buffer.c index c468b93ca88c..d39710a7273c 100644 --- a/src/audio/buffer.c +++ b/src/audio/buffer.c @@ -286,8 +286,15 @@ void comp_update_buffer_consume(struct comp_buffer __sparse_cache *buffer, uint3 (char *)audio_stream_get_addr(&buffer->stream))); } +/* + * Locking: must be called with interrupts disabled! Serialized IPCs protect us + * from racing attach / detach calls, but the scheduler can interrupt the IPC + * thread and begin using the buffer for streaming. FIXME: this is still a + * problem with different cores. + */ void buffer_attach(struct comp_buffer *buffer, struct list_item *head, int dir) { + struct list_item *list = buffer_comp_list(buffer, dir); struct list_item __sparse_cache *needs_sync; bool further_buffers_exist; @@ -302,11 +309,17 @@ void buffer_attach(struct comp_buffer *buffer, struct list_item *head, int dir) if (further_buffers_exist) dcache_writeback_region(needs_sync, sizeof(struct list_item)); /* The cache line can be prefetched here, invalidate it after prepending */ - list_item_prepend(buffer_comp_list(buffer, dir), head); + list_item_prepend(list, head); if (further_buffers_exist) dcache_invalidate_region(needs_sync, sizeof(struct list_item)); + /* no dirty cache lines exist for this buffer yet, no need to write back */ + dcache_invalidate_region(uncache_to_cache(list), sizeof(*list)); } +/* + * Locking: must be called with interrupts disabled! See buffer_attach() above + * for details + */ void buffer_detach(struct comp_buffer *buffer, struct list_item *head, int dir) { struct list_item __sparse_cache *needs_sync_prev, *needs_sync_next; @@ -329,8 +342,10 @@ void buffer_detach(struct comp_buffer *buffer, struct list_item *head, int dir) dcache_writeback_region(needs_sync_next, sizeof(struct list_item)); if (buffers_before_exist) dcache_writeback_region(needs_sync_prev, sizeof(struct list_item)); + dcache_writeback_region(uncache_to_cache(buf_list), sizeof(*buf_list)); /* buffers before or after can be prefetched here */ list_item_del(buf_list); + dcache_invalidate_region(uncache_to_cache(buf_list), sizeof(*buf_list)); if (buffers_after_exist) dcache_invalidate_region(needs_sync_next, sizeof(struct list_item)); if (buffers_before_exist) diff --git a/src/audio/module_adapter/module_adapter.c b/src/audio/module_adapter/module_adapter.c index cb0796c50941..21c401de2b4f 100644 --- a/src/audio/module_adapter/module_adapter.c +++ b/src/audio/module_adapter/module_adapter.c @@ -18,6 +18,7 @@ #include #include #include +#include #include #include @@ -439,13 +440,17 @@ int module_adapter_prepare(struct comp_dev *dev) for (i = 0; i < mod->num_output_buffers; i++) { struct comp_buffer *buffer = buffer_alloc(buff_size, SOF_MEM_CAPS_RAM, 0, PLATFORM_DCACHE_ALIGN); + uint32_t flags; + if (!buffer) { comp_err(dev, "module_adapter_prepare(): failed to allocate local buffer"); ret = -ENOMEM; goto free; } + irq_local_disable(flags); buffer_attach(buffer, &mod->sink_buffer_list, PPL_DIR_UPSTREAM); + irq_local_enable(flags); buffer_c = buffer_acquire(buffer); buffer_set_params(buffer_c, mod->stream_params, BUFFER_UPDATE_FORCE); @@ -480,8 +485,11 @@ int module_adapter_prepare(struct comp_dev *dev) list_for_item_safe(blist, _blist, &mod->sink_buffer_list) { struct comp_buffer *buffer = container_of(blist, struct comp_buffer, sink_list); + uint32_t flags; + irq_local_disable(flags); buffer_detach(buffer, &mod->sink_buffer_list, PPL_DIR_UPSTREAM); + irq_local_enable(flags); buffer_free(buffer); } @@ -1404,8 +1412,11 @@ void module_adapter_free(struct comp_dev *dev) list_for_item_safe(blist, _blist, &mod->sink_buffer_list) { struct comp_buffer *buffer = container_of(blist, struct comp_buffer, sink_list); + uint32_t flags; + irq_local_disable(flags); buffer_detach(buffer, &mod->sink_buffer_list, PPL_DIR_UPSTREAM); + irq_local_enable(flags); buffer_free(buffer); }