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

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

[Jerome Forissier]

Hi,

I am trying to enable FIT image verification by SPL on Rockchip 4B (U-Boot
v2022.04) and I hit a couple of issues.

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, 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.

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].

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?

[1] https://www.rockchip.fr/Rockchip%20RK3399%20TRM%20V1.4%20Part1.pdf


Thanks,
-- 
Jerome

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

[Xavier Drudis Ferran]

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'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.

When I didn't use that patch, my SPL hanged after "Trying to boot from MMC1", before 
loading the ATF2 image, just when loading ATF1. But maybe depening on what's in 
that area, or the particular layout resulting from a particular build, 
the effect might be different. It might be a little random.

> 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 ? Why can one only use the
first 8K from the 64K INTMEM1 ? Did you find any reason the rest
shouldn't be writable?

Could corrupted memory explain what you're seeing ?

Re: [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

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

[Xavier Drudis Ferran]

Sorry for the noise. 

El Fri, Jun 03, 2022 at 04:58:38PM +0200, Xavier Drudis Ferran deia:
> 
> 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?
>

It was just that I didn't  notice the problem. 

> 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
>

It doesn't matter, it seems it's just me who gets ATF1 at address 0.
  
> It's that the change shouldn't be needed ? Why can one only use the
> first 8K from the 64K INTMEM1 ? Did you find any reason the rest
> shouldn't be writable?
>

Because INTMEM1 is only 8K long in RK3399. 64K is just the length 
of the addressing page it's in, but the SRAM is just 8Kb in size.
INTMEM0 is 192 Kb, but not INTMEM1