rimage: various rimage fixes by lgirdwood · Pull Request #10882 · thesofproject/sof

lgirdwood · 2026-06-11T14:56:54Z

Mostly minor things that provide better ELF handling and error handling.

A symbol name was duplicated from the string table without verifying a terminator within the section, so an unterminated table could be read past its end. Validate the index and bound the length to the section. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

Copilot

Pull request overview

This PR hardens rimage by improving validation of untrusted inputs (ELF/firmware images) and by fixing several error-handling and bounds-checking issues, aligning with the goal of “better ELF handling and error handling”.

Changes:

Add input validation to prevent unsigned underflow/overreads when computing manifest verification hash ranges.
Add bounds checks for ELF-derived counts/offsets (module manifests, string tables, extended manifest headers).
Improve verification/resign scanning loops and propagate verification results instead of always returning success.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tools/rimage/src/pkcs1_5.c	Adds guards around CSS size/header length usage during manifest verification.
tools/rimage/src/manifest.c	Adds bounds checking for `.module` manifests; improves `$CPD` scan loop bounds; returns verification status.
tools/rimage/src/hash.c	Fixes output buffer size validation for OpenSSL digest computation.
tools/rimage/src/ext_manifest.c	Adds a header-boundary check when iterating extended manifest elements.
tools/rimage/src/elf_file.c	Hardens string table access (negative index, bounds, and NUL-termination validation).

Comments suppressed due to low confidence (3)

tools/rimage/src/manifest.c:1684

In verify mode, image->image_end is never set before calling verify_firmware(). This breaks v1.5 verification (ri_manifest_verify_v1_5 uses image_end for size calculation) and can lead to out-of-bounds reads/incorrect hashing. Also, verify_firmware() returns OpenSSL-style values (1 valid, 0 invalid), so returning it directly will invert/lose the intended CLI exit-code semantics. Set image_end based on the detected CSE header offset and normalize the verify result to 0 on success and a negative errno on failure.

		if (*(uint32_t *)(buffer + i) == CSE_HEADER_MAKER) {
			image->fw_image = buffer + i;
			ret = image->adsp->verify_firmware(image);
			goto out;

tools/rimage/src/pkcs1_5.c:945

The new underflow guard helps, but css.size/header_len are still untrusted. size2 is used to hash memory starting at MAN_SIG_PKG_OFFSET_V1_8; without bounding it against image->image_end, a crafted image can cause out-of-bounds reads in hash_update(). Add a bound check to ensure the computed payload fits within the verified image buffer.

	unsigned const size2 =
		(man->css.size - man->css.header_len) * sizeof(uint32_t);

	return pkcs_v1_5_verify_man_v1_8(image, man, data1, size1, data2, size2);

tools/rimage/src/pkcs1_5.c:972

The new underflow guard helps, but css.size/header_len are still untrusted. size2 is used to hash memory starting at MAN_SIG_PKG_OFFSET_V2_5; without bounding it against image->image_end, a crafted image can cause out-of-bounds reads in hash_update(). Add a bound check to ensure the computed payload fits within the verified image buffer.

	unsigned const size2 =
			(man->css.size - man->css.header_len) * sizeof(uint32_t);

	return pkcs_v1_5_verify_man_v2_5(image, man, data1, size1, data2, size2);

lgirdwood · 2026-06-12T13:20:56Z

+	if (n_mod > MAX_MODULES) {
+		fprintf(stderr, "error: too many module manifests (%u > %u) in '.module' section.\n",
+			n_mod, MAX_MODULES);
+		elf_section_free(&section);
+		return -ENOEXEC;
+	}


Good catch — fixed. The check now bounds the cumulative slot count rather than just this ELF's: if (modules->output_mod_cfg_count + n_mod > MAX_MODULES), so a set of crafted ELFs can't overflow the descriptor array between them.

lgirdwood · 2026-06-12T13:20:57Z

+		/* make sure a whole header remains before copying it out */
+		if (offset + sizeof(head) > section_size) {
+			fprintf(stderr,
+				"error: extended manifest header straddles section end\n");
+			return -EINVAL;


Fixed — ext_man_write() no longer overwrites the result with -errno. ext_man_validate() already returns -EINVAL, so the caller now propagates that return value directly instead of an unset errno.

lyakh · 2026-06-12T09:02:33Z

+	}

-	*str = strdup((const char *)strings->section.data + index);
+	*str = strdup(base + index);


for my understanding: rimage is "just" a helper tool run by a user locally to generate firmware images. So they always can just create corrupted firmware images themselves - using whatever tools they want? It would be only "attackable" in scenarios where users submit ELF images to some deployment service? It's good to tighten up things of course, just trying to understand the scope, the severity level

Right — rimage is a local build-time tool, so for a developer building their own firmware the threat model is minimal: they can already produce any image they like. The case these harden is a build/CI pipeline that runs rimage on untrusted inputs — e.g. an automated signer/verifier gating contributed ELF/firmware. There a crafted input could crash the tool, or (for the verify exit-code bug) cause a tampered image to be accepted by a rimage -y && deploy gate. So the severity is low: robustness / CI-integrity hardening, not a firmware-runtime vulnerability — which is why these are rated Low.

lyakh · 2026-06-12T09:08:33Z

+	 * must be at least that large; reject an undersized output buffer
+	 */
+	if (output_len < (size_t)algo_out_size)
 		return -ENOBUFS;


this is interesting. How did it work before? It isn't the normal case that they are equal, is it?

It is the normal case that they're equal — every in-tree caller (hash_sha256/hash_sha384 and the manifest code) passes an output buffer sized exactly to the digest. The old guard if (out_len > digest_len) therefore passed for the equal case and only (wrongly) rejected oversized buffers, which nobody passes. The genuinely broken case — an undersized out_len, which would overflow on the digest write — was never exercised by any in-tree caller, so the bug was latent. The fix just makes the guard correct for that case; hence Low/defensive.

The signed module size was computed as a difference of two header fields from the image without an ordering check, underflowing to a huge value that was then hashed past the buffer. Reject a header whose size is less than its header length. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

The number of module manifests packed in a section came from the input file and drove writes into the fixed-size module descriptor array. Reject a count above the maximum before the write loop. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

Verify mode logged signature failures and a missing header but returned success, so callers using the exit code as a gate would accept a tampered image. Return an error when no header is found and propagate the verification result. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

The extended-manifest validator copied a fixed header from an offset that could leave fewer than a header's worth of bytes, reading past the section. Require a whole header to remain before the copy. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

The output-buffer guard was reversed: it rejected oversized buffers and accepted undersized ones, so a short output buffer could be overflowed by the digest write. Reject when the buffer is smaller than the digest. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

The marker scan read a 32-bit word at each offset up to the last byte, reading a few bytes past the buffer at the tail. Stop once fewer than a word remains, in both scan sites. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

lgirdwood · 2026-06-12T13:20:59Z

Also addressed the low-confidence points from the Copilot summary:

verify_firmware result / image_end: verify_firmware() returns OpenSSL RSA_verify semantics (1 = valid, 0 = invalid, < 0 = error), so returning it directly would have inverted the CLI exit code. It's now normalized to 0 on success and a negative errno otherwise. image->image_end is also set (to the size from the detected $CPD header to EOF) before verification, which v1.5 needs.
pkcs1_5.c size2 bounds: the signed-payload range (MAN_SIG_PKG_OFFSET .. + size2) is now bounded against image->image_end for both v1.8 and v2.5, so a crafted css.size/header_len can't drive an out-of-bounds read in hash_update().

All folded into their respective commits and force-pushed.

lyakh · 2026-06-12T14:10:35Z

+			if (ret == 1)
+				ret = 0;
+			else if (ret >= 0)
+				ret = -EINVAL;


this last one now looks really strange to me... Sure not <= 0?

Copilot AI review requested due to automatic review settings June 11, 2026 14:56

lgirdwood requested review from dbaluta, kv2019i, lbetlej, mmaka1 and plbossart as code owners June 11, 2026 14:56

Copilot started reviewing on behalf of lgirdwood June 11, 2026 14:57 View session

Copilot AI reviewed Jun 11, 2026

View reviewed changes

lyakh reviewed Jun 12, 2026

View reviewed changes

lgirdwood force-pushed the fix-rimage branch from 3053c2e to 4c0740c Compare June 12, 2026 09:41

lrgirdwo added 6 commits June 12, 2026 13:06

lgirdwood force-pushed the fix-rimage branch from 4c0740c to 01a0eed Compare June 12, 2026 12:07

lyakh reviewed Jun 12, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rimage: various rimage fixes#10882

rimage: various rimage fixes#10882
lgirdwood wants to merge 7 commits into
thesofproject:mainfrom
lgirdwood:fix-rimage

lgirdwood commented Jun 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

lgirdwood Jun 12, 2026

Uh oh!

lgirdwood Jun 12, 2026

Uh oh!

lyakh Jun 12, 2026

Uh oh!

lgirdwood Jun 12, 2026

Uh oh!

lyakh Jun 12, 2026

Uh oh!

lgirdwood Jun 12, 2026

Uh oh!

lgirdwood commented Jun 12, 2026

Uh oh!

lyakh Jun 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

lgirdwood commented Jun 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

lgirdwood Jun 12, 2026

Choose a reason for hiding this comment

Uh oh!

lgirdwood Jun 12, 2026

Choose a reason for hiding this comment

Uh oh!

lyakh Jun 12, 2026

Choose a reason for hiding this comment

Uh oh!

lgirdwood Jun 12, 2026

Choose a reason for hiding this comment

Uh oh!

lyakh Jun 12, 2026

Choose a reason for hiding this comment

Uh oh!

lgirdwood Jun 12, 2026

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Jun 12, 2026

Uh oh!

lyakh Jun 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants