| Message ID | 20220916045423.3635985-8-judge.packham@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | arm: mvebu: Support for 98DX25xx/98DX35xx (AlleyCat5) | expand |
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 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@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 >
On Fri, 16 Sep 2022, 8:12 PM Pali Rohár, <pali@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. 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@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 > > >
On Friday 16 September 2022 22:34:52 Chris Packham wrote: > On Fri, 16 Sep 2022, 8:12 PM Pali Rohár, <pali@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@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 > > > > >
On Friday 16 September 2022 22:34:52 Chris Packham wrote: > I do wonder if the boot seqence and xmodem stuff could be abstracted out to > something that could be reused by other tools. In the past I was thinking about it... but I come to the conclusion that it is easier to write specific tools which implements communication with just one BootROM. Trying to write one universal thing just opens a lot of issues and at the end it would do same thing like if you implement N independent applications and then additional "launcher" application which starts the correct one. It looks like that most Marvell SoCs use xmodem protocol. Except 3720 which uses WTPTP. But as I figured out, every SoC use slightly modified protocol based on xmodem. So well, in theory "sx" would work. But if you want to have other features (like progress bar or bootrom output or speed change) then all this is bootrom specific and has to be implemented directly into xmodem state machine. So either you provide N xmodem implementations or try to create something "hookable" and since beginning I have feeling that "hooks" would just introduce new bugs and make it harder to debug. kwboot is currently in the state that it supports kwbimage v0 and kwbimage v1 formats, which IIRC covers all 32-bit widely used SoCs from kirkwood, dove, armada and switches which integrates those CPUs, and probably also avanta. At lot of stages it expects valid kwbimage and that is why there is validation at the beginning. It injects 32-bit ARM binary code for changing UART speed and requires above SoC (as it touches internal registers, which are same on all those mentioned). It also contains workarounds for bugs in Armada 385 BootROMs. I really think that kwboot is now in state when it is not easily possible to extend it for different platform without lot of energy and extra testing that it does not break something existing.
Hi Pali, On Sat, Sep 17, 2022 at 12:37 AM Pali Rohár <pali@kernel.org> wrote: > > On Friday 16 September 2022 22:34:52 Chris Packham wrote: > > I do wonder if the boot seqence and xmodem stuff could be abstracted out to > > something that could be reused by other tools. > > In the past I was thinking about it... but I come to the conclusion that > it is easier to write specific tools which implements communication with > just one BootROM. Trying to write one universal thing just opens a lot > of issues and at the end it would do same thing like if you implement N > independent applications and then additional "launcher" application > which starts the correct one. > > It looks like that most Marvell SoCs use xmodem protocol. Except 3720 > which uses WTPTP. But as I figured out, every SoC use slightly modified > protocol based on xmodem. So well, in theory "sx" would work. But if you > want to have other features (like progress bar or bootrom output or > speed change) then all this is bootrom specific and has to be > implemented directly into xmodem state machine. So either you provide N > xmodem implementations or try to create something "hookable" and since > beginning I have feeling that "hooks" would just introduce new bugs and > make it harder to debug. > > > kwboot is currently in the state that it supports kwbimage v0 and > kwbimage v1 formats, which IIRC covers all 32-bit widely used SoCs from > kirkwood, dove, armada and switches which integrates those CPUs, and > probably also avanta. At lot of stages it expects valid kwbimage and > that is why there is validation at the beginning. It injects 32-bit ARM > binary code for changing UART speed and requires above SoC (as it > touches internal registers, which are same on all those mentioned). It > also contains workarounds for bugs in Armada 385 BootROMs. > > I really think that kwboot is now in state when it is not easily > possible to extend it for different platform without lot of energy and > extra testing that it does not break something existing. Having looked more into mox-imager (and WtpDownloader) I've realised that the AlleyCat5 uses something called "TIM" but it's different to the format used by Armada-3700. I suspect what I'm actually dealing with is a TIMv0 that pre-dates the version used by Armada-3700. All the more reason to do something new instead of trying to shoe-horn it into one of the existing places. I'll probably end up making a fork of your mvebu64boot and adapting it (mox-imager just does too much).
On Monday 19 September 2022 10:57:10 Chris Packham wrote: > Having looked more into mox-imager (and WtpDownloader) I've realised > that the AlleyCat5 uses something called "TIM" but it's different to > the format used by Armada-3700. I suspect what I'm actually dealing > with is a TIMv0 that pre-dates the version used by Armada-3700. All > the more reason to do something new instead of trying to shoe-horn it > into one of the existing places. I'll probably end up making a fork of > your mvebu64boot and adapting it (mox-imager just does too much). Hello! If that AC5 TIM format is different than A3720 TIM then it really makes sense to create a new tool. Forking mvebu64boot into completely new project seems like the easiest way how to achieve working tool without being afraid to break existing support. Anyway, is not there any Marvell tool for this AC5 TIM format? Like A3720 TBB or WtpDownloader?
On Mon, Sep 19, 2022 at 11:03 AM Pali Rohár <pali@kernel.org> wrote: > > On Monday 19 September 2022 10:57:10 Chris Packham wrote: > > Having looked more into mox-imager (and WtpDownloader) I've realised > > that the AlleyCat5 uses something called "TIM" but it's different to > > the format used by Armada-3700. I suspect what I'm actually dealing > > with is a TIMv0 that pre-dates the version used by Armada-3700. All > > the more reason to do something new instead of trying to shoe-horn it > > into one of the existing places. I'll probably end up making a fork of > > your mvebu64boot and adapting it (mox-imager just does too much). > > Hello! If that AC5 TIM format is different than A3720 TIM then it really > makes sense to create a new tool. Forking mvebu64boot into completely > new project seems like the easiest way how to achieve working tool > without being afraid to break existing support. > > Anyway, is not there any Marvell tool for this AC5 TIM format? Like > A3720 TBB or WtpDownloader? Nope the only official support is send the boot pattern using ascii/raw then send the image using xmodem. Which is what led to me looking for an alternative that can actually tell when the SoC has detected the pattern and hand off to the xmodem download. Supporting the signed images would be a bonus but not for my immediate need for recovering from bad bootloaders.
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");
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@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(+)