[U-Boot] [PATCH v2] imx: hab: extend hab_auth_img to calculate ivt_offset

[U-Boot] [PATCH v2] imx: hab: extend hab_auth_img to calculate ivt_offset

[Parthiban Nallathambi]

Current implementation of hab_auth_img command needs ivt_offset to
authenticate the image. But ivt header is placed at the end of image
date after padding.

This leaves the usage of hab_auth_img command to fixed size or static
offset for ivt header. New function "get_image_ivt_offset" is introduced
to find the ivt offset during runtime. The case conditional check in this
function is same as boot_get_kernel in common/bootm.c

With this variable length image e.g. FIT image with any random size can
have IVT at the end and ivt_offset option can be left optional

Can be used as "hab_auth_img $loadaddr $filesize" from u-boot script

Signed-off-by: Parthiban Nallathambi <pn@denx.de>
---

Notes:
    Changelog in v2:
    - Finding IVT offset doesn't need length. Removed the
    length argument from get_image_ivt_offset

 arch/arm/mach-imx/hab.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-imx/hab.c b/arch/arm/mach-imx/hab.c
index b88acd13da..dbfd692fa3 100644
--- a/arch/arm/mach-imx/hab.c
+++ b/arch/arm/mach-imx/hab.c
@@ -6,6 +6,8 @@
 #include <common.h>
 #include <config.h>
 #include <fuse.h>
+#include <mapmem.h>
+#include <image.h>
 #include <asm/io.h>
 #include <asm/system.h>
 #include <asm/arch/clock.h>
@@ -302,18 +304,41 @@ static int do_hab_status(cmd_tbl_t *cmdtp, int flag, int argc,
 	return 0;
 }
 
+static ulong get_image_ivt_offset(ulong img_addr)
+{
+	const void *buf;
+
+	buf = map_sysmem(img_addr, 0);
+	switch (genimg_get_format(buf)) {
+#if defined(CONFIG_IMAGE_FORMAT_LEGACY)
+	case IMAGE_FORMAT_LEGACY:
+		return (image_get_image_size((image_header_t *)img_addr)
+			+ 0x1000 - 1)  & ~(0x1000 - 1);
+#endif
+#if IMAGE_ENABLE_FIT
+	case IMAGE_FORMAT_FIT:
+		return (fit_get_size(buf) + 0x1000 - 1)  & ~(0x1000 - 1);
+#endif
+	default:
+		return 0;
+	}
+}
+
 static int do_authenticate_image(cmd_tbl_t *cmdtp, int flag, int argc,
 				 char * const argv[])
 {
 	ulong	addr, length, ivt_offset;
 	int	rcode = 0;
 
-	if (argc < 4)
+	if (argc < 3)
 		return CMD_RET_USAGE;
 
 	addr = simple_strtoul(argv[1], NULL, 16);
 	length = simple_strtoul(argv[2], NULL, 16);
-	ivt_offset = simple_strtoul(argv[3], NULL, 16);
+	if (argc == 3)
+		ivt_offset = get_image_ivt_offset(addr);
+	else
+		ivt_offset = simple_strtoul(argv[3], NULL, 16);
 
 	rcode = imx_hab_authenticate_image(addr, length, ivt_offset);
 	if (rcode == 0)
-- 
2.17.2

[U-Boot] [PATCH v2] imx: hab: extend hab_auth_img to calculate ivt_offset

[Breno Matheus Lima]

Hi Parthiban,

Em qua, 21 de nov de 2018 às 11:50, Parthiban Nallathambi <pn@denx.de> escreveu:
>
> Current implementation of hab_auth_img command needs ivt_offset to
> authenticate the image. But ivt header is placed at the end of image
> date after padding.
>
> This leaves the usage of hab_auth_img command to fixed size or static
> offset for ivt header. New function "get_image_ivt_offset" is introduced
> to find the ivt offset during runtime. The case conditional check in this
> function is same as boot_get_kernel in common/bootm.c
>
> With this variable length image e.g. FIT image with any random size can
> have IVT at the end and ivt_offset option can be left optional
>
> Can be used as "hab_auth_img $loadaddr $filesize" from u-boot script
>
> Signed-off-by: Parthiban Nallathambi <pn@denx.de>
> ---
>
> Notes:
>     Changelog in v2:
>     - Finding IVT offset doesn't need length. Removed the
>     length argument from get_image_ivt_offset
>
>  arch/arm/mach-imx/hab.c | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-imx/hab.c b/arch/arm/mach-imx/hab.c
> index b88acd13da..dbfd692fa3 100644
> --- a/arch/arm/mach-imx/hab.c
> +++ b/arch/arm/mach-imx/hab.c
> @@ -6,6 +6,8 @@
>  #include <common.h>
>  #include <config.h>
>  #include <fuse.h>
> +#include <mapmem.h>
> +#include <image.h>
>  #include <asm/io.h>
>  #include <asm/system.h>
>  #include <asm/arch/clock.h>
> @@ -302,18 +304,41 @@ static int do_hab_status(cmd_tbl_t *cmdtp, int flag, int argc,
>         return 0;
>  }
>
> +static ulong get_image_ivt_offset(ulong img_addr)
> +{
> +       const void *buf;
> +
> +       buf = map_sysmem(img_addr, 0);
> +       switch (genimg_get_format(buf)) {
> +#if defined(CONFIG_IMAGE_FORMAT_LEGACY)
> +       case IMAGE_FORMAT_LEGACY:
> +               return (image_get_image_size((image_header_t *)img_addr)
> +                       + 0x1000 - 1)  & ~(0x1000 - 1);
> +#endif
> +#if IMAGE_ENABLE_FIT
> +       case IMAGE_FORMAT_FIT:
> +               return (fit_get_size(buf) + 0x1000 - 1)  & ~(0x1000 - 1);
> +#endif
> +       default:
> +               return 0;
> +       }
> +}


Do you have more details about the image header I should use here?

I'm trying to get the IVT offset for my Kernel image based on NXP
4.9.11_2.0.0_GA Linux release loaded at 0x80800000:

=> fatload mmc 0:1 0x80800000 zImage
=> hab_auth_img 0x80800000 <Len not being used>

The zImage size in Header looks correct (0x00726690), but
get_image_ivt_offset() is returning 0x0

$ hexdump zImage | head
0000000 0000 e1a0 0000 e1a0 0000 e1a0 0000 e1a0
*
0000020 0003 ea00 2818 016f 0000 0000 6690 0072
0000030 0201 0403 9000 e10f 04f8 eb00 7001 e1a0
0000040 8002 e1a0 2000 e10f 0003 e312 0001 1a00

Seems that genimg_get_format() is returning 0x0.

Any ideias if I'm missing something?

> +
>  static int do_authenticate_image(cmd_tbl_t *cmdtp, int flag, int argc,
>                                  char * const argv[])
>  {
>         ulong   addr, length, ivt_offset;
>         int     rcode = 0;
>
> -       if (argc < 4)
> +       if (argc < 3)

I think we can also change here to argc < 2, the function
get_image_ivt_offset() only requires the img addr now.

>                 return CMD_RET_USAGE;
>
>         addr = simple_strtoul(argv[1], NULL, 16);
>         length = simple_strtoul(argv[2], NULL, 16);
> -       ivt_offset = simple_strtoul(argv[3], NULL, 16);
> +       if (argc == 3)

argc ==2

[U-Boot] [PATCH v2] imx: hab: extend hab_auth_img to calculate ivt_offset

[Parthiban Nallathambi]

Hi Breno,

On 11/21/18 5:45 PM, Breno Matheus Lima wrote:
> Hi Parthiban,
> 
> Em qua, 21 de nov de 2018 às 11:50, Parthiban Nallathambi <pn@denx.de> escreveu:
>>
>> Current implementation of hab_auth_img command needs ivt_offset to
>> authenticate the image. But ivt header is placed at the end of image
>> date after padding.
>>
>> This leaves the usage of hab_auth_img command to fixed size or static
>> offset for ivt header. New function "get_image_ivt_offset" is introduced
>> to find the ivt offset during runtime. The case conditional check in this
>> function is same as boot_get_kernel in common/bootm.c
>>
>> With this variable length image e.g. FIT image with any random size can
>> have IVT at the end and ivt_offset option can be left optional
>>
>> Can be used as "hab_auth_img $loadaddr $filesize" from u-boot script
>>
>> Signed-off-by: Parthiban Nallathambi <pn@denx.de>
>> ---
>>
>> Notes:
>>      Changelog in v2:
>>      - Finding IVT offset doesn't need length. Removed the
>>      length argument from get_image_ivt_offset
>>
>>   arch/arm/mach-imx/hab.c | 29 +++++++++++++++++++++++++++--
>>   1 file changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mach-imx/hab.c b/arch/arm/mach-imx/hab.c
>> index b88acd13da..dbfd692fa3 100644
>> --- a/arch/arm/mach-imx/hab.c
>> +++ b/arch/arm/mach-imx/hab.c
>> @@ -6,6 +6,8 @@
>>   #include <common.h>
>>   #include <config.h>
>>   #include <fuse.h>
>> +#include <mapmem.h>
>> +#include <image.h>
>>   #include <asm/io.h>
>>   #include <asm/system.h>
>>   #include <asm/arch/clock.h>
>> @@ -302,18 +304,41 @@ static int do_hab_status(cmd_tbl_t *cmdtp, int flag, int argc,
>>          return 0;
>>   }
>>
>> +static ulong get_image_ivt_offset(ulong img_addr)
>> +{
>> +       const void *buf;
>> +
>> +       buf = map_sysmem(img_addr, 0);
>> +       switch (genimg_get_format(buf)) {
>> +#if defined(CONFIG_IMAGE_FORMAT_LEGACY)
>> +       case IMAGE_FORMAT_LEGACY:
>> +               return (image_get_image_size((image_header_t *)img_addr)
>> +                       + 0x1000 - 1)  & ~(0x1000 - 1);
>> +#endif
>> +#if IMAGE_ENABLE_FIT
>> +       case IMAGE_FORMAT_FIT:
>> +               return (fit_get_size(buf) + 0x1000 - 1)  & ~(0x1000 - 1);
>> +#endif
>> +       default:
>> +               return 0;
>> +       }
>> +}
> 
> 
> Do you have more details about the image header I should use here?

Is the image signed using CST or similar tool? Is so, the signature data
(HAB data: CSF, Certs and signature) pads at the end of the kernel
image.

> 
> I'm trying to get the IVT offset for my Kernel image based on NXP
> 4.9.11_2.0.0_GA Linux release loaded at 0x80800000:
> 
> => fatload mmc 0:1 0x80800000 zImage
> => hab_auth_img 0x80800000 <Len not being used>

Length for hab_auth_img is still mandatory. Length hear means the file
size or total length of the image, this is required for the HAB API to
authenticate (HAB_RVT_AUTHENTICATE_IMAGE).

> 
> The zImage size in Header looks correct (0x00726690), but
> get_image_ivt_offset() is returning 0x0

Looks like IVT offset is not found in the image.

> 
> $ hexdump zImage | head
> 0000000 0000 e1a0 0000 e1a0 0000 e1a0 0000 e1a0
> *
> 0000020 0003 ea00 2818 016f 0000 0000 6690 0072
> 0000030 0201 0403 9000 e10f 04f8 eb00 7001 e1a0
> 0000040 8002 e1a0 2000 e10f 0003 e312 0001 1a00
> 
> Seems that genimg_get_format() is returning 0x0.

Image is not signed?

> 
> Any ideias if I'm missing something?
> 
>> +
>>   static int do_authenticate_image(cmd_tbl_t *cmdtp, int flag, int argc,
>>                                   char * const argv[])
>>   {
>>          ulong   addr, length, ivt_offset;
>>          int     rcode = 0;
>>
>> -       if (argc < 4)
>> +       if (argc < 3)
> 
> I think we can also change here to argc < 2, the function
> get_image_ivt_offset() only requires the img addr now.

No, length is mandatory for authentication. To brief, this patch just
removes the ivt_offset as argument and make it optional. This is needed
because, in images like FIT, the location of the ivt header varies
dynamically depending on the total number of images clubbed.

In such cases, the existing hab_auth_img is hard to use as ivt header
offset needs to be pre-calculated everytime and fed into.

> 
>>                  return CMD_RET_USAGE;
>>
>>          addr = simple_strtoul(argv[1], NULL, 16);
>>          length = simple_strtoul(argv[2], NULL, 16);
>> -       ivt_offset = simple_strtoul(argv[3], NULL, 16);
>> +       if (argc == 3)
> 
> argc ==2

No, length is needed as stated above.

> 

-- 
Thanks,
Parthiban N

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-22 Fax: (+49)-8142-66989-80 Email: pn at denx.de

[U-Boot] [PATCH v2] imx: hab: extend hab_auth_img to calculate ivt_offset

[Breno Matheus Lima]

Hi Parthiban,

Em qua, 21 de nov de 2018 às 15:52, Parthiban Nallathambi <pn@denx.de> escreveu:
>
> Hi Breno,
>
> On 11/21/18 5:45 PM, Breno Matheus Lima wrote:
> > Hi Parthiban,
> >
> > Em qua, 21 de nov de 2018 às 11:50, Parthiban Nallathambi <pn@denx.de> escreveu:
> >>
> >> Current implementation of hab_auth_img command needs ivt_offset to
> >> authenticate the image. But ivt header is placed at the end of image
> >> date after padding.
> >>
> >> This leaves the usage of hab_auth_img command to fixed size or static
> >> offset for ivt header. New function "get_image_ivt_offset" is introduced
> >> to find the ivt offset during runtime. The case conditional check in this
> >> function is same as boot_get_kernel in common/bootm.c
> >>
> >> With this variable length image e.g. FIT image with any random size can
> >> have IVT at the end and ivt_offset option can be left optional
> >>
> >> Can be used as "hab_auth_img $loadaddr $filesize" from u-boot script
> >>
> >> Signed-off-by: Parthiban Nallathambi <pn@denx.de>
> >> ---
> >>
> >> Notes:
> >>      Changelog in v2:
> >>      - Finding IVT offset doesn't need length. Removed the
> >>      length argument from get_image_ivt_offset
> >>
> >>   arch/arm/mach-imx/hab.c | 29 +++++++++++++++++++++++++++--
> >>   1 file changed, 27 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-imx/hab.c b/arch/arm/mach-imx/hab.c
> >> index b88acd13da..dbfd692fa3 100644
> >> --- a/arch/arm/mach-imx/hab.c
> >> +++ b/arch/arm/mach-imx/hab.c
> >> @@ -6,6 +6,8 @@
> >>   #include <common.h>
> >>   #include <config.h>
> >>   #include <fuse.h>
> >> +#include <mapmem.h>
> >> +#include <image.h>
> >>   #include <asm/io.h>
> >>   #include <asm/system.h>
> >>   #include <asm/arch/clock.h>
> >> @@ -302,18 +304,41 @@ static int do_hab_status(cmd_tbl_t *cmdtp, int flag, int argc,
> >>          return 0;
> >>   }
> >>
> >> +static ulong get_image_ivt_offset(ulong img_addr)
> >> +{
> >> +       const void *buf;
> >> +
> >> +       buf = map_sysmem(img_addr, 0);
> >> +       switch (genimg_get_format(buf)) {
> >> +#if defined(CONFIG_IMAGE_FORMAT_LEGACY)
> >> +       case IMAGE_FORMAT_LEGACY:
> >> +               return (image_get_image_size((image_header_t *)img_addr)
> >> +                       + 0x1000 - 1)  & ~(0x1000 - 1);
> >> +#endif
> >> +#if IMAGE_ENABLE_FIT
> >> +       case IMAGE_FORMAT_FIT:
> >> +               return (fit_get_size(buf) + 0x1000 - 1)  & ~(0x1000 - 1);
> >> +#endif
> >> +       default:
> >> +               return 0;
> >> +       }
> >> +}
> >
> >
> > Do you have more details about the image header I should use here?
>
> Is the image signed using CST or similar tool? Is so, the signature data
> (HAB data: CSF, Certs and signature) pads at the end of the kernel
> image.

Yes, my Kernel image contains an IVT and is signed with CST. The image
layout looks like link below:
https://pastebin.com/5qEt7ETa

>
> >
> > I'm trying to get the IVT offset for my Kernel image based on NXP
> > 4.9.11_2.0.0_GA Linux release loaded at 0x80800000:
> >
> > => fatload mmc 0:1 0x80800000 zImage
> > => hab_auth_img 0x80800000 <Len not being used>
>
> Length for hab_auth_img is still mandatory. Length hear means the file
> size or total length of the image, this is required for the HAB API to
> authenticate (HAB_RVT_AUTHENTICATE_IMAGE).

Oh ok, I understood the scenario right now.

From my first overview I thought we would add IVT_SIZE + CSF_PAD_SIZE
in ivt_offset to calculate the image length, similar approach we have
in spl.c:
http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/mach-imx/spl.c;h=a20b30d154d788e4ebd4e22e9a6568a4f24c057e;hb=HEAD#l226

In this case only load addr would be necessary.

>
> >
> > The zImage size in Header looks correct (0x00726690), but
> > get_image_ivt_offset() is returning 0x0
>
> Looks like IVT offset is not found in the image.
>
> >
> > $ hexdump zImage | head
> > 0000000 0000 e1a0 0000 e1a0 0000 e1a0 0000 e1a0
> > *
> > 0000020 0003 ea00 2818 016f 0000 0000 6690 0072
> > 0000030 0201 0403 9000 e10f 04f8 eb00 7001 e1a0
> > 0000040 8002 e1a0 2000 e10f 0003 e312 0001 1a00
> >
> > Seems that genimg_get_format() is returning 0x0.
>
> Image is not signed?

No, my zImage is signed.

>
> >
> > Any ideias if I'm missing something?
> >
> >> +
> >>   static int do_authenticate_image(cmd_tbl_t *cmdtp, int flag, int argc,
> >>                                   char * const argv[])
> >>   {
> >>          ulong   addr, length, ivt_offset;
> >>          int     rcode = 0;
> >>
> >> -       if (argc < 4)
> >> +       if (argc < 3)
> >
> > I think we can also change here to argc < 2, the function
> > get_image_ivt_offset() only requires the img addr now.
>
> No, length is mandatory for authentication. To brief, this patch just
> removes the ivt_offset as argument and make it optional. This is needed
> because, in images like FIT, the location of the ivt header varies
> dynamically depending on the total number of images clubbed.
>
> In such cases, the existing hab_auth_img is hard to use as ivt header
> offset needs to be pre-calculated everytime and fed into.

Yes I understood your scenario right now, as I stated above I thought
your patch would also calculate the image length as we have in spl.c.

I'm only wondering why genimg_get_format() is returning
IMAGE_FORMAT_INVALID, do you happen to know if zImage header should
return IMAGE_FORMAT_LEGACY?

Thanks,
Breno Lima

[U-Boot] [PATCH v2] imx: hab: extend hab_auth_img to calculate ivt_offset

[Parthiban Nallathambi]

Hi Breno,

On 11/21/18 7:42 PM, Breno Matheus Lima wrote:
> Hi Parthiban,
> 
> Em qua, 21 de nov de 2018 às 15:52, Parthiban Nallathambi <pn@denx.de> escreveu:
>>
>> Hi Breno,
>>
>> On 11/21/18 5:45 PM, Breno Matheus Lima wrote:
>>> Hi Parthiban,
>>>
>>> Em qua, 21 de nov de 2018 às 11:50, Parthiban Nallathambi <pn@denx.de> escreveu:
>>>>
>>>> Current implementation of hab_auth_img command needs ivt_offset to
>>>> authenticate the image. But ivt header is placed at the end of image
>>>> date after padding.
>>>>
>>>> This leaves the usage of hab_auth_img command to fixed size or static
>>>> offset for ivt header. New function "get_image_ivt_offset" is introduced
>>>> to find the ivt offset during runtime. The case conditional check in this
>>>> function is same as boot_get_kernel in common/bootm.c
>>>>
>>>> With this variable length image e.g. FIT image with any random size can
>>>> have IVT at the end and ivt_offset option can be left optional
>>>>
>>>> Can be used as "hab_auth_img $loadaddr $filesize" from u-boot script
>>>>
>>>> Signed-off-by: Parthiban Nallathambi <pn@denx.de>
>>>> ---
>>>>
>>>> Notes:
>>>>      Changelog in v2:
>>>>      - Finding IVT offset doesn't need length. Removed the
>>>>      length argument from get_image_ivt_offset
>>>>
>>>>   arch/arm/mach-imx/hab.c | 29 +++++++++++++++++++++++++++--
>>>>   1 file changed, 27 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-imx/hab.c b/arch/arm/mach-imx/hab.c
>>>> index b88acd13da..dbfd692fa3 100644
>>>> --- a/arch/arm/mach-imx/hab.c
>>>> +++ b/arch/arm/mach-imx/hab.c
>>>> @@ -6,6 +6,8 @@
>>>>   #include <common.h>
>>>>   #include <config.h>
>>>>   #include <fuse.h>
>>>> +#include <mapmem.h>
>>>> +#include <image.h>
>>>>   #include <asm/io.h>
>>>>   #include <asm/system.h>
>>>>   #include <asm/arch/clock.h>
>>>> @@ -302,18 +304,41 @@ static int do_hab_status(cmd_tbl_t *cmdtp, int flag, int argc,
>>>>          return 0;
>>>>   }
>>>>
>>>> +static ulong get_image_ivt_offset(ulong img_addr)
>>>> +{
>>>> +       const void *buf;
>>>> +
>>>> +       buf = map_sysmem(img_addr, 0);
>>>> +       switch (genimg_get_format(buf)) {
>>>> +#if defined(CONFIG_IMAGE_FORMAT_LEGACY)
>>>> +       case IMAGE_FORMAT_LEGACY:
>>>> +               return (image_get_image_size((image_header_t *)img_addr)
>>>> +                       + 0x1000 - 1)  & ~(0x1000 - 1);
>>>> +#endif
>>>> +#if IMAGE_ENABLE_FIT
>>>> +       case IMAGE_FORMAT_FIT:
>>>> +               return (fit_get_size(buf) + 0x1000 - 1)  & ~(0x1000 - 1);
>>>> +#endif
>>>> +       default:
>>>> +               return 0;
>>>> +       }
>>>> +}
>>>
>>>
>>> Do you have more details about the image header I should use here?
>>
>> Is the image signed using CST or similar tool? Is so, the signature data
>> (HAB data: CSF, Certs and signature) pads at the end of the kernel
>> image.
> 
> Yes, my Kernel image contains an IVT and is signed with CST. The image
> layout looks like link below:
> https://pastebin.com/5qEt7ETa
> 
>>
>>>
>>> I'm trying to get the IVT offset for my Kernel image based on NXP
>>> 4.9.11_2.0.0_GA Linux release loaded at 0x80800000:
>>>
>>> => fatload mmc 0:1 0x80800000 zImage
>>> => hab_auth_img 0x80800000 <Len not being used>
>>
>> Length for hab_auth_img is still mandatory. Length hear means the file
>> size or total length of the image, this is required for the HAB API to
>> authenticate (HAB_RVT_AUTHENTICATE_IMAGE).
> 
> Oh ok, I understood the scenario right now.
> 
>>From my first overview I thought we would add IVT_SIZE + CSF_PAD_SIZE
> in ivt_offset to calculate the image length, similar approach we have
> in spl.c:
> http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/mach-imx/spl.c;h=a20b30d154d788e4ebd4e22e9a6568a4f24c057e;hb=HEAD#l226
> 
> In this case only load addr would be necessary.
> 
>>
>>>
>>> The zImage size in Header looks correct (0x00726690), but
>>> get_image_ivt_offset() is returning 0x0
>>
>> Looks like IVT offset is not found in the image.
>>
>>>
>>> $ hexdump zImage | head
>>> 0000000 0000 e1a0 0000 e1a0 0000 e1a0 0000 e1a0
>>> *
>>> 0000020 0003 ea00 2818 016f 0000 0000 6690 0072
>>> 0000030 0201 0403 9000 e10f 04f8 eb00 7001 e1a0
>>> 0000040 8002 e1a0 2000 e10f 0003 e312 0001 1a00
>>>
>>> Seems that genimg_get_format() is returning 0x0.
>>
>> Image is not signed?
> 
> No, my zImage is signed.
> 
>>
>>>
>>> Any ideias if I'm missing something?
>>>
>>>> +
>>>>   static int do_authenticate_image(cmd_tbl_t *cmdtp, int flag, int argc,
>>>>                                   char * const argv[])
>>>>   {
>>>>          ulong   addr, length, ivt_offset;
>>>>          int     rcode = 0;
>>>>
>>>> -       if (argc < 4)
>>>> +       if (argc < 3)
>>>
>>> I think we can also change here to argc < 2, the function
>>> get_image_ivt_offset() only requires the img addr now.
>>
>> No, length is mandatory for authentication. To brief, this patch just
>> removes the ivt_offset as argument and make it optional. This is needed
>> because, in images like FIT, the location of the ivt header varies
>> dynamically depending on the total number of images clubbed.
>>
>> In such cases, the existing hab_auth_img is hard to use as ivt header
>> offset needs to be pre-calculated everytime and fed into.
> 
> Yes I understood your scenario right now, as I stated above I thought
> your patch would also calculate the image length as we have in spl.c.
> 
> I'm only wondering why genimg_get_format() is returning
> IMAGE_FORMAT_INVALID, do you happen to know if zImage header should
> return IMAGE_FORMAT_LEGACY?

No, LEGACY directly checks for uImage magic "0x27051956". In general,
genimg_get_format handles only uImage, fitimage and android magic alone.

> 
> Thanks,
> Breno Lima
> 

-- 
Thanks,
Parthiban N

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-22 Fax: (+49)-8142-66989-80 Email: pn at denx.de

[U-Boot] [PATCH v2] imx: hab: extend hab_auth_img to calculate ivt_offset

[Breno Matheus Lima]

Hi Parthiban,

Em qua, 21 de nov de 2018 às 18:47, Parthiban Nallathambi <pn@denx.de> escreveu:
>
> Hi Breno,
>
> On 11/21/18 7:42 PM, Breno Matheus Lima wrote:
> > Hi Parthiban,
> >
> > Em qua, 21 de nov de 2018 às 15:52, Parthiban Nallathambi <pn@denx.de> escreveu:
> >>
> >> Hi Breno,
> >>
> >> On 11/21/18 5:45 PM, Breno Matheus Lima wrote:
> >>> Hi Parthiban,
> >>>
> >>> Em qua, 21 de nov de 2018 às 11:50, Parthiban Nallathambi <pn@denx.de> escreveu:
> >>>>
> >>>> Current implementation of hab_auth_img command needs ivt_offset to
> >>>> authenticate the image. But ivt header is placed at the end of image
> >>>> date after padding.
> >>>>
> >>>> This leaves the usage of hab_auth_img command to fixed size or static
> >>>> offset for ivt header. New function "get_image_ivt_offset" is introduced
> >>>> to find the ivt offset during runtime. The case conditional check in this
> >>>> function is same as boot_get_kernel in common/bootm.c
> >>>>
> >>>> With this variable length image e.g. FIT image with any random size can
> >>>> have IVT at the end and ivt_offset option can be left optional
> >>>>
> >>>> Can be used as "hab_auth_img $loadaddr $filesize" from u-boot script
> >>>>
> >>>> Signed-off-by: Parthiban Nallathambi <pn@denx.de>
> >>>> ---
> >>>>
> >>>> Notes:
> >>>>      Changelog in v2:
> >>>>      - Finding IVT offset doesn't need length. Removed the
> >>>>      length argument from get_image_ivt_offset
> >>>>
> >>>>   arch/arm/mach-imx/hab.c | 29 +++++++++++++++++++++++++++--
> >>>>   1 file changed, 27 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/arch/arm/mach-imx/hab.c b/arch/arm/mach-imx/hab.c
> >>>> index b88acd13da..dbfd692fa3 100644
> >>>> --- a/arch/arm/mach-imx/hab.c
> >>>> +++ b/arch/arm/mach-imx/hab.c
> >>>> @@ -6,6 +6,8 @@
> >>>>   #include <common.h>
> >>>>   #include <config.h>
> >>>>   #include <fuse.h>
> >>>> +#include <mapmem.h>
> >>>> +#include <image.h>
> >>>>   #include <asm/io.h>
> >>>>   #include <asm/system.h>
> >>>>   #include <asm/arch/clock.h>
> >>>> @@ -302,18 +304,41 @@ static int do_hab_status(cmd_tbl_t *cmdtp, int flag, int argc,
> >>>>          return 0;
> >>>>   }
> >>>>
> >>>> +static ulong get_image_ivt_offset(ulong img_addr)
> >>>> +{
> >>>> +       const void *buf;
> >>>> +
> >>>> +       buf = map_sysmem(img_addr, 0);
> >>>> +       switch (genimg_get_format(buf)) {
> >>>> +#if defined(CONFIG_IMAGE_FORMAT_LEGACY)
> >>>> +       case IMAGE_FORMAT_LEGACY:
> >>>> +               return (image_get_image_size((image_header_t *)img_addr)
> >>>> +                       + 0x1000 - 1)  & ~(0x1000 - 1);
> >>>> +#endif
> >>>> +#if IMAGE_ENABLE_FIT
> >>>> +       case IMAGE_FORMAT_FIT:
> >>>> +               return (fit_get_size(buf) + 0x1000 - 1)  & ~(0x1000 - 1);
> >>>> +#endif
> >>>> +       default:
> >>>> +               return 0;
> >>>> +       }
> >>>> +}
> >>>
> >>>
> >>> Do you have more details about the image header I should use here?
> >>
> >> Is the image signed using CST or similar tool? Is so, the signature data
> >> (HAB data: CSF, Certs and signature) pads at the end of the kernel
> >> image.
> >
> > Yes, my Kernel image contains an IVT and is signed with CST. The image
> > layout looks like link below:
> > https://pastebin.com/5qEt7ETa
> >
> >>
> >>>
> >>> I'm trying to get the IVT offset for my Kernel image based on NXP
> >>> 4.9.11_2.0.0_GA Linux release loaded at 0x80800000:
> >>>
> >>> => fatload mmc 0:1 0x80800000 zImage
> >>> => hab_auth_img 0x80800000 <Len not being used>
> >>
> >> Length for hab_auth_img is still mandatory. Length hear means the file
> >> size or total length of the image, this is required for the HAB API to
> >> authenticate (HAB_RVT_AUTHENTICATE_IMAGE).
> >
> > Oh ok, I understood the scenario right now.
> >
> >>From my first overview I thought we would add IVT_SIZE + CSF_PAD_SIZE
> > in ivt_offset to calculate the image length, similar approach we have
> > in spl.c:
> > http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/mach-imx/spl.c;h=a20b30d154d788e4ebd4e22e9a6568a4f24c057e;hb=HEAD#l226
> >
> > In this case only load addr would be necessary.
> >
> >>
> >>>
> >>> The zImage size in Header looks correct (0x00726690), but
> >>> get_image_ivt_offset() is returning 0x0
> >>
> >> Looks like IVT offset is not found in the image.
> >>
> >>>
> >>> $ hexdump zImage | head
> >>> 0000000 0000 e1a0 0000 e1a0 0000 e1a0 0000 e1a0
> >>> *
> >>> 0000020 0003 ea00 2818 016f 0000 0000 6690 0072
> >>> 0000030 0201 0403 9000 e10f 04f8 eb00 7001 e1a0
> >>> 0000040 8002 e1a0 2000 e10f 0003 e312 0001 1a00
> >>>
> >>> Seems that genimg_get_format() is returning 0x0.
> >>
> >> Image is not signed?
> >
> > No, my zImage is signed.
> >
> >>
> >>>
> >>> Any ideias if I'm missing something?
> >>>
> >>>> +
> >>>>   static int do_authenticate_image(cmd_tbl_t *cmdtp, int flag, int argc,
> >>>>                                   char * const argv[])
> >>>>   {
> >>>>          ulong   addr, length, ivt_offset;
> >>>>          int     rcode = 0;
> >>>>
> >>>> -       if (argc < 4)
> >>>> +       if (argc < 3)
> >>>
> >>> I think we can also change here to argc < 2, the function
> >>> get_image_ivt_offset() only requires the img addr now.
> >>
> >> No, length is mandatory for authentication. To brief, this patch just
> >> removes the ivt_offset as argument and make it optional. This is needed
> >> because, in images like FIT, the location of the ivt header varies
> >> dynamically depending on the total number of images clubbed.
> >>
> >> In such cases, the existing hab_auth_img is hard to use as ivt header
> >> offset needs to be pre-calculated everytime and fed into.
> >
> > Yes I understood your scenario right now, as I stated above I thought
> > your patch would also calculate the image length as we have in spl.c.
> >
> > I'm only wondering why genimg_get_format() is returning
> > IMAGE_FORMAT_INVALID, do you happen to know if zImage header should
> > return IMAGE_FORMAT_LEGACY?
>
> No, LEGACY directly checks for uImage magic "0x27051956". In general,
> genimg_get_format handles only uImage, fitimage and android magic alone.

Thanks for the details, the use case is now clear.

Reviewed-by: Breno Lima <breno.lima@nxp.com>

[U-Boot] [PATCH v2] imx: hab: extend hab_auth_img to calculate ivt_offset

[Stefano Babic]

On 21/11/18 14:50, Parthiban Nallathambi wrote:
> Current implementation of hab_auth_img command needs ivt_offset to
> authenticate the image. But ivt header is placed at the end of image
> date after padding.
> 
> This leaves the usage of hab_auth_img command to fixed size or static
> offset for ivt header. New function "get_image_ivt_offset" is introduced
> to find the ivt offset during runtime. The case conditional check in this
> function is same as boot_get_kernel in common/bootm.c
> 
> With this variable length image e.g. FIT image with any random size can
> have IVT at the end and ivt_offset option can be left optional
> 
> Can be used as "hab_auth_img $loadaddr $filesize" from u-boot script
> 
> Signed-off-by: Parthiban Nallathambi <pn@denx.de>
> ---
> 

Applied to u-boot-imx, master, thanks !

Best regards,
Stefano Babic


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================