Re: [PATCH RFC u-boot-mvebu 19/59] tools: kwboot: Show image type and error parsing reasons

Tony Dinh
2023-02-22T05:51:32Z

On Tue, Feb 21, 2023 at 12:22 PM Pali Rohár <pali@kernel.org> wrote:
>
> Show image type and version during parsing of kwbimage.
> And show reasons in error messages when parsing failed.
> This can help to debug issues with invalid images.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
> tools/kwboot.c | 39 ++++++++++++++++++++++++++++++++++-----
> 1 file changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/tools/kwboot.c b/tools/kwboot.c
> index cb31d5b858ce..7c666486f31f 100644
> --- a/tools/kwboot.c
> +++ b/tools/kwboot.c
> @@ -1976,6 +1976,21 @@ _inject_baudrate_change_code(void *img, size_t *size, int for_data,
> }
> }
>
> +static const char *
> +kwboot_img_type(uint8_t blockid)
> +{
> + switch (blockid) {
> + case IBR_HDR_I2C_ID: return "I2C";
> + case IBR_HDR_SPI_ID: return "SPI";
> + case IBR_HDR_NAND_ID: return "NAND";
> + case IBR_HDR_SATA_ID: return "SATA";
> + case IBR_HDR_PEX_ID: return "PEX";
> + case IBR_HDR_UART_ID: return "UART";
> + case IBR_HDR_SDIO_ID: return "SDIO";
> + default: return "unknown";
> + }
> +}
> +
> static int
> kwboot_img_patch(void *img, size_t *size, int baudrate)
> {
> @@ -1989,8 +2004,10 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
>
> hdr = img;
>
> - if (*size < sizeof(struct main_hdr_v1))
> + if (*size < sizeof(struct main_hdr_v1)) {
> + fprintf(stderr, "Invalid image header size\n");
> goto err;
> + }
>
> image_ver = kwbimage_version(img);
> if (image_ver != 0 && image_ver != 1) {
> @@ -2000,12 +2017,18 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
>
> hdrsz = kwbheader_size(hdr);
>
> - if (*size < hdrsz)
> + if (*size < hdrsz) {
> + fprintf(stderr, "Invalid image header size\n");
> goto err;
> + }
> +
> + kwboot_printv("Detected kwbimage v%d with %s boot signature\n", image_ver, kwboot_img_type(hdr->blockid));
>
> csum = kwboot_hdr_csum8(hdr) - hdr->checksum;
> - if (csum != hdr->checksum)
> + if (csum != hdr->checksum) {
> + fprintf(stderr, "Image has invalid header checksum stored in image header\n");
> goto err;
> + }
>
> srcaddr = le32_to_cpu(hdr->srcaddr);
>
> @@ -2028,9 +2051,15 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
> break;
> }
>
> - if (hdrsz > le32_to_cpu(hdr->srcaddr) ||
> - *size < le32_to_cpu(hdr->srcaddr) + le32_to_cpu(hdr->blocksize))
> + if (hdrsz > le32_to_cpu(hdr->srcaddr)) {
> + fprintf(stderr, "Image has invalid data offset stored in image header\n");
> + goto err;
> + }
> +
> + if (*size < le32_to_cpu(hdr->srcaddr) + le32_to_cpu(hdr->blocksize)) {
> + fprintf(stderr, "Image has invalid data size stored in image header\n");
> goto err;
> + }
>
> for_each_opt_hdr_v1 (ohdr, hdr) {
> if (!opt_hdr_v1_valid_size(ohdr, (const uint8_t *)hdr + hdrsz)) {
> --
> 2.20.1
>
Nominal cases (no error) with Armada 385 and KIrkwood 6192:

Detected kwbimage v1 with SPI boot signature
Detected kwbimage v0 with NAND boot signature

Tested-by: Tony Dinh <mibodhi@gmail.com>

All the best,
Tony