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

[Breno Matheus Lima <brenomatheus@gmail.com>]

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>