lib_manager: bound build info offset and check resume allocation#10931
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Hardens library manager restore and module parsing paths by adding bounds/NULL checks to prevent OOB reads and NULL dereferences under adverse conditions.
Changes:
- Add NULL-check for resume-path context allocation before dereferencing.
- Validate manifest-supplied TEXT segment
file_offsetagainst the library image size before using it to derivebuild_info.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/library_manager/llext_manager_dram.c | Adds allocation failure handling on DRAM/IMR resume path. |
| src/library_manager/lib_manager.c | Adds bounds check for TEXT file_offset before dereferencing derived build-info pointer. |
| const size_t lib_size = (size_t)desc->header.preload_page_count * PAGE_SZ; | ||
| const uint32_t text_off = mod->segment[SOF_MAN_SEGMENT_TEXT].file_offset; | ||
|
|
||
| if (text_off > lib_size || lib_size - text_off < sizeof(*build_info)) { | ||
| tr_err(&lib_manager_tr, "Invalid TEXT file_offset %u", text_off); | ||
| return MOD_TYPE_INVALID; | ||
| } |
There was a problem hiding this comment.
Per maintainer guidance we're not adding the per-multiply overflow check here (the same preload_page_count * PAGE_SZ is computed without it at the other call sites, and the value comes from the manifest header). Kept consistent rather than guarding only this one site.
There was a problem hiding this comment.
Lets not trust the manifest header i.e. if the manifest is wrong, lets check and LOG / return the error up the stack.
| if (text_off > lib_size || lib_size - text_off < sizeof(*build_info)) { | ||
| tr_err(&lib_manager_tr, "Invalid TEXT file_offset %u", text_off); | ||
| return MOD_TYPE_INVALID; | ||
| } |
There was a problem hiding this comment.
Done — the error now logs both the offending offset and the limit: "Invalid TEXT file_offset %u, lib_size %zu".
| * against the library image size before dereferencing so a crafted | ||
| * offset cannot read outside the library buffer. | ||
| */ | ||
| { |
There was a problem hiding this comment.
Done — dropped the scope block; lib_size and text_off are now declared with the other locals at the top of the function.
| * offset cannot read outside the library buffer. | ||
| */ | ||
| { | ||
| const size_t lib_size = (size_t)desc->header.preload_page_count * PAGE_SZ; |
There was a problem hiding this comment.
elsewhere we added overflow-controlled multiplication for this. I think we don't need to check preload_page_count every time we multiply it by PAGE_SZ so I wouldn't do that overflow-check everywhere, but just mentioning it here to make sure we do this consciously, not by oversight
There was a problem hiding this comment.
Understood — I'll leave the preload_page_count * PAGE_SZ here without an overflow check, consistent with the other call sites, rather than sprinkling the check everywhere. preload_page_count comes from the (already auth-checked, on production) manifest header. Noting it consciously per your point.
There was a problem hiding this comment.
lets confirm the overflow check is done only once and we catch any manifest OOB values.
| if (!ctx) { | ||
| tr_err(&lib_manager_tr, "library context allocation failure"); | ||
| goto nomem; | ||
| } |
There was a problem hiding this comment.
ouch... that's a fallout from 5821682#diff-4fe4a5f637539df7e066bafb9c10d019c5e7764a6438aab2e3520b36dfaa61daL194-R195
There was a problem hiding this comment.
Right — the missing NULL check was a regression from that refactor; this restores it (fail the restore gracefully instead of writing through a NULL ctx). No code change needed beyond what's here.
The build info pointer was derived from a manifest-supplied text segment offset without bounds, so a crafted manifest could read outside the library buffer. Validate the offset against the library image size before dereferencing and fail the module type lookup otherwise. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
The resume path allocated a library context and immediately wrote through it without a NULL check, crashing under memory pressure. Check the allocation and fail the restore gracefully. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
f153935 to
7d796ce
Compare
Two robustness fixes in the library manager:
image size before deriving the build-info pointer, so a crafted manifest
cannot read outside the library buffer
through it (NULL dereference under memory pressure otherwise)