ipc3: Don't trust size values in protocol input #7854

andyross · 2023-06-22T21:22:08Z

The handling for SOF_COMP_NONE was extracting a "size" field from a
struct sof_ipc_comp_process command and passing it down into the
module_adapter layer where it was being used as the range for a
memcpy_s().

But that struct is IPC input data from the host! The size can
trivially be wrong and cause an overflow. And needless to say,
fuzzing discovered the hole and blew it up.

Note that the previous behavior when LIBRARY||UNIT_TEST is left
unchanged (fuzz builds as "real" firmware and doesn't use those
features). It turns out that this is still in production use, though
the details of why the code is different are unclear. Drop a note in
to fix this later.

lyakh · 2023-06-26T06:00:49Z

src/ipc/ipc3/helper.c

no, proc->data is an array, so this is perfectly valid

Why do we handle this differently on host ? Expectation is that ABI should be the same, I ssuspect the testbench is doing something different @singalsu @andrula-song ?

I think the data advance to past possible UUID can be done in every build, not just UT and testbench. The kernel is not sending the data bundled to just after comp new IPC like tests do, but in separate binary control IPC, so it has not been needed. If there is no blob the config->process.size is zero and config->process.data is not accessed.

btian1 · 2023-06-26T06:31:05Z

src/ipc/ipc3/helper.c

it is not only for NONE, it is for ten component cases, check line :268-278.
Are you sure all above cases all does not need this?

lgirdwood

I suspect UT or testbench is doing something different to runtime FW here and should be fixed to align (alongside this).

lgirdwood · 2023-07-03T15:54:14Z

src/ipc/ipc3/helper.c

Why do we handle this differently on host ? Expectation is that ABI should be the same, I ssuspect the testbench is doing something different @singalsu @andrula-song ?

andyross · 2023-07-06T17:37:38Z

So what's the consensus here? As @lyakh points out, my explanation is wrong. It's not the buffer pointer that is at fault, it's that the resulting transfer is unbounded. Nothing in the current scheme passes a valid size for the buffer, so we don't know how much we can copy.

I can try to fix this (by extracting a size from somewhere up higher in the IPC layer I guess), but it seems like there's confusion about whether or not the unit test stuff needs it?

My assumption had been that this code path would never be hit in practice and was just a fuzzable edge. What's the actual circumstance where you'd get data passed in any testing rig for a COMP_NONE component?

Broadly: what happens if we just turn the COMP_NONE handling into a runtime error instead? That would be even less code.

lgirdwood · 2023-07-06T19:40:36Z

@andrula-song what happens if we make the host handling the same as FW handling here ? Does testbench still work as expected ? We need to align handling here.

andrula-song · 2023-07-07T06:34:54Z

@andrula-song what happens if we make the host handling the same as FW handling here ? Does testbench still work as expected ? We need to align handling here.

if we make the host handling the same as FW handling here would lead to those sof_ipc_comp_process components(eq,mux/demux) unit test failed. Checked the code, the comp->ext_data_length is UUID_SIZE.

lgirdwood · 2023-07-07T13:01:04Z

@andrula-song what happens if we make the host handling the same as FW handling here ? Does testbench still work as expected ? We need to align handling here.

if we make the host handling the same as FW handling here would lead to those sof_ipc_comp_process components(eq,mux/demux) unit test failed. Checked the code, the comp->ext_data_length is UUID_SIZE.

Thanks it does sound the UT is broken then ? Can the UTs be fixed to use same handling as FW ?

andrula-song · 2023-07-11T03:09:31Z

hi @lgirdwood
yes, it will break the UT for eq-iir, eq-fir,mux and demux. I tried to remove the 'ext_data_length' segment yesterday, but will get error " get_drv(): driver not found,". Seems needs more effort to remove this segment.

lgirdwood · 2023-07-11T12:57:15Z

@andyross we need to fix the length issue first. Seppo is best to look at this when he's back. Not sure why testbench is doing something else.

The handling for SOF_COMP_NONE was extracting a "size" field from a struct sof_ipc_comp_process command and passing it down into the module_adapter layer where it was being used as the range for a memcpy_s(). But that struct is IPC input data from the host! The size can trivially be wrong and cause an overflow. And needless to say, fuzzing discovered the hole and blew it up. Note that the previous behavior when LIBRARY||UNIT_TEST is left unchanged (fuzz builds as "real" firmware and doesn't use those features). It turns out that this is still in production use, though the details of why the code is different are unclear. Drop a note in to fix this later. Signed-off-by: Andy Ross <andyross@google.com>

andyross · 2023-07-11T14:34:11Z

OK. This version leaves the UNIT_TEST/LIBRARY path unchanged (with a FIXME note). It will have the same bug on bad input, but that isn't involved in either production firmware nor fuzzing, so nothing is getting worse at least.

Also, instead of nulling the pointer out with the bad explanation, this does a slightly more sophisticated trick where it clamps the protocol-provided garbage size value (which was the real root cause) to the known size of the IPC command buffer. I still argue we should remove this whole switch case and replace it with an error, but I guess that will need to wait for the test updates?

lyakh

This is now handling the same fuzzing detected problem as #7830 and as I commented there in #7830 (comment) I think this function should return an error in case of invalid input data

src/ipc/ipc3/helper.c

singalsu · 2023-07-27T15:22:25Z

What do you think of this fix approach?

diff --git a/src/include/ipc/header.h b/src/include/ipc/header.h
index 04b97bb80..0ea1d9f80 100644
--- a/src/include/ipc/header.h
+++ b/src/include/ipc/header.h
@@ -329,7 +329,11 @@ struct ipc_cmd_hdr;
 #define SOF_IPC_MESSAGE_ID(x)			((x) & 0xffff)
 
 /** Maximum message size for mailbox Tx/Rx */
+#if CONFIG_LIBRARY || UNIT_TEST
+#define SOF_IPC_MSG_MAX_SIZE			1024
+#else
 #define SOF_IPC_MSG_MAX_SIZE			384
+#endif
 
 /** @} */
 
diff --git a/src/ipc/ipc3/helper.c b/src/ipc/ipc3/helper.c
index 96124538d..9d7527dae 100644
--- a/src/ipc/ipc3/helper.c
+++ b/src/ipc/ipc3/helper.c
@@ -276,13 +276,14 @@ static int comp_specific_builder(struct sof_ipc_comp *comp,
 	case SOF_COMP_SMART_AMP:
 	case SOF_COMP_MODULE_ADAPTER:
 	case SOF_COMP_NONE:
+		if (proc->size + comp->ext_data_length > SOF_IPC_MSG_MAX_SIZE - sizeof(*proc)) {
+			tr_err(&comp_tr, "comp_specific_builder(): Illegal process IPC size");
+			return -ENOMEM;
+		}
+
 		config->process.type = proc->type;
 		config->process.size = proc->size;
-#if CONFIG_LIBRARY || UNIT_TEST
-		config->process.data = proc->data + comp->ext_data_length;
-#else
-		config->process.data = proc->data;
-#endif
+		config->process.data = proc->data + comp->ext_data_length; /* Skip possible UUID */
 		break;
 	case SOF_COMP_MIXER:
 		break;

There are some unit tests with large blobs, so with this check in place the IPC max size needs an increase for CONFIG_LIBRARY and UNIT_TEST. Testbench could send the multi-part IPC but it would be a quite big change for unit tests, so proposing this way now.

singalsu

Need to error the exceed, not limit. Also limiting would break FIR UT.

the comment, triggering the change request has been resolved. Now just a specific has to be agreed upon

singalsu · 2023-07-28T10:23:11Z

I removed my reject to not block this while I'm one week OOO. But I don't agree with this (no error, cut end of too long IPC) fix approach.

lgirdwood · 2023-07-31T14:57:44Z

I removed my reject to not block this while I'm one week OOO. But I don't agree with this (no error, cut end of too long IPC) fix approach.

ack, it seems the best course is to return an error up the stack and add some error message in the logs.

andyross · 2023-08-08T12:10:53Z

Sorry, this got left stale. I'm going to close this, as AFAICT the error it addresses isn't repeating post- #7830 The side question about why unit testing is using a variant protocol can be addressed elsewhere.

andyross requested review from dbaluta, lbetlej, lgirdwood, mmaka1 and plbossart as code owners June 22, 2023 21:22

andyross mentioned this pull request Jun 22, 2023

Audio: SRC: Add safeguard against completely missing init_data #7830

Merged

lyakh previously requested changes Jun 26, 2023

View reviewed changes

btian1 reviewed Jun 26, 2023

View reviewed changes

lgirdwood reviewed Jul 3, 2023

View reviewed changes

lgirdwood assigned singalsu Jul 11, 2023

lgirdwood added this to the v2.7 milestone Jul 11, 2023

andyross force-pushed the comp-none-data branch from 86c5855 to ff3bc08 Compare July 11, 2023 14:30

andyross changed the title ~~ipc3: Don't exract local pointer values from input data~~ ipc3: Don't trust size values in protocol input Jul 11, 2023

kv2019i approved these changes Jul 12, 2023

View reviewed changes

lyakh reviewed Jul 12, 2023

View reviewed changes

lgirdwood reviewed Jul 12, 2023

View reviewed changes

src/ipc/ipc3/helper.c Show resolved Hide resolved

dbaluta approved these changes Jul 27, 2023

View reviewed changes

singalsu requested changes Jul 27, 2023

View reviewed changes

singalsu self-requested a review July 28, 2023 10:21

btian1 approved these changes Aug 8, 2023

View reviewed changes

andyross closed this Aug 8, 2023

ipc3: Don't trust size values in protocol input #7854

ipc3: Don't trust size values in protocol input #7854

Uh oh!

Conversation

andyross commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lyakh Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

lgirdwood Jul 3, 2023

Choose a reason for hiding this comment

Uh oh!

singalsu Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

btian1 Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

lgirdwood Jul 3, 2023

Choose a reason for hiding this comment

Uh oh!

andyross commented Jul 6, 2023

Uh oh!

lgirdwood commented Jul 6, 2023

Uh oh!

andrula-song commented Jul 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood commented Jul 7, 2023

Uh oh!

andrula-song commented Jul 11, 2023

Uh oh!

lgirdwood commented Jul 11, 2023

Uh oh!

andyross commented Jul 11, 2023

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

singalsu commented Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

singalsu left a comment

Choose a reason for hiding this comment

Uh oh!

singalsu commented Jul 28, 2023

Uh oh!

lgirdwood commented Jul 31, 2023

Uh oh!

andyross commented Aug 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

andyross commented Jun 22, 2023 •

edited

Loading

singalsu Jul 27, 2023 •

edited

Loading

andrula-song commented Jul 7, 2023 •

edited

Loading

singalsu commented Jul 27, 2023 •

edited

Loading