On 3/23/23 20:57, Pali Rohár wrote:

Armada 385 BootROM ignores low 7 bits of headersz when parsing kwbimage header of UART type, which effectively means that headersz is rounded down to multiply of 128 bytes. For all other image types BootROM reads and use all bits of headersz. Therefore fill into UART type of kwbimage v1 headersz aligned to 128 bytes.

Fixes: 2b0980c24027 ("tools: kwbimage: Fill the real header size into the main header") Signed-off-by: Pali Rohár pali@kernel.org

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

tools/kwbimage.c | 10 ++++++++++ 1 file changed, 10 insertions(+)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c index 309657a5637b..177084adf825 100644 --- a/tools/kwbimage.c +++ b/tools/kwbimage.c @@ -1231,6 +1231,16 @@ static size_t image_headersz_v1(int *hasext) if (count > 0) headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;

• /*

• * For all images except UART, headersz stored in header itself should
  

• * contains header size without padding. For UART image BootROM rounds
  

• * down headersz to multiply of 128 bytes. Therefore align UART headersz
  

• * to multiply of 128 bytes to ensure that remaining UART header bytes
  

• * are not ignored by BootROM.
  

• */
  

• if (image_get_bootfrom() == IBR_HDR_UART_ID)

• headersz = ALIGN(headersz, 128);
  

• return headersz; }

Viele Grüße, Stefan Roese

On 3/23/23 20:57, Pali Rohár wrote:

Ensure that UART aligned header size is always stored into kwbimage v1 header. It is needed for proper UART booting. Calculation of headersz field was broken in commit d656f5a0ee22 ("tools: kwboot: Calculate real used space in kwbimage header when calling kwboot_img_grow_hdr()") which introduced optimization of kwboot_img_grow_hdr() function.

Fixes: d656f5a0ee22 ("tools: kwboot: Calculate real used space in kwbimage header when calling kwboot_img_grow_hdr()") Signed-off-by: Pali Rohár pali@kernel.org

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

tools/kwboot.c | 11 +++++++++++ 1 file changed, 11 insertions(+)

diff --git a/tools/kwboot.c b/tools/kwboot.c index c131711444ec..dd894e80db1c 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -2171,6 +2171,17 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)

kwboot_printv("Aligning image header to Xmodem block size\n");
kwboot_img_grow_hdr(img, size, grow);

• hdrsz += grow;
  

• /*
  

•  * kwbimage v1 contains header size field and for UART type it
  

•  * must be set to the aligned xmodem header size because BootROM
  

•  * rounds header size down to xmodem block size.
  

•  */
  

• if (kwbimage_version(img) == 1) {
  

• 	hdr->headersz_msb = hdrsz >> 16;
  

• 	hdr->headersz_lsb = cpu_to_le16(hdrsz & 0xffff);
  

• }
  

  }

  hdr->checksum = kwboot_hdr_csum8(hdr) - csum;

Viele Grüße, Stefan Roese

On 3/23/23 20:57, Pali Rohár wrote:

Commit 7665ed2fa04e ("tools: kwboot: Fix parsing UART image without data checksum") added fixup code to insert place for data checksum if UART image does not have it. Together with option -B (change baudrate), kwboot calculates this checksum. Without option -B, it inserts only place for checksum but does not calculate it.

This commit fix above logic and calculate data checksum also when kwboot is used without -B option.

Fixes: 7665ed2fa04e ("tools: kwboot: Fix parsing UART image without data checksum") Signed-off-by: Pali Rohár pali@kernel.org

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

tools/kwboot.c | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/tools/kwboot.c b/tools/kwboot.c index dd894e80db1c..23a893a9b9f8 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -2082,6 +2082,8 @@ kwboot_img_patch(void *img, size_t *size, int baudrate) goto err; } kwboot_img_grow_data_right(img, size, sizeof(uint32_t));

• /* Update the 32-bit data checksum */
  

• *kwboot_img_csum32_ptr(img) = kwboot_img_csum32(img);
  

  }

  if (!kwboot_img_has_ddr_init(img) &&

Viele Grüße, Stefan Roese

On 3/23/23 20:57, Pali Rohár wrote:

Sending of very small images (smaller than 128 bytes = xmodem block size) cause out-of-bound memory read access. Fix this issue by ensuring that hdrsz when sending image is not larger than total size of the image. Issue was introduced in commit f8017c37799c ("tools: kwboot: Fix sending Kirkwood v0 images"). Special case when total image is smaller than header size aligned to multiply of xmodem size is already handled since that commit.

Fixes: f8017c37799c ("tools: kwboot: Fix sending Kirkwood v0 images") Signed-off-by: Pali Rohár pali@kernel.org

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

tools/kwboot.c | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/tools/kwboot.c b/tools/kwboot.c index 23a893a9b9f8..1cf78dda6755 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -1458,6 +1458,8 @@ kwboot_xmodem(int tty, const void *_img, size_t size, int baudrate) * followed by the header. So align header size to xmodem block size. */ hdrsz += (KWBOOT_XM_BLKSZ - hdrsz % KWBOOT_XM_BLKSZ) % KWBOOT_XM_BLKSZ;

• if (hdrsz > size)

• hdrsz = size;
  
  

  pnum = 1;

Viele Grüße, Stefan Roese

On 3/23/23 20:57, Pali Rohár wrote:

A38x BootROM has a bug which cause that BootROM loads data part of UART image into RAM target address increased by one byte when source address and header size stored in the image header are not same.

Workaround this bug by completely removing a gap between header and data part of the UART image. Without gap, this BootROM bug is not triggered.

This gap can be present in SDIO or SATA image types which have aligned start of the data part to the media sector size. With this workaround kwboot should be able to convert and send SDIO or SATA images for UART booting.

Signed-off-by: Pali Rohár pali@kernel.org

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

tools/kwboot.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)

diff --git a/tools/kwboot.c b/tools/kwboot.c index 1cf78dda6755..2b92966919da 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -78,6 +78,17 @@

• • IBR_HDR_UART_ID (0x69):
• UART image can be transfered via xmodem protocol over first UART.

• • Unlike all other image types, header size stored in the image must be
• • multiply of the 128 bytes (for all other image types it can be any size)
• • and data part of the image does not have to contain 32-bit checksum
• • (all other image types must have valid 32-bit checksum in its data part).
• • And data size stored in the image is ignored. A38x BootROM determinates
• • size of the data part implicitly by the end of the xmodem transfer.
• • A38x BootROM has a bug which cause that BootROM loads data part of UART
• • image into RAM target address increased by one byte when source address
• • and header size stored in the image header are not same. So UART image
• • should be constructed in a way that there is no gap between header and
• • data part.
  • • IBR_HDR_I2C_ID (0x4D):
  • It is unknown for what kind of storage is used this image. It is not

@@ -2188,6 +2199,18 @@ kwboot_img_patch(void *img, size_t *size, int baudrate) } }

• /* Header size and source address must be same for UART type due to A38x BootROM bug */

• if (hdrsz != le32_to_cpu(hdr->srcaddr)) {

• if (is_secure) {
  

• 	fprintf(stderr, "Cannot align image with secure header\n");
  

• 	goto err;
  

• }
  

• kwboot_printv("Removing gap between image header and data\n");
  

• memmove(img + hdrsz, img + le32_to_cpu(hdr->srcaddr), le32_to_cpu(hdr->blocksize));
  

• hdr->srcaddr = cpu_to_le32(hdrsz);
  

• }

• hdr->checksum = kwboot_hdr_csum8(hdr) - csum;

  *size = le32_to_cpu(hdr->srcaddr) + le32_to_cpu(hdr->blocksize);

Viele Grüße, Stefan Roese