[SPAM] RockPi4B: spl_load_fit_image() writing past end of buffer & MMC errors

[Jerome Forissier]

Hi Xavier,

On 6/3/22 16:58, Xavier Drudis Ferran wrote:
> El Fri, Jun 03, 2022 at 03:17:05PM +0200, Jerome Forissier deia:
>> First, I noticed that spl_load_fit_image() may write memory outside the range
>> given in the image node. For example on RockPi 4B I have the following FIT
>> (irrelevant parts omitted):
>>
>> / {
>> 	images {
>> 		atf_2 {
>> 			/* File size is 8024 bytes */
>> 			data = /incbin/("bl31_0xff3b0000.bin");
>> 			compression = "none";
>> 			load = <0xff3b0000>
>> 		}
>> 	}
>> }
>>
>> I expect SPL to load atf_2 into 0xff3b0000 - 0xff3b200b but due to the alignment
>> of the source data in the MMC, spl_load_fit_image() writes one more block and
>> later moves the data in place with memcpy(). What are the guarantees that it
>> is a safe thing to do?
>>
> 
> In my case bl31_0xff3b0000.bin is 8020 bytes in size. 
>  
>> In my case, the image starts at offset 308 in the 512-byte MMC block (that
>> offset is called 'overhead' in spl_load_fit_image()). As a result,
>> (8204 + 308) / 512 + 1 = 17 blocks = 8704 bytes are read.
> 
> My overhead is 352, it's reading 17 blocks too.
> 
>>
>> For some reason I can't explain, on my board only the first 8K of the 64K SRAM
>> range 0xff3b0000 - 0xff3c0000 (INTMEM1) can be safely written to. Any data
>> written after this point corrupt the lower 8K. I found nothing in the rk3399
>> TRM about this [1].
>>
> 
> In my Rock Pi 4B this does not happen. It happily writes the 17 blocks. 
> Or it's just that I'm not noticing the problem ? Does it hang or give you 
> an error, or do you just find it out by reading the SRAM?

I found out when trying to use signed FIT images. The board would boot fine
otherwise.

I am setting CONFIG_SPL_FIT_SIGNATURE=y and I have modified the script
arch/arm/mach-rockchip/make_fit_atf.py so that it generates the required
hash and signature nodes in u-boot.its. Then I am basically following the
instructions in doc/uImage.FIT/signature.txt -- generating a RSA key pair
with OpenSSL and using 'mkimage -k keys -K -r $(other-args...)'.

Please refer to the patches at:

https://git.codelinaro.org/linaro/dependable-boot/meta-ts/-/merge_requests/7/diffs

It is work in progress really but it should give you an idea of what I am doing,
and possibly help reproduce the issue.

I also confirmed the issue with a small test case. Could you please try the
following patch?

---8<-----------------------------------------------------------------------
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index f6ccd837aa..810d5fa6db 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -401,6 +401,12 @@ static int mmc_read_blocks(struct mmc *mmc, void *dst, lbaint_t start,
 	struct mmc_cmd cmd;
 	struct mmc_data data;
 
+	printf("== 0x%08x\n", *(u32 *)0xff3b0100);
+	*(u32 *)0xff3b0100 = 0;
+	printf("== 0x%08x\n", *(u32 *)0xff3b0100);
+	*(u32 *)0xff3b2100 = 0xdeadbeef;
+	printf("== 0x%08x\n", *(u32 *)0xff3b0100);
+
 	if (blkcnt > 1)
 		cmd.cmdidx = MMC_CMD_READ_MULTIPLE_BLOCK;
 	else
---8<-----------------------------------------------------------------------

Guess what, this **does** print "0xdeadbeef" on the console on my board! :O

> 
> I'm using the patch I sent yesterday: 
> 
> [PATCH 0/3] arm: rockchip: rk3399: rock-pi-4: power domain driver to boot from MMCSD 
> https://lists.denx.de/pipermail/u-boot/2022-June/485498.html
> 
> (instead of the patch you can simply configure
> CONFIG_ROCKCHIP_SPL_RESERVE_IRAM from make nconfig or so) 
> 
> Which reserves 0x0 - 0x14fff bytes in DDRAM for bl31. I found that
> otherwise, SPL memory in that area gets overwritten by
> bl31_0x00000000.bin.  I don't remember exactly what was there, but in
> my tests, bl31_0x00000000 gets loaded before
> bl31_0xff3b0000.bin. Depending on what data or code is overwritten,
> SPL might be doing something funny.

I don't have bl31_0x00000000.bin. The bl31*.bin files come from bl31.elf
which is split by arch/arm/mach-rockchip/make_fit_atf.py.
It could be that we are using different versions of TF-A or different build
parameters, but that seems unrelated I think.
 
>> Anyways, I tried using a temporary buffer allocated on the heap to handle
>> the first and last blocks at least (the load address is properly aligned
>> for info->read() so the middle blocks can be read in one go). It works but
>> not reliably. And that is the second problem. mmc_read_blocks() in mmc_bread()
>> sometimes returns an error. If I read blocks one by one in a loop THE read
>> randomly fails after a few blocks only. The error is -110 (-ETIMEDOUT) from
>> dwmci_send_cmd().
>>
>> Am I using the MMC read incorrectly?
>>
>  
> It's not that you're using the read incorrectly. 
> 
> It's that the change shouldn't be needed ?

Ideally, yes, but it doesn't seem right to overflow the specified range. Who
knows what might be immediately after the end of the buffer for a given image?
Perhaps another image has been written there already, or the image might be
right before a reserved address range such is some mapped I/O area?

> Why can one only use the
> first 8K from the 64K INTMEM1 ? Did you find any reason the rest
> shouldn't be writable?

No, I don't understand. As I said the rest seems writable (at least up to some
point) but writing to it corrupts the memory below...

> Could corrupted memory explain what you're seeing ?

I found no evidence of corruption until SPL writes to 0xff3b2000 and above.

Thanks,
-- 
Jerome