From 69b30b6150db1ac15ed9eb586980bfeb8896fc66 Mon Sep 17 00:00:00 2001 From: Andy Ross Date: Tue, 6 Jun 2023 08:01:53 -0700 Subject: [PATCH 1/4] kpb: Don't assert on memcpy overruns The config block for the component gets provided externally and is copied directly in using a byte count likewise provided by the host. The use of memcpy_s() prevents overruns, but the error that was detected was being reported via assert(). To fuzzing, that assertion is a fatal error, when clearly this needs to be a runtime failure as it's due to external input and not a local code bug. Signed-off-by: Andy Ross --- rimage | 2 +- src/audio/kpb.c | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/rimage b/rimage index 4ce79b152e23..ab0429fdbe56 160000 --- a/rimage +++ b/rimage @@ -1 +1 @@ -Subproject commit 4ce79b152e23c7b3937a09ef57cdd69d125a9214 +Subproject commit ab0429fdbe563ef6abe499c69b2483e96c4762d0 diff --git a/src/audio/kpb.c b/src/audio/kpb.c index b3af7cfbe70b..7716e0efebae 100644 --- a/src/audio/kpb.c +++ b/src/audio/kpb.c @@ -394,7 +394,12 @@ static int kpb_set_verify_ipc_params(struct comp_dev *dev, ret = memcpy_s(&kpb->config, sizeof(kpb->config), ipc_config->data, ipc_config->size); - assert(!ret); + + if (ret) { + comp_err(dev, "kpb_new(): cannot memcpy_s %d bytes into sof_kpb_config (%d)\n", + ipc_config->size, sizeof(kpb->config)); + return -EINVAL; + } /* Initialize sinks */ kpb->sel_sink = NULL; From daee0b689b4301bd2f31c7b73ad270f849271638 Mon Sep 17 00:00:00 2001 From: Andy Ross Date: Tue, 6 Jun 2023 08:07:10 -0700 Subject: [PATCH 2/4] ipc/pipeline: Check type in ipc_pipeline_complete() This function is called based on external commands, and with an arbitrary component ID that may not actually be a pipeline. Check the type before following garbage pointers and passing them down into call trees. Found via fuzzing. Signed-off-by: Andy Ross --- src/ipc/ipc-helper.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/ipc/ipc-helper.c b/src/ipc/ipc-helper.c index 058795f9865f..b5d20164ef71 100644 --- a/src/ipc/ipc-helper.c +++ b/src/ipc/ipc-helper.c @@ -201,6 +201,12 @@ int ipc_pipeline_complete(struct ipc *ipc, uint32_t comp_id) return -EINVAL; } + if (ipc_pipe->type != COMP_TYPE_PIPELINE) { + tr_err(&ipc_tr, "ipc_pipeline_complete(): component %d not pipeline (type %d)", + ipc_pipe->id, ipc_pipe->type); + return -EINVAL; + } + /* check core */ if (!cpu_is_me(ipc_pipe->core)) return ipc_process_on_core(ipc_pipe->core, false); From 6dcf11e5c913ea979960562a224e4758789e2a11 Mon Sep 17 00:00:00 2001 From: Andy Ross Date: Tue, 6 Jun 2023 08:08:33 -0700 Subject: [PATCH 3/4] ipc: Tolerate uninitialized source/sink list nodes These list heads in the comp_dev struct are not uniformly initialized (grepping the source, the list_init() calls for these fields seem to be spread around the source tree in individual components). Fuzzing is seeing nulls here, presumably because it's possible to reach ipc_comp_free() in "unintended lifecycle" circumstances where they weren't initialized. Check the fields before crashing. Signed-off-by: Andy Ross --- src/ipc/ipc-helper.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/ipc/ipc-helper.c b/src/ipc/ipc-helper.c index b5d20164ef71..a8fb4b41f548 100644 --- a/src/ipc/ipc-helper.c +++ b/src/ipc/ipc-helper.c @@ -291,6 +291,21 @@ int ipc_comp_free(struct ipc *ipc, uint32_t comp_id) return -EINVAL; } + if (!icd->cd->bsource_list.next || !icd->cd->bsource_list.next) { + /* Unfortunate: the buffer list node gets initialized + * at the component level and thus can contain NULLs + * (which is an invalid list!) if the component's + * lifecycle hasn't reached that point. There's no + * single place to ensure a valid/empty list, so we + * have to do it here and eat the resulting memory + * leak on error. Bug-free host drivers won't do + * this, this was found via fuzzing. + */ + tr_err(&ipc_tr, "ipc_comp_free(): uninitialized buffer lists on comp %d\n", + icd->id); + return -EINVAL; + } + irq_local_disable(flags); list_for_item_safe(clist, tmp, &icd->cd->bsource_list) { struct comp_buffer *buffer = container_of(clist, struct comp_buffer, sink_list); From 5b797b0a99b3ee90a5c80a7f44471a7ccbde69a8 Mon Sep 17 00:00:00 2001 From: Andy Ross Date: Tue, 6 Jun 2023 08:11:18 -0700 Subject: [PATCH 4/4] ipc3: Check component type in ipc_stream_pcm_params() This handler was missing its type check and would follow comp_dev union fields on components that might be buffers or pipelines, leading to crashes downstream. Found via fuzzing. Signed-off-by: Andy Ross --- src/ipc/ipc3/handler.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/ipc/ipc3/handler.c b/src/ipc/ipc3/handler.c index 24249f09b86e..bf45bb4ebb53 100644 --- a/src/ipc/ipc3/handler.c +++ b/src/ipc/ipc3/handler.c @@ -222,6 +222,12 @@ static int ipc_stream_pcm_params(uint32_t stream) return -ENODEV; } + if (pcm_dev->type != COMP_TYPE_COMPONENT) { + tr_err(&ipc_tr, "ipc_stream_pcm_params(): pcm_dev id %d not component (type %d)", + pcm_dev->id, pcm_dev->type); + return -EINVAL; + } + /* check core */ if (!cpu_is_me(pcm_dev->core)) return ipc_process_on_core(pcm_dev->core, false);