audio: module_adapter: fix sizeof(pointer) and underflow in module_ext_init_decode#10748
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a vulnerability chain in IPC4 module extended-init decoding that could underflow spec->size and allow out-of-bounds mailbox reads during module adapter initialization.
Changes:
- Validate
spec->sizeagainst the fullext_initheader (sizeof(*ext_init)) before dereferencing any fields. - Prevent unsigned underflow by checking the computed consumed-byte count before adjusting
spec->size. - Add a defense-in-depth upper bound in
module_adapter_init_data()to reject configs larger thanMAILBOX_HOSTBOX_SIZE.
jsarha
left a comment
There was a problem hiding this comment.
Good caught. The copilot comment is valid, but not that critical (and the error is probably copypasted from my earlier print, feel free fix that too if you get to it).
| spec->size -= (unsigned char *)obj - spec->data; | ||
| consumed = (unsigned char *)obj - spec->data; | ||
|
|
||
| if (consumed > spec->size) { |
There was a problem hiding this comment.
I think this is already checked in the loop when processing that last object in lines 55-67 (with the patch applied), isn't it?
There was a problem hiding this comment.
You're right, with the sizeof fix in place this check is technically redundant. The no-loop path can't underflow because we already validated spec->size >= sizeof(*ext_init), and when the loop runs the bounds checks on lines 55-67 cover it.
I kept it as a defense-in-depth catch-all right before the subtraction. It's one comparison and it makes the function self-contained. If someone refactors the loop bounds logic later, this still prevents the underflow. Happy to drop it if you feel it's unnecessary noise.
dcde013 to
8da6464
Compare
…t_init_decode Three weaknesses compose into a single chain in module_ext_init_decode() that allows a crafted IPC4 ModuleInit payload to corrupt spec->size and spec->data before they are consumed by module_adapter_init_data(). The size guard used the wrong sizeof operand: if (spec->size < sizeof(ext_init)) /* sizeof(pointer) = 4, not 12 */ This accepted any payload >= 4 bytes even though the struct header is 12 bytes. Additionally, ext_init->data_obj_array was dereferenced before the guard ran, allowing the object-walk loop to be skipped with no size validation. When the loop is skipped, the unconditional spec->size adjustment: spec->size -= (unsigned char *)obj - spec->data; /* obj = data + 12 */ produces an unsigned underflow for spec->size in [4, 11], yielding values around 0xFFFFFFFC. The corrupted spec is then passed to module_adapter_init_data() where the inflated size bypasses the base_cfg guard and dst->base_cfg is populated from mailbox bytes beyond the declared payload boundary. Found by semgrep static analysis, confirmed by manual review of the caller chain through module_adapter_init_data(), and verified with prepared tests. Fixes: 1. Move size guard before ext_init dereference so spec->size is validated against sizeof(*ext_init) before any field is read. 2. Correct sizeof operand from sizeof(ext_init) to sizeof(*ext_init) (4 bytes → 12 bytes). 3. Guard the unconditional spec adjustment — compute consumed bytes and return -EINVAL if consumed > spec->size before subtracting. 4. Add upper-bound check in module_adapter_init_data() — reject cfgsz greater than MAILBOX_HOSTBOX_SIZE as a defense-in-depth measure. Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
Three weaknesses compose into a single chain in module_ext_init_decode() that allows a crafted IPC4 ModuleInit payload to corrupt spec->size and spec->data before they are consumed by module_adapter_init_data().
The size guard used the wrong sizeof operand:
if (spec->size < sizeof(ext_init)) /* sizeof(pointer) = 4, not 12 */
This accepted any payload >= 4 bytes even though the struct header is 12 bytes. Additionally, ext_init->data_obj_array was dereferenced before the guard ran, allowing the object-walk loop to be skipped with no size validation. When the loop is skipped, the unconditional spec->size adjustment:
spec->size -= (unsigned char )obj - spec->data; / obj = data + 12 */
produces an unsigned underflow for spec->size in [4, 11], yielding values around 0xFFFFFFFC. The corrupted spec is then passed to module_adapter_init_data() where the inflated size bypasses the base_cfg guard and dst->base_cfg is populated from mailbox bytes beyond the declared payload boundary.
Found by semgrep static analysis, confirmed by manual review of the caller chain through module_adapter_init_data(), and verified with prepared tests.
Fixes:
Move size guard before ext_init dereference so spec->size is validated against sizeof(*ext_init) before any field is read.
Correct sizeof operand from sizeof(ext_init) to sizeof(*ext_init) (4 bytes → 12 bytes).
Guard the unconditional spec adjustment — compute consumed bytes and return -EINVAL if consumed > spec->size before subtracting.
Add upper-bound check in module_adapter_init_data() — reject cfgsz greater than MAILBOX_HOSTBOX_SIZE as a defense-in-depth measure.