Skip to content

rimage: various rimage fixes #10882

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lgirdwood wants to merge 7 commits into
base: main
Choose a base branch
from
Open

rimage: various rimage fixes #10882

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b7b1d87
rimage: bound string table lookups to the section size
lrgirdwo Jun 11, 2026
b0e1da3
rimage: reject reversed manifest size fields
lrgirdwo Jun 11, 2026
ef41cc1
rimage: bound module manifest count from input
lrgirdwo Jun 11, 2026
35067ac
rimage: propagate verification result to the exit code
lrgirdwo Jun 11, 2026
95fb7b7
rimage: bound extended manifest header read to the section
lrgirdwo Jun 11, 2026
9ba74f6
rimage: fix inverted hash output buffer size check
lrgirdwo Jun 11, 2026
01a0eed
rimage: avoid reading past the buffer scanning for the marker
lrgirdwo Jun 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions tools/rimage/src/elf_file.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,10 +531,23 @@ int elf_strings_read_by_index(const struct elf_file *elf, int index, struct elf_

int elf_strings_get(const struct elf_strings *strings, int index, char **str)
{
if (index >= strings->section.header.data.size)
size_t size = strings->section.header.data.size;
const char *base = (const char *)strings->section.data;

if (index < 0 || (size_t)index >= size)
return -EINVAL;

/*
* A crafted ELF may provide a string table that is not NUL-terminated;
* make sure a terminator exists within the section before strdup() so
* it cannot read past the end of the mapped section.
*/
if (strnlen(base + index, size - index) == size - index) {
fprintf(stderr, "error: unterminated string in string table\n");
return -EINVAL;
}

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

@lyakh lyakh Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@lgirdwood lgirdwood Jun 12, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (!*str)
return -ENOMEM;

Expand Down
14 changes: 11 additions & 3 deletions tools/rimage/src/ext_manifest.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,12 @@ static int ext_man_validate(uint32_t section_size, const void *section_data)

/* copy each head to local struct to omit memory align issues */
while (offset < section_size) {
/* 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;
Comment on lines +72 to +76

@lgirdwood lgirdwood Jun 12, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
memcpy(&head, &sbuf[offset], sizeof(head));
fprintf(stdout, "Extended manifest found module, type: 0x%04X size: 0x%04X (%4d) offset: 0x%04X\n",
head.type, head.elem_size, head.elem_size, offset);
Expand Down Expand Up @@ -150,10 +156,12 @@ int ext_man_write(struct image *image)
/* validate metadata section */
ret = ext_man_validate(ext_man->full_size - ext_man->header_size,
(char *)ext_man + ext_man->header_size);
if (ret) {
ret = -errno;
if (ret)
/* ext_man_validate() already returns a negative errno; do not
* overwrite it with -errno, which is not set on validation
* failure and would mask the error
*/
goto out;
}

/* write extended metadata to file */
count = fwrite(ext_man, 1, ext_man->full_size, image->out_ext_man_fd);
Expand Down
5 changes: 4 additions & 1 deletion tools/rimage/src/hash.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,10 @@ int hash_single(const void *data, size_t size, const EVP_MD *algo, void *output,
if (algo_out_size <= 0)
return -EINVAL;

if (output_len > algo_out_size)
/* EVP_Digest writes algo_out_size bytes into output, so the buffer
* must be at least that large; reject an undersized output buffer
*/
if (output_len < (size_t)algo_out_size)
return -ENOBUFS;

@lyakh lyakh Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@lgirdwood lgirdwood Jun 12, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (!EVP_Digest(data, size, output, NULL, algo, NULL)) {
Expand Down
38 changes: 35 additions & 3 deletions tools/rimage/src/manifest.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,20 @@ static int man_module_create_reloc(struct image *image, struct manifest_module *
return -ENOEXEC;
}

/*
* n_mod comes from the (potentially untrusted) ELF and each manifest
* consumes a sof_man_module descriptor slot written into fw_image.
* Bound the cumulative count across all input modules (this ELF adds
* n_mod to the running output_mod_cfg_count) so a crafted .module
* section cannot overflow the fixed manifest descriptor array.
*/
if (modules->output_mod_cfg_count + n_mod > MAX_MODULES) {
fprintf(stderr, "error: too many module manifests (%u + %u > %u).\n",
modules->output_mod_cfg_count, n_mod, MAX_MODULES);
elf_section_free(&section);
return -ENOEXEC;
}

unsigned int i;

for (i = 0, sof_mod = section.data; i < n_mod; i++, sof_mod++) {
Expand Down Expand Up @@ -1664,21 +1678,39 @@ int verify_image(struct image *image)
ret = file_error("unable to read whole file", image->verify_file);
goto out;
}
for (i = 0; i < size; i += sizeof(uint32_t)) {
for (i = 0; i + sizeof(uint32_t) <= size; i += sizeof(uint32_t)) {
/* find CSE header marker "$CPD" */
if (*(uint32_t *)(buffer + i) == CSE_HEADER_MAKER) {
image->fw_image = buffer + i;
/* size of the image from the CSE header to the end of the
* file, used by v1.5 verification and the signed-payload
* bounds checks
*/
image->image_end = size - i;
ret = image->adsp->verify_firmware(image);
/* verify_firmware() returns 1 = valid, 0 = invalid and
* < 0 = error (OpenSSL RSA_verify semantics); normalize to
* 0 on success and a negative errno otherwise so the CLI
* exit code reflects the verification result
*/
if (ret == 1)
ret = 0;
else if (ret >= 0)
ret = -EINVAL;

@lyakh lyakh Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

goto out;
}
}

/* no header found */
fprintf(stderr, "error: could not find valid CSE header $CPD in %s\n",
image->verify_file);
ret = -EINVAL;
out:
fclose(in_file);
return 0;
/* propagate verification result so callers (and the exit code) can
* detect a failed/missing verification instead of always seeing success
*/
return ret;
}


Expand Down Expand Up @@ -1717,7 +1749,7 @@ int resign_image(struct image *image)

fclose(in_file);

for (i = 0; i < size; i += sizeof(uint32_t)) {
for (i = 0; i + sizeof(uint32_t) <= size; i += sizeof(uint32_t)) {
/* find CSE header marker "$CPD" */
if (*(uint32_t *)(buffer + i) == CSE_HEADER_MAKER) {
image->fw_image = buffer + i;
Expand Down
38 changes: 38 additions & 0 deletions tools/rimage/src/pkcs1_5.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -929,9 +929,28 @@ int ri_manifest_verify_v1_8(struct image *image)
MAN_RSA_SIGNATURE_LEN);

char *const data2 = (char *)man + MAN_SIG_PKG_OFFSET_V1_8;

/* css.size and css.header_len come from the (untrusted) image; reject
* a reversed ordering that would underflow the unsigned size2 below.
*/
if (man->css.size < man->css.header_len) {
fprintf(stderr, "error: invalid css size %u < header_len %u\n",
man->css.size, man->css.header_len);
return -EINVAL;
}

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

/* size2 is derived from untrusted css fields and is hashed starting at
* data2; make sure that range stays within the verified image buffer.
*/
if (image->image_end < MAN_SIG_PKG_OFFSET_V1_8 ||
size2 > image->image_end - MAN_SIG_PKG_OFFSET_V1_8) {
fprintf(stderr, "error: signed payload size 0x%x exceeds image\n", size2);
return -EINVAL;
}

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

Expand All @@ -946,9 +965,28 @@ int ri_manifest_verify_v2_5(struct image *image)
MAN_RSA_SIGNATURE_LEN_2_5);

char *const data2 = (char *)man + MAN_SIG_PKG_OFFSET_V2_5;

/* css.size and css.header_len come from the (untrusted) image; reject
* a reversed ordering that would underflow the unsigned size2 below.
*/
if (man->css.size < man->css.header_len) {
fprintf(stderr, "error: invalid css size %u < header_len %u\n",
man->css.size, man->css.header_len);
return -EINVAL;
}

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

/* size2 is derived from untrusted css fields and is hashed starting at
* data2; make sure that range stays within the verified image buffer.
*/
if (image->image_end < MAN_SIG_PKG_OFFSET_V2_5 ||
size2 > image->image_end - MAN_SIG_PKG_OFFSET_V2_5) {
fprintf(stderr, "error: signed payload size 0x%x exceeds image\n", size2);
return -EINVAL;
}

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

Expand Down
Loading