intel_adsp_cavs25: output some unrecognized ASCII char when using rimage to sign a binary

chen-png commented 2 years ago

Describe the bug

for intel_adsp_cavs25 platform, when using rimage to sign the binary, it reported some unrecognized ASCII char when parsing cse entry name "cavs0015.met", the output as below, there are some unrecognized char after "cavs0015.met". please ignore data of entry.offset and length, I removed them due to private policy.

 partition_name: 'ADSP'
         header_version: 2
          entry_version: 1
             nb_entries: 3
             entry.name: 'ADSP.man'
           entry.offset: 
           entry.length:
             entry.name: 'cavs0015.met▒'
           entry.offset: 
           entry.length: 
             entry.name: 'cavs0015'
           entry.offset: 
           entry.length:

this will causing an error "UnicodeDecodeError: 'utf-8' codec can't decode byte 0xc0 in position 899: invalid start byte" when we want to decode the output of rimage command. the raw data captured at position 899 as below, cannot decode \xc0\x04 using utf-8.

entry.name: 'cavs0015.met\xc0\x04'\n

Debug Info I found that in tgl.toml file, there is a cse entry as below, the strlen(name) is 12 bytes,

[[cse.entry]]
name = "cavs0015.met"
offset = 
length =

but the cse entry definition in rimage source code is as below, the entry name is uint8 [12], it includes the empty char for string. so when rimage parsing entry name "cavs0015.met", it just overlay the empty char, then when we output this entry_name, it will output some other chars until meeting another empty char. so when we define a cse entry name, the max strlen(name) should be 11 chars, not 12 chars.

struct CsePartitionDirEntry {
        uint8_t  entry_name[12];
        uint32_t offset;
        uint32_t length;
        uint32_t reserved;
}  __attribute__((packed));

Suggestions Could we increase the bytes of cse entry name in rimage source code?

Another minimal issue this seems like a mistype.

diff --git a/src/adsp_config.c b/src/adsp_config.c
index 2e6f39f..5d5aa5a 100644
--- a/src/adsp_config.c
+++ b/src/adsp_config.c
@@ -627,7 +627,7 @@ static int parse_cse_v2_5(const toml_table_t *toml, struct parse_ctx *pctx,

        hdr->nb_entries = toml_array_nelem(cse_entry_array);

-       if (1 || verbose)
+       if (verbose)
                dump_cse_v2_5(hdr, out);

andyross commented 2 years ago

I don't think we can change the values in that header, it's specified by a protocol somewhere and there are multiple tools (including, I believe, firmware in the CSME) that parse it. The correct fix is for rimage to simply not overrun the buffer then printing it; it's not a null-terminated string.

FWIW, for context: this glitch is the proximate cause of some Zephyr log parsing utilities blowing up due a utf-8 decode error in python. Obviously our tools should be more robust, but rimage still shouldn't be writing random bytes to stdout.

chen-png commented 2 years ago

the output string "entry.name: 'cavs0015.met▒'" which blocked our tools, actually it's output from "dump_cse_v2_5(hdr, out);", not sure whether this debug information is always output, seems like a mistype, but if without those debug output, then it won't block.

static int parse_cse_v2_5(const toml_table_t *toml, struct parse_ctx *pctx,

       hdr->nb_entries = toml_array_nelem(cse_entry_array);

       if (1 || verbose)

                dump_cse_v2_5(hdr, out);