[PATCH 7/7] tools: kwboot: Add knowledge of Marvell's TIM

[Pali Rohár]

On Friday 16 September 2022 22:34:52 Chris Packham wrote:
> On Fri, 16 Sep 2022, 8:12 PM Pali Rohár, <pali at kernel.org> wrote:
> 
> > Hello! I think it does not make sense to hack kwboot to skip validation
> > of kwbimage format when ad-hoc TIM header is detected. kwboot has now
> > lot of features which requires and expects valid kwbimage format and is
> > now written to work specially with 32-bit mvebu ARM BootROMs.
> >
> > TIM and kwbimage are totally different formats and it really does not
> > make sense to starting rewriting kwboot to support also other format.
> > Instead it would be better to write other dedicated tool for it.
> >
> > For example, there is already tool mox-imager [1], which despite its
> > name supports all A3720 BootROMS and mvebu64boot [2] which supports
> > A70x0, A80x0 and CN9130 BootROMS.
> >
> > [1] - https://gitlab.nic.cz/turris/mox-imager
> > [2] - https://github.com/pali/mvebu64boot
> 
> 
> Yeah I tend to agree.
> 
> Mvebu64boot would fit better since this is a 64 bit Marveel SoC but again
> I'd have to teach it about TIM.

mox-imager already implements generating and parsing TIM images as this
is used on A3720. But for UART booting is used custom WTPTP protocol,
not variant of xmodem.

> One reason I went with kwboot (aside from
> not knowing anything else existed) was that the hand off between the uart
> boot sequence and the xmodem was awkward with the "official" methods (2
> different transfer with 2 different protocols).
> 
> I do wonder if the boot seqence and xmodem stuff could be abstracted out to
> something that could be reused by other tools.
> 
> I  the short term at least I'll drop this out of the series.
> 
> 
> >
> > On Friday 16 September 2022 16:54:23 Chris Packham wrote:
> > > Marvell's proprietary TIM (trusted image) is used on the Armada-3700 and
> > > AlledCat5/5X (and possibly others). It has a whole host of features that
> > > work to implement a version of secure boot.
> > >
> > > Kwboot's interest in this format is simply to detect that the image is
> > > one of these and not attempt to patch it (the images will work over UART
> > > boot as-is). This is done by checking for a specific magic value
> > > ("TIMH") in the first 32bits of the image.
> > >
> > > Signed-off-by: Chris Packham <judge.packham at gmail.com>
> > > ---
> > > It might be possible to make the check more robust by validating more of
> > > the image. There is a checksum field that might be useful for this
> > > purpose. I haven't done this as I couldn't figure out Marvell's
> > > validation of this field.
> > >
> > >  tools/kwbimage.h | 29 +++++++++++++++++++++++++++++
> > >  tools/kwboot.c   |  3 +++
> > >  2 files changed, 32 insertions(+)
> > >
> > > diff --git a/tools/kwbimage.h b/tools/kwbimage.h
> > > index 505522332b..8aab26952a 100644
> > > --- a/tools/kwbimage.h
> > > +++ b/tools/kwbimage.h
> > > @@ -224,6 +224,28 @@ struct register_set_hdr_v1 {
> > >  #define OPT_HDR_V1_BINARY_TYPE   0x2
> > >  #define OPT_HDR_V1_REGISTER_TYPE 0x3
> > >
> > > +/* TIM (trusted image), Armada 3700, AlleyCat5 */
> > > +struct tim_block_hdr {
> > > +     uint32_t signature_id;
> > > +     uint16_t opcode;
> > > +     uint16_t blocksize;
> > > +} __packed;
> > > +
> > > +struct tim_hdr {
> > > +     struct tim_block_hdr hdr;
> > > +     uint32_t trusted;
> > > +     uint32_t signed_tim_size;
> > > +     uint32_t unsigned_tim_size;
> > > +     uint32_t unique_id;
> > > +     uint64_t loadaddr;
> > > +     uint32_t flags;
> > > +     uint32_t software_prot_version;
> > > +     uint32_t num_blocks;
> > > +     uint32_t checksum;
> > > +} __packed;
> > > +
> > > +#define TIM_HDR_SIGNATURE    0x54494d48 /* "TIMH" */
> > > +
> > >  /*
> > >   * Byte 8 of the image header contains the version number. In the v0
> > >   * header, byte 8 was reserved, and always set to 0. In the v1 header,
> > > @@ -270,6 +292,13 @@ static inline size_t kwbheader_size_for_csum(const
> > void *header)
> > >               return kwbheader_size(header);
> > >  }
> > >
> > > +static inline bool kwbimage_is_tim(void *img)
> > > +{
> > > +     const struct tim_hdr *hdr = img;
> > > +
> > > +     return le32_to_cpu(hdr->hdr.signature_id) == TIM_HDR_SIGNATURE;
> > > +}
> > > +
> > >  static inline struct ext_hdr_v0 *ext_hdr_v0_first(void *img)
> > >  {
> > >       struct main_hdr_v0 *mhdr;
> > > diff --git a/tools/kwboot.c b/tools/kwboot.c
> > > index da4fe32da2..a9b3d0fd04 100644
> > > --- a/tools/kwboot.c
> > > +++ b/tools/kwboot.c
> > > @@ -1869,6 +1869,9 @@ kwboot_img_patch(void *img, size_t *size, int
> > baudrate)
> > >       if (*size < sizeof(struct main_hdr_v1))
> > >               goto err;
> > >
> > > +     if (kwbimage_is_tim(img))
> > > +             return 0;
> > > +
> > >       image_ver = kwbimage_version(img);
> > >       if (image_ver != 0 && image_ver != 1) {
> > >               fprintf(stderr, "Invalid image header version\n");
> > > --
> > > 2.37.3
> > >
> >