diff mbox series

[v2,12/23] arm: imx: hab: Print CSF based on IVT descriptor

Message ID 1514486982-19059-13-git-send-email-bryan.odonoghue@linaro.org
State Superseded
Headers show
Series Fix and extend i.MX HAB layer | expand

Commit Message

Bryan O'Donoghue Dec. 28, 2017, 6:49 p.m. UTC
The IVT gives the absolute address of the CSF. There is no requirement for
the CSF to be located adjacent to the IVT so lets use the address provided
in the IVT header instead of the fixed CSF offset currently in place.

Its worth noting if you use u-boot mkimage and the i.MX CST tool as
described in the NXP documentation you will get an image like

IVT | BINARY | CSF not IVT | CSF | BINARY as the code currently assumes.
The IVT header must correctly describe the location of the CSF or the
BootROM will reject it - so the current dependence on a fixed offset is
nothing except limiting.

Fix it now.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Albert Aribaud <albert.u.boot@aribaud.net>
Cc: Sven Ebenfeld <sven.ebenfeld@gmail.com>
Cc: George McCollister <george.mccollister@gmail.com>
Cc: Breno Matheus Lima <brenomatheus@gmail.com>
---
 arch/arm/mach-imx/hab.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Breno Matheus Lima Dec. 29, 2017, 4:42 p.m. UTC | #1
Hi Bryan,

2017-12-28 16:49 GMT-02:00 Bryan O'Donoghue <bryan.odonoghue@linaro.org>:
> The IVT gives the absolute address of the CSF. There is no requirement for
> the CSF to be located adjacent to the IVT so lets use the address provided
> in the IVT header instead of the fixed CSF offset currently in place.
>
> Its worth noting if you use u-boot mkimage and the i.MX CST tool as
> described in the NXP documentation you will get an image like
>
> IVT | BINARY | CSF not IVT | CSF | BINARY as the code currently assumes.

Your patch looks fine, just a comment here.

The hab_rvt_authenticate_image() is usually executed for extending the
root of trust beyond the initial boot image (zImage, u-boot-ivt.img),
in my understanding the layout described on the NXP documentation " |
IVT | BINARY | CSF | " just applies for the initial boot images.

For additional boot images the expected layout is currently documented
in "arch/arm/mach-imx/hab.c" | BINARY |  IVT  |  CSF |

Maybe this sentence can be reformulated for better understanding.

Thanks,
Breno Lima
Bryan O'Donoghue Dec. 29, 2017, 5:30 p.m. UTC | #2
On 29/12/17 16:42, Breno Matheus Lima wrote:
> The hab_rvt_authenticate_image() is usually executed for extending the
> root of trust beyond the initial boot image (zImage, u-boot-ivt.img),
> in my understanding the layout described on the NXP documentation " |
> IVT | BINARY | CSF | " just applies for the initial boot images.
> 
> For additional boot images the expected layout is currently documented
> in "arch/arm/mach-imx/hab.c" | BINARY |  IVT  |  CSF |
> 
> Maybe this sentence can be reformulated for better understanding.

I suppose the core point to get across is that we current depend on 
fixed offsets and this is prohibitively difficult.

For example on the secure-boot stuff I'm working on, all of our images 
are IVT | BINARY | CSF - because this is the required format for the 
BootROM to load u-boot, it's just easier/more-logical to produce all of 
our binaries kernel, dtb, boot.scr, etc in that format.

But, yes, I'll re-word this to scan a bit better.

Thanks for taking the time to test this out.

---
bod
diff mbox series

Patch

diff --git a/arch/arm/mach-imx/hab.c b/arch/arm/mach-imx/hab.c
index 479ed96..d620524 100644
--- a/arch/arm/mach-imx/hab.c
+++ b/arch/arm/mach-imx/hab.c
@@ -465,8 +465,7 @@  int authenticate_image(uint32_t ddr_start, uint32_t image_size,
 	print_buffer(ivt_addr, (void *)(ivt_addr), 4, 0x8, 0);
 
 	puts("Dumping CSF Header\n");
-	print_buffer(ivt_addr + IVT_SIZE, (void *)(ivt_addr + IVT_SIZE), 4,
-		     0x10, 0);
+	print_buffer(ivt->csf, (void *)(ivt->csf), 4, 0x10, 0);
 
 #if  !defined(CONFIG_SPL_BUILD)
 	get_hab_status();