ipc3: Add size checks on ipc subtypes

[cujomalainey]

We have a few gaps in input validation from the kernel. Right now we check the IPC doesn't claim its larger the window, this patch adds the following checks:

1. That the IPC size is at least large enough for any downcast type on comp new
2. That any reported size for a process total size is less than the IPC window.

[cujomalainey]

Pushed as draft as validation is still ongoing but I wanted early input

[cujomalainey]

whoops, was building IPC4 when testing, not IPC3, will fix tomorrow

[lyakh]

at the very least maybe IPC_TAIL_SIZE_IS_INVALID() and ideally I'd make this

#define IPC_TAIL_SIZE_IS_VALID(object) \
	((object).hdr.size + (object).ext_data_length == sizeof(object))

because in general affirmative names might be easier to work with, and the == comparison already returns a perfect boolean result, but the definition above already does the other way, so...

[cujomalainey]

I would rather IPC_TAIL_IS_SIZE_INVALID to keep the naming semantics the same as above or update the one above to match your suggestion. But i rather not deviate on this alone.

Seconded on the brackets, i thought that was weird, not sure why they were ommitted

I am not sure either, it is the same as using != I am really not sure what is going on with that macro, i can update both to keep them the same

Looks like @lgirdwood was the original author of the macro. Maybe he can share some of his logic

[cujomalainey]

@singalsu The testbench is failing due to bad test cases. on the ext_data_length I am seeing the following issue in the testbench CI. scratch that, i think comp->size is a subset of hdr.size

EXPECTED = 384 (SOF_IPC_MSG_MAX_SIZE)
GOT = 716 (proc->comp.hdr.size + proc->comp.ext_data_length + proc->size)

@lyakh I flipped the comparison to < because it seems that testbench has other bad cases as well and as long as we have the memory it should be safe.

[cujomalainey]

On second thought this is possibly more of a mess than i originally thought.

This check here given its a == check seems to imply that the size of the IPC is locked to sizeof(struct sof_ipc_comp) which is not true! There is clearly additional data appended based on comp_specific_builder.

So what do we want to fix? Do we want to have the firmware not able to trust the top level size? Or do we want to fix the kernel and firmware to properly emit sizes in the top level header (likely bringing in breakages between versions).

[cujomalainey]

@lgirdwood @plbossart thoughts on the proper fix here? I would argue the ABI spec is either wrong or violated here

[cujomalainey]

Moving to bug for discussion

[cujomalainey]

Change of plans, i don't think an abi change will help, going to continue hammering this.

[lgirdwood]

I think this looks correct for IPC3

[lgirdwood]

SOFCI TEST

[lgirdwood]

Reruns CI as been pending for a while.