smex: harden ELF parsing against malformed input#10922
Conversation
lgirdwood
requested review from
dbaluta,
kv2019i,
lbetlej,
mmaka1 and
plbossart
as code owners
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Harden smex ELF parsing when handling malformed/untrusted objects by adding bounds checks and safer error handling around string tables, section lookups, and manifest parsing.
Changes:
- Validate section-header string table index/size and bound section-name offsets against the string table size.
- Prevent double-free by nulling the caller buffer on
elf_read_section()error. - Add size/bounds checks for
.fw_readyand extended manifest element walking.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| smex/ldc.c | Adds defensive size/bounds checks when parsing .fw_ready and extended manifest elements. |
| smex/elf.c | Validates shstrndx/string-table invariants, bounds section name offsets, and hardens error cleanup. |
| while ((uintptr_t)ext_hdr + sizeof(*ext_hdr) <= | ||
| (uintptr_t)buffer + section_size) { | ||
| if (ext_hdr->type == EXT_MAN_ELEM_DBG_ABI) { | ||
| header->version.abi_version = | ||
| ((struct ext_man_dbg_abi *) | ||
| ext_hdr)->dbg_abi.abi_dbg_version; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Fixed — before reading the dbg-abi element it now checks elem_size >= sizeof(struct ext_man_dbg_abi) and that the whole struct lies within the section; otherwise it returns -ENOEXEC.
| /* elem_size must advance the cursor and keep the next header | ||
| * within the section; otherwise stop instead of looping | ||
| * forever (elem_size == 0) or reading past the section | ||
| */ | ||
| if (ext_hdr->elem_size == 0 || | ||
| (uintptr_t)ext_hdr + ext_hdr->elem_size > | ||
| (uintptr_t)buffer + section_size) | ||
| break; |
There was a problem hiding this comment.
Changed — a malformed element (elem_size == 0 or one that runs past the section) now returns -ENOEXEC (frees the buffer first) instead of breaking out as if parsing succeeded. The "no DBG_ABI extension present" case still completes normally and keeps the kernel ABI version.
| module->strings = calloc(1, section[hdr->shstrndx].size); | ||
| if (!module->strings) { |
There was a problem hiding this comment.
Fixed — the string table is now allocated with one extra byte (calloc(1, size + 1), which zeroes it), so it's always NUL-terminated and the name lookups/compares can't run off the end even if the section itself lacks a terminator.
| if (hdr->shstrndx >= hdr->shnum) { | ||
| fprintf(stderr, "error: invalid shstrndx %u >= shnum %u\n", | ||
| hdr->shstrndx, hdr->shnum); | ||
| return -EINVAL; | ||
| } |
There was a problem hiding this comment.
SHN_XINDEX (extended section numbering) only applies when a file has >= 0xff00 sections, which doesn't occur for SOF firmware images (they have a handful of sections). Supporting it would mean reading the real index from section[0].sh_link, which is out of scope here — rejecting is the safe behaviour for these inputs. I can add an explicit SHN_XINDEX detection + clearer message if you'd prefer.
| if (section[i].name >= section[hdr->shstrndx].size) { | ||
| fprintf(stderr, "error: %s section %d name offset %u out of range\n", | ||
| module->elf_file, i, section[i].name); | ||
| return -EINVAL; |
There was a problem hiding this comment.
In Zephyr ELF format bugs are usually marked with ENOEXEC, might be good to do the same here
There was a problem hiding this comment.
Done — switched the ELF-format rejections (bad shstrndx, zero-size string section, out-of-range section name offset) to -ENOEXEC to match the Zephyr convention.
The section name string table was located using a header index without bounding it against the section count, and was null terminated at size minus one without checking for a zero-size section. Reject an out-of-range index and a zero-size string section before use. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
On a read error the section reader freed the output buffer but left the caller's pointer set, so a caller cleanup path could free it again. Clear the pointer after freeing. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
Section names are used as offsets into the string table; an out-of-range name offset from a crafted ELF read past the table. Validate every section name offset against the string-table size after loading the sections. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
The fw-ready section was cast to a struct and field-read without checking its size, reading past a short section. Reject a section smaller than the struct. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
The extended-manifest walk advanced by an element size read from the section without validating it, so a zero size looped forever and a large size read past the section. Stop on a zero size or one that would leave the section. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
Harden smex against malformed/untrusted ELF input (e.g. a build pipeline
running smex on a contributed object):
string section before indexing
double-free in the caller
.fw_readysection before casting it to a structruns past the section)