Message ID | 1479341621-30021-1-git-send-email-eric@nelint.com |
---|---|
State | Accepted |
Commit | e5491f3ef5aa4b38067bd10dbdded9520305670f |
Delegated to: | Stefano Babic |
Headers | show |
Hi Eric, All, On Wed, Nov 16, 2016 at 05:13:41PM -0700, Eric Nelson wrote: > These values can be used to sign a U-Boot image for use when > loading an image through the Serial Download Protocol (SDP). > > Note that the address of 0x910000 is usable with the stock > configuration of imx_usb_loader on i.MX6 and i.MX7 SOCs: > > https://github.com/boundarydevices/imx_usb_loader/blob/master/mx6_usb_work.conf#L3 > > Refer to the section on imx_usb_loader in this post for more > details: > > https://boundarydevices.com/high-assurance-boot-hab-dummies/ > > Signed-off-by: Eric Nelson <eric@nelint.com> Thanks, indeed such patch would ease the life of anybody that needs to deal with HAB when creating the CSF files. > --- > tools/imximage.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/tools/imximage.c b/tools/imximage.c > index c9e42ec..2cd8d88 100644 > --- a/tools/imximage.c > +++ b/tools/imximage.c > @@ -281,7 +281,6 @@ static void set_dcd_rst_v2(struct imx_header *imxhdr, uint32_t dcd_len, > d = (struct dcd_v2_cmd *)(((char *)d) + len); > > len = (char *)d - (char *)&dcd_v2->header; > - Is this part of the patch intended? > dcd_v2->header.tag = DCD_HEADER_TAG; > dcd_v2->header.length = cpu_to_be16(len); > dcd_v2->header.version = DCD_VERSION; > @@ -501,10 +500,19 @@ static void print_hdr_v2(struct imx_header *imx_hdr) > printf("Entry Point: %08x\n", (uint32_t)fhdr_v2->entry); > if (fhdr_v2->csf && (imximage_ivt_offset != UNDEFINED) && > (imximage_csf_size != UNDEFINED)) { > + uint16_t dcdlen; > + int offs; > + > + dcdlen = hdr_v2->data.dcd_table.header.length; > + offs = (char *)&hdr_v2->data.dcd_table > + - (char *)hdr_v2; > + > printf("HAB Blocks: %08x %08x %08x\n", > (uint32_t)fhdr_v2->self, 0, This isn't part of the patch, but why is self cast into a uint32_t although it's already a uint32_t? > hdr_v2->boot_data.size - imximage_ivt_offset - > imximage_csf_size); > + printf("DCD Blocks: 00910000 %08x %08x\n", > + offs, be16_to_cpu(dcdlen)); > } Not sure if "DCD Blocks" is the best naming, cause it really just applies to SDP protocol. This got me thinking and I think the printf above should also show the Blocks for encryption which is also missing right now. What about something like the snippet below? if (fhdr_v2->csf && (imximage_ivt_offset != UNDEFINED) && (imximage_csf_size != UNDEFINED)) { uint16_t dcdlen; uint32_t dcdoff; uint32_t entryoff; dcdlen = hdr_v2->data.dcd_table.header.length; dcdoff = (char *)&hdr_v2->data.dcd_table - (char *)hdr_v2; entryoff = fhdr_v2->entry - fhdr_v2->self; printf("[HAB][Signature]\n"); printf("Blocks: %08x %08x %08x\n", (uint32_t)fhdr_v2->self, 0, hdr_v2->boot_data.size - imximage_ivt_offset - imximage_csf_size); printf("[HAB][Encryption]\n"); printf("Blocks: %08x %08x %08x\n", fhdr_v2->self, 0, dcdoff + be16_to_cpu(dcdlen)); printf("Blocks: %08x %08x %08x\n", fhdr_v2->entry, entryoff, hdr_v2->boot_data.size - imximage_ivt_offset - imximage_csf_size - entryoff); printf("[HAB][SDP]\n"); printf("Blocks: 00910000 %08x %08x\n", dcdoff, be16_to_cpu(dcdlen)); } Regards, Gary
Hi Gary, On 11/17/2016 05:16 AM, Gary Bisson wrote: > Hi Eric, All, > > On Wed, Nov 16, 2016 at 05:13:41PM -0700, Eric Nelson wrote: >> These values can be used to sign a U-Boot image for use when >> loading an image through the Serial Download Protocol (SDP). >> >> Note that the address of 0x910000 is usable with the stock >> configuration of imx_usb_loader on i.MX6 and i.MX7 SOCs: >> >> https://github.com/boundarydevices/imx_usb_loader/blob/master/mx6_usb_work.conf#L3 >> >> Refer to the section on imx_usb_loader in this post for more >> details: >> >> https://boundarydevices.com/high-assurance-boot-hab-dummies/ >> >> Signed-off-by: Eric Nelson <eric@nelint.com> > > Thanks, indeed such patch would ease the life of anybody that needs to > deal with HAB when creating the CSF files. > >> --- >> tools/imximage.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/tools/imximage.c b/tools/imximage.c >> index c9e42ec..2cd8d88 100644 >> --- a/tools/imximage.c >> +++ b/tools/imximage.c >> @@ -281,7 +281,6 @@ static void set_dcd_rst_v2(struct imx_header *imxhdr, uint32_t dcd_len, >> d = (struct dcd_v2_cmd *)(((char *)d) + len); >> >> len = (char *)d - (char *)&dcd_v2->header; >> - > > Is this part of the patch intended? > Nope. >> dcd_v2->header.tag = DCD_HEADER_TAG; >> dcd_v2->header.length = cpu_to_be16(len); >> dcd_v2->header.version = DCD_VERSION; >> @@ -501,10 +500,19 @@ static void print_hdr_v2(struct imx_header *imx_hdr) >> printf("Entry Point: %08x\n", (uint32_t)fhdr_v2->entry); >> if (fhdr_v2->csf && (imximage_ivt_offset != UNDEFINED) && >> (imximage_csf_size != UNDEFINED)) { >> + uint16_t dcdlen; >> + int offs; >> + >> + dcdlen = hdr_v2->data.dcd_table.header.length; >> + offs = (char *)&hdr_v2->data.dcd_table >> + - (char *)hdr_v2; >> + >> printf("HAB Blocks: %08x %08x %08x\n", >> (uint32_t)fhdr_v2->self, 0, > > This isn't part of the patch, but why is self cast into a uint32_t > although it's already a uint32_t? > Unrelated, but good catch! >> hdr_v2->boot_data.size - imximage_ivt_offset - >> imximage_csf_size); >> + printf("DCD Blocks: 00910000 %08x %08x\n", >> + offs, be16_to_cpu(dcdlen)); >> } > > Not sure if "DCD Blocks" is the best naming, cause it really just > applies to SDP protocol. > > This got me thinking and I think the printf above should also show the > Blocks for encryption which is also missing right now. > > What about something like the snippet below? > > if (fhdr_v2->csf && (imximage_ivt_offset != UNDEFINED) && > (imximage_csf_size != UNDEFINED)) { > uint16_t dcdlen; > uint32_t dcdoff; > uint32_t entryoff; > > dcdlen = hdr_v2->data.dcd_table.header.length; > dcdoff = (char *)&hdr_v2->data.dcd_table > - (char *)hdr_v2; > entryoff = fhdr_v2->entry - fhdr_v2->self; > > printf("[HAB][Signature]\n"); > printf("Blocks: %08x %08x %08x\n", > (uint32_t)fhdr_v2->self, 0, I think somebody just pointed out that self is already a uint32_t, so why the cast? > hdr_v2->boot_data.size - imximage_ivt_offset - > imximage_csf_size); > printf("[HAB][Encryption]\n"); > printf("Blocks: %08x %08x %08x\n", > fhdr_v2->self, 0, dcdoff + be16_to_cpu(dcdlen)); > printf("Blocks: %08x %08x %08x\n", > fhdr_v2->entry, entryoff, > hdr_v2->boot_data.size - imximage_ivt_offset - > imximage_csf_size - entryoff); > printf("[HAB][SDP]\n"); > printf("Blocks: 00910000 %08x %08x\n", > dcdoff, be16_to_cpu(dcdlen)); > } I like the more specific tags, but wonder if some minor edits wouldn't make this more useful. In particular, adding 0x before %08x would allow easier cut and paste into signing scripts in a manual process. I looked briefly at the output of 'mkimage -l' and found that it doesn't reach this code block because imximage_ivt/csf_size variables are set during the parsing of a .cfg file (they're UNDEFINED with 'mkimage -l').
Hi Eric, Gary, On 17/11/2016 15:23, Eric Nelson wrote: > Hi Gary, > > On 11/17/2016 05:16 AM, Gary Bisson wrote: >> Hi Eric, All, >> >> On Wed, Nov 16, 2016 at 05:13:41PM -0700, Eric Nelson wrote: >>> These values can be used to sign a U-Boot image for use when >>> loading an image through the Serial Download Protocol (SDP). >>> >>> Note that the address of 0x910000 is usable with the stock >>> configuration of imx_usb_loader on i.MX6 and i.MX7 SOCs: >>> >>> https://github.com/boundarydevices/imx_usb_loader/blob/master/mx6_usb_work.conf#L3 >>> >>> Refer to the section on imx_usb_loader in this post for more >>> details: >>> >>> https://boundarydevices.com/high-assurance-boot-hab-dummies/ >>> >>> Signed-off-by: Eric Nelson <eric@nelint.com> >> >> Thanks, indeed such patch would ease the life of anybody that needs to >> deal with HAB when creating the CSF files. >> >>> --- >>> tools/imximage.c | 10 +++++++++- >>> 1 file changed, 9 insertions(+), 1 deletion(-) >>> >>> diff --git a/tools/imximage.c b/tools/imximage.c >>> index c9e42ec..2cd8d88 100644 >>> --- a/tools/imximage.c >>> +++ b/tools/imximage.c >>> @@ -281,7 +281,6 @@ static void set_dcd_rst_v2(struct imx_header *imxhdr, uint32_t dcd_len, >>> d = (struct dcd_v2_cmd *)(((char *)d) + len); >>> >>> len = (char *)d - (char *)&dcd_v2->header; >>> - >> >> Is this part of the patch intended? >> > > Nope. > >>> dcd_v2->header.tag = DCD_HEADER_TAG; >>> dcd_v2->header.length = cpu_to_be16(len); >>> dcd_v2->header.version = DCD_VERSION; >>> @@ -501,10 +500,19 @@ static void print_hdr_v2(struct imx_header *imx_hdr) >>> printf("Entry Point: %08x\n", (uint32_t)fhdr_v2->entry); >>> if (fhdr_v2->csf && (imximage_ivt_offset != UNDEFINED) && >>> (imximage_csf_size != UNDEFINED)) { >>> + uint16_t dcdlen; >>> + int offs; >>> + >>> + dcdlen = hdr_v2->data.dcd_table.header.length; >>> + offs = (char *)&hdr_v2->data.dcd_table >>> + - (char *)hdr_v2; >>> + >>> printf("HAB Blocks: %08x %08x %08x\n", >>> (uint32_t)fhdr_v2->self, 0, >> >> This isn't part of the patch, but why is self cast into a uint32_t >> although it's already a uint32_t? >> > > Unrelated, but good catch! > >>> hdr_v2->boot_data.size - imximage_ivt_offset - >>> imximage_csf_size); >>> + printf("DCD Blocks: 00910000 %08x %08x\n", >>> + offs, be16_to_cpu(dcdlen)); >>> } >> >> Not sure if "DCD Blocks" is the best naming, cause it really just >> applies to SDP protocol. >> >> This got me thinking and I think the printf above should also show the >> Blocks for encryption which is also missing right now. >> >> What about something like the snippet below? >> >> if (fhdr_v2->csf && (imximage_ivt_offset != UNDEFINED) && >> (imximage_csf_size != UNDEFINED)) { >> uint16_t dcdlen; >> uint32_t dcdoff; >> uint32_t entryoff; >> >> dcdlen = hdr_v2->data.dcd_table.header.length; >> dcdoff = (char *)&hdr_v2->data.dcd_table >> - (char *)hdr_v2; >> entryoff = fhdr_v2->entry - fhdr_v2->self; >> >> printf("[HAB][Signature]\n"); >> printf("Blocks: %08x %08x %08x\n", >> (uint32_t)fhdr_v2->self, 0, > > I think somebody just pointed out that self is already a uint32_t, > so why the cast? > >> hdr_v2->boot_data.size - imximage_ivt_offset - >> imximage_csf_size); >> printf("[HAB][Encryption]\n"); >> printf("Blocks: %08x %08x %08x\n", >> fhdr_v2->self, 0, dcdoff + be16_to_cpu(dcdlen)); >> printf("Blocks: %08x %08x %08x\n", >> fhdr_v2->entry, entryoff, >> hdr_v2->boot_data.size - imximage_ivt_offset - >> imximage_csf_size - entryoff); >> printf("[HAB][SDP]\n"); >> printf("Blocks: 00910000 %08x %08x\n", >> dcdoff, be16_to_cpu(dcdlen)); >> } > > I like the more specific tags, but wonder if some minor edits wouldn't > make this more useful. Right. > > In particular, adding 0x before %08x would allow easier cut and paste > into signing scripts in a manual process. Most important is to get values. How easy is then to copy&paste, well, it is not so important. I have applied the patch to u-boot-imx - thanks ! Best regards, Stefano Babic
diff --git a/tools/imximage.c b/tools/imximage.c index c9e42ec..2cd8d88 100644 --- a/tools/imximage.c +++ b/tools/imximage.c @@ -281,7 +281,6 @@ static void set_dcd_rst_v2(struct imx_header *imxhdr, uint32_t dcd_len, d = (struct dcd_v2_cmd *)(((char *)d) + len); len = (char *)d - (char *)&dcd_v2->header; - dcd_v2->header.tag = DCD_HEADER_TAG; dcd_v2->header.length = cpu_to_be16(len); dcd_v2->header.version = DCD_VERSION; @@ -501,10 +500,19 @@ static void print_hdr_v2(struct imx_header *imx_hdr) printf("Entry Point: %08x\n", (uint32_t)fhdr_v2->entry); if (fhdr_v2->csf && (imximage_ivt_offset != UNDEFINED) && (imximage_csf_size != UNDEFINED)) { + uint16_t dcdlen; + int offs; + + dcdlen = hdr_v2->data.dcd_table.header.length; + offs = (char *)&hdr_v2->data.dcd_table + - (char *)hdr_v2; + printf("HAB Blocks: %08x %08x %08x\n", (uint32_t)fhdr_v2->self, 0, hdr_v2->boot_data.size - imximage_ivt_offset - imximage_csf_size); + printf("DCD Blocks: 00910000 %08x %08x\n", + offs, be16_to_cpu(dcdlen)); } } else { imx_header_v2_t *next_hdr_v2;
These values can be used to sign a U-Boot image for use when loading an image through the Serial Download Protocol (SDP). Note that the address of 0x910000 is usable with the stock configuration of imx_usb_loader on i.MX6 and i.MX7 SOCs: https://github.com/boundarydevices/imx_usb_loader/blob/master/mx6_usb_work.conf#L3 Refer to the section on imx_usb_loader in this post for more details: https://boundarydevices.com/high-assurance-boot-hab-dummies/ Signed-off-by: Eric Nelson <eric@nelint.com> --- tools/imximage.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)