[PATCH] mkimage: Call verify_header after writing image to disk
If image backend provides verify_header callback then call it after writing image to disk. This ensures that written image is correct.
Signed-off-by: Pali Rohár pali@kernel.org --- tools/mkimage.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)
diff --git a/tools/mkimage.c b/tools/mkimage.c index fbe883ce3620..2d282630787c 100644 --- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -698,6 +698,40 @@ int main(int argc, char **argv) exit (EXIT_FAILURE); }
+ if (tparams->verify_header) { + ifd = open(params.imagefile, O_RDONLY | O_BINARY); + if (ifd < 0) { + fprintf(stderr, "%s: Can't open %s: %s\n", + params.cmdname, params.imagefile, + strerror(errno)); + exit(EXIT_FAILURE); + } + + if (fstat(ifd, &sbuf) < 0) { + fprintf(stderr, "%s: Can't stat %s: %s\n", + params.cmdname, params.imagefile, strerror(errno)); + exit(EXIT_FAILURE); + } + params.file_size = sbuf.st_size; + + map_len = sbuf.st_size; + ptr = mmap(0, sbuf.st_size, PROT_READ, MAP_SHARED, ifd, 0); + if (ptr == MAP_FAILED) { + fprintf(stderr, "%s: Can't map %s: %s\n", + params.cmdname, params.imagefile, strerror(errno)); + exit(EXIT_FAILURE); + } + + if (tparams->verify_header((unsigned char *)ptr, sbuf.st_size, ¶ms) != 0) { + fprintf(stderr, "%s: Failed to verify header of %s\n", + params.cmdname, params.imagefile); + exit(EXIT_FAILURE); + } + + (void)munmap((void *)ptr, map_len); + close(ifd); + } + exit (EXIT_SUCCESS); }
Hi Pali,
On Thu, 13 Jan 2022 at 07:42, Pali Rohár pali@kernel.org wrote:
If image backend provides verify_header callback then call it after writing image to disk. This ensures that written image is correct.
Signed-off-by: Pali Rohár pali@kernel.org
tools/mkimage.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)
diff --git a/tools/mkimage.c b/tools/mkimage.c index fbe883ce3620..2d282630787c 100644 --- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -698,6 +698,40 @@ int main(int argc, char **argv) exit (EXIT_FAILURE); }
if (tparams->verify_header) {ifd = open(params.imagefile, O_RDONLY | O_BINARY);if (ifd < 0) {fprintf(stderr, "%s: Can't open %s: %s\n",params.cmdname, params.imagefile,strerror(errno));exit(EXIT_FAILURE);}if (fstat(ifd, &sbuf) < 0) {fprintf(stderr, "%s: Can't stat %s: %s\n",params.cmdname, params.imagefile, strerror(errno));exit(EXIT_FAILURE);}params.file_size = sbuf.st_size;map_len = sbuf.st_size;ptr = mmap(0, sbuf.st_size, PROT_READ, MAP_SHARED, ifd, 0);if (ptr == MAP_FAILED) {fprintf(stderr, "%s: Can't map %s: %s\n",params.cmdname, params.imagefile, strerror(errno));exit(EXIT_FAILURE);}if (tparams->verify_header((unsigned char *)ptr, sbuf.st_size, ¶ms) != 0) {fprintf(stderr, "%s: Failed to verify header of %s\n",params.cmdname, params.imagefile);exit(EXIT_FAILURE);}(void)munmap((void *)ptr, map_len);close(ifd);}exit (EXIT_SUCCESS);}
main() is so long already. Could this move into a function?
Regards, Simon
If image backend provides verify_header callback then call it after writing image to disk. This ensures that written image is correct.
Signed-off-by: Pali Rohár pali@kernel.org --- tools/mkimage.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
diff --git a/tools/mkimage.c b/tools/mkimage.c index fbe883ce3620..d5ad0925225c 100644 --- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -336,6 +336,44 @@ static void process_args(int argc, char **argv) usage("Missing output filename"); }
+static void verify_image(const struct image_type_params *tparams) +{ + struct stat sbuf; + void *ptr; + int ifd; + + ifd = open(params.imagefile, O_RDONLY | O_BINARY); + if (ifd < 0) { + fprintf(stderr, "%s: Can't open %s: %s\n", + params.cmdname, params.imagefile, + strerror(errno)); + exit(EXIT_FAILURE); + } + + if (fstat(ifd, &sbuf) < 0) { + fprintf(stderr, "%s: Can't stat %s: %s\n", + params.cmdname, params.imagefile, strerror(errno)); + exit(EXIT_FAILURE); + } + params.file_size = sbuf.st_size; + + ptr = mmap(0, params.file_size, PROT_READ, MAP_SHARED, ifd, 0); + if (ptr == MAP_FAILED) { + fprintf(stderr, "%s: Can't map %s: %s\n", + params.cmdname, params.imagefile, strerror(errno)); + exit(EXIT_FAILURE); + } + + if (tparams->verify_header((unsigned char *)ptr, params.file_size, ¶ms) != 0) { + fprintf(stderr, "%s: Failed to verify header of %s\n", + params.cmdname, params.imagefile); + exit(EXIT_FAILURE); + } + + (void)munmap(ptr, params.file_size); + (void)close(ifd); +} + int main(int argc, char **argv) { int ifd = -1; @@ -698,6 +736,9 @@ int main(int argc, char **argv) exit (EXIT_FAILURE); }
+ if (tparams->verify_header) + verify_image(tparams); + exit (EXIT_SUCCESS); }
On 1/14/22 18:34, Pali Rohár wrote:
If image backend provides verify_header callback then call it after writing image to disk. This ensures that written image is correct.
Signed-off-by: Pali Rohár pali@kernel.org
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
tools/mkimage.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
diff --git a/tools/mkimage.c b/tools/mkimage.c index fbe883ce3620..d5ad0925225c 100644 --- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -336,6 +336,44 @@ static void process_args(int argc, char **argv) usage("Missing output filename"); }
+static void verify_image(const struct image_type_params *tparams) +{
- struct stat sbuf;
- void *ptr;
- int ifd;
- ifd = open(params.imagefile, O_RDONLY | O_BINARY);
- if (ifd < 0) {
fprintf(stderr, "%s: Can't open %s: %s\n",params.cmdname, params.imagefile,strerror(errno));exit(EXIT_FAILURE);- }
- if (fstat(ifd, &sbuf) < 0) {
fprintf(stderr, "%s: Can't stat %s: %s\n",params.cmdname, params.imagefile, strerror(errno));exit(EXIT_FAILURE);- }
- params.file_size = sbuf.st_size;
- ptr = mmap(0, params.file_size, PROT_READ, MAP_SHARED, ifd, 0);
- if (ptr == MAP_FAILED) {
fprintf(stderr, "%s: Can't map %s: %s\n",params.cmdname, params.imagefile, strerror(errno));exit(EXIT_FAILURE);- }
- if (tparams->verify_header((unsigned char *)ptr, params.file_size, ¶ms) != 0) {
fprintf(stderr, "%s: Failed to verify header of %s\n",params.cmdname, params.imagefile);exit(EXIT_FAILURE);- }
- (void)munmap(ptr, params.file_size);
- (void)close(ifd);
+}
- int main(int argc, char **argv) { int ifd = -1;
@@ -698,6 +736,9 @@ int main(int argc, char **argv) exit (EXIT_FAILURE); }
- if (tparams->verify_header)
verify_image(tparams);- exit (EXIT_SUCCESS); }
Viele Grüße, Stefan Roese
On Fri, 14 Jan 2022 at 10:35, Pali Rohár pali@kernel.org wrote:
If image backend provides verify_header callback then call it after writing image to disk. This ensures that written image is correct.
Signed-off-by: Pali Rohár pali@kernel.org
tools/mkimage.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
Missing change log, BTW.
On Fri, Jan 14, 2022 at 06:34:43PM +0100, Pali Rohár wrote:
If image backend provides verify_header callback then call it after writing image to disk. This ensures that written image is correct.
Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Simon Glass sjg@chromium.org
tools/mkimage.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
This breaks a number of platforms such as ls1021atwr_sdcard_qspi and it's not clear to me why exactly.
On Friday 21 January 2022 16:21:33 Tom Rini wrote:
On Fri, Jan 14, 2022 at 06:34:43PM +0100, Pali Rohár wrote:
If image backend provides verify_header callback then call it after writing image to disk. This ensures that written image is correct.
Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Simon Glass sjg@chromium.org
tools/mkimage.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
This breaks a number of platforms such as ls1021atwr_sdcard_qspi and it's not clear to me why exactly.
Maybe they were already broken and this patch just detected it? Or verify_header callback for particular image type is reject valid image?
Do you have some pointers to failed build logs?
On Sat, Jan 22, 2022 at 02:44:22AM +0100, Pali Rohár wrote:
On Friday 21 January 2022 16:21:33 Tom Rini wrote:
On Fri, Jan 14, 2022 at 06:34:43PM +0100, Pali Rohár wrote:
If image backend provides verify_header callback then call it after writing image to disk. This ensures that written image is correct.
Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Simon Glass sjg@chromium.org
tools/mkimage.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
This breaks a number of platforms such as ls1021atwr_sdcard_qspi and it's not clear to me why exactly.
Maybe they were already broken and this patch just detected it? Or verify_header callback for particular image type is reject valid image?
Do you have some pointers to failed build logs?
Try building for ls1021atwr_sdcard_qspi with your patch applied, the only new thing that's shown in the logs is the error message.
On Friday 21 January 2022 21:15:43 Tom Rini wrote:
On Sat, Jan 22, 2022 at 02:44:22AM +0100, Pali Rohár wrote:
On Friday 21 January 2022 16:21:33 Tom Rini wrote:
On Fri, Jan 14, 2022 at 06:34:43PM +0100, Pali Rohár wrote:
If image backend provides verify_header callback then call it after writing image to disk. This ensures that written image is correct.
Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Simon Glass sjg@chromium.org
tools/mkimage.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
This breaks a number of platforms such as ls1021atwr_sdcard_qspi and it's not clear to me why exactly.
Maybe they were already broken and this patch just detected it? Or verify_header callback for particular image type is reject valid image?
Do you have some pointers to failed build logs?
Try building for ls1021atwr_sdcard_qspi with your patch applied, the only new thing that's shown in the logs is the error message.
So... I have tried following without this patch:
$ make ls1021atwr_sdcard_qspi_defconfig $ make CROSS_COMPILE=arm-linux-gnueabi- -j8
It generated file spl/u-boot-spl.pbl without error. Now I called -l on this generated file for type pblimage and I got following output:
$ ./tools/dumpimage -T pblimage -l spl/u-boot-spl.pbl GP Header: Size aa55aa55 LoadAddr 1ee0100
$ ./tools/mkimage -T pblimage -l spl/u-boot-spl.pbl GP Header: Size aa55aa55 LoadAddr 1ee0100
"GP Header:" line is from the TI OMAP image backend type gpimage or type omapimage (implemented in file gpimage-common.c).
So it means that files generated by ls1021atwr_sdcard_qspi are already broken and my patch just detected it. Or it is also possible that validation code in pblimage.c file is incorrect and broken.
What to do with it now?
On Saturday 22 January 2022 17:31:18 Pali Rohár wrote:
On Friday 21 January 2022 21:15:43 Tom Rini wrote:
On Sat, Jan 22, 2022 at 02:44:22AM +0100, Pali Rohár wrote:
On Friday 21 January 2022 16:21:33 Tom Rini wrote:
On Fri, Jan 14, 2022 at 06:34:43PM +0100, Pali Rohár wrote:
If image backend provides verify_header callback then call it after writing image to disk. This ensures that written image is correct.
Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Simon Glass sjg@chromium.org
tools/mkimage.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
This breaks a number of platforms such as ls1021atwr_sdcard_qspi and it's not clear to me why exactly.
Maybe they were already broken and this patch just detected it? Or verify_header callback for particular image type is reject valid image?
Do you have some pointers to failed build logs?
Try building for ls1021atwr_sdcard_qspi with your patch applied, the only new thing that's shown in the logs is the error message.
So... I have tried following without this patch:
$ make ls1021atwr_sdcard_qspi_defconfig $ make CROSS_COMPILE=arm-linux-gnueabi- -j8
It generated file spl/u-boot-spl.pbl without error. Now I called -l on this generated file for type pblimage and I got following output:
$ ./tools/dumpimage -T pblimage -l spl/u-boot-spl.pbl GP Header: Size aa55aa55 LoadAddr 1ee0100
$ ./tools/mkimage -T pblimage -l spl/u-boot-spl.pbl GP Header: Size aa55aa55 LoadAddr 1ee0100
"GP Header:" line is from the TI OMAP image backend type gpimage or type omapimage (implemented in file gpimage-common.c).
So it means that files generated by ls1021atwr_sdcard_qspi are already broken and my patch just detected it. Or it is also possible that validation code in pblimage.c file is incorrect and broken.
Just to note validation is failing on this tools/pblimage.c check:
if (pbl_hdr->rcwheader != reverse_byte(RCW_HEADER))
What to do with it now?
On Sat, Jan 22, 2022 at 05:31:18PM +0100, Pali Rohár wrote:
On Friday 21 January 2022 21:15:43 Tom Rini wrote:
On Sat, Jan 22, 2022 at 02:44:22AM +0100, Pali Rohár wrote:
On Friday 21 January 2022 16:21:33 Tom Rini wrote:
On Fri, Jan 14, 2022 at 06:34:43PM +0100, Pali Rohár wrote:
If image backend provides verify_header callback then call it after writing image to disk. This ensures that written image is correct.
Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Simon Glass sjg@chromium.org
tools/mkimage.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
This breaks a number of platforms such as ls1021atwr_sdcard_qspi and it's not clear to me why exactly.
Maybe they were already broken and this patch just detected it? Or verify_header callback for particular image type is reject valid image?
Do you have some pointers to failed build logs?
Try building for ls1021atwr_sdcard_qspi with your patch applied, the only new thing that's shown in the logs is the error message.
So... I have tried following without this patch:
$ make ls1021atwr_sdcard_qspi_defconfig $ make CROSS_COMPILE=arm-linux-gnueabi- -j8
It generated file spl/u-boot-spl.pbl without error. Now I called -l on this generated file for type pblimage and I got following output:
$ ./tools/dumpimage -T pblimage -l spl/u-boot-spl.pbl GP Header: Size aa55aa55 LoadAddr 1ee0100
$ ./tools/mkimage -T pblimage -l spl/u-boot-spl.pbl GP Header: Size aa55aa55 LoadAddr 1ee0100
"GP Header:" line is from the TI OMAP image backend type gpimage or type omapimage (implemented in file gpimage-common.c).
So it means that files generated by ls1021atwr_sdcard_qspi are already broken and my patch just detected it. Or it is also possible that validation code in pblimage.c file is incorrect and broken.
What to do with it now?
Thanks for digging. This is a problem for a number of the ls1021, ls1043 and ls1046 platforms, so lets add some maintainers there.
-----Original Message----- From: Tom Rini trini@konsulko.com Sent: Saturday, January 22, 2022 10:05 PM To: Pali Rohár pali@kernel.org; Alison Wang alison.wang@nxp.com; Priyanka Jain priyanka.jain@nxp.com; Mingkai Hu mingkai.hu@nxp.com; Rajesh Bhagat rajesh.bhagat@nxp.com Cc: Simon Glass sjg@chromium.org; Alexandru Gagniuc mr.nuke.me@gmail.com; Yann Dirson yann@blade-group.com; Stefan Roese sr@denx.de; Marek Behún marek.behun@nic.cz; u- boot@lists.denx.de Subject: Re: [PATCH v2] tools: mkimage: Call verify_header after writing image to disk
On Sat, Jan 22, 2022 at 05:31:18PM +0100, Pali Rohár wrote:
On Friday 21 January 2022 21:15:43 Tom Rini wrote:
On Sat, Jan 22, 2022 at 02:44:22AM +0100, Pali Rohár wrote:
On Friday 21 January 2022 16:21:33 Tom Rini wrote:
On Fri, Jan 14, 2022 at 06:34:43PM +0100, Pali Rohár wrote:
If image backend provides verify_header callback then call it after writing image to disk. This ensures that written image is correct.
Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Simon Glass sjg@chromium.org
tools/mkimage.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
This breaks a number of platforms such as ls1021atwr_sdcard_qspi and it's not clear to me why exactly.
Maybe they were already broken and this patch just detected it? Or verify_header callback for particular image type is reject valid image?
Do you have some pointers to failed build logs?
Try building for ls1021atwr_sdcard_qspi with your patch applied, the only new thing that's shown in the logs is the error message.
So... I have tried following without this patch:
$ make ls1021atwr_sdcard_qspi_defconfig $ make CROSS_COMPILE=arm-linux-gnueabi- -j8
It generated file spl/u-boot-spl.pbl without error. Now I called -l on this generated file for type pblimage and I got following output:
$ ./tools/dumpimage -T pblimage -l spl/u-boot-spl.pbl GP Header: Size aa55aa55 LoadAddr 1ee0100
$ ./tools/mkimage -T pblimage -l spl/u-boot-spl.pbl GP Header: Size aa55aa55 LoadAddr 1ee0100
"GP Header:" line is from the TI OMAP image backend type gpimage or type omapimage (implemented in file gpimage-common.c).
So it means that files generated by ls1021atwr_sdcard_qspi are already broken and my patch just detected it. Or it is also possible that validation code in pblimage.c file is incorrect and broken.
What to do with it now?
Thanks for digging. This is a problem for a number of the ls1021, ls1043 and ls1046 platforms, so lets add some maintainers there.
-- Tom
I will ask NXP-platform owners to check on this.
Thanks Priyanka
On Wednesday 02 February 2022 09:06:30 Priyanka Jain wrote:
-----Original Message----- From: Tom Rini trini@konsulko.com Sent: Saturday, January 22, 2022 10:05 PM To: Pali Rohár pali@kernel.org; Alison Wang alison.wang@nxp.com; Priyanka Jain priyanka.jain@nxp.com; Mingkai Hu mingkai.hu@nxp.com; Rajesh Bhagat rajesh.bhagat@nxp.com Cc: Simon Glass sjg@chromium.org; Alexandru Gagniuc mr.nuke.me@gmail.com; Yann Dirson yann@blade-group.com; Stefan Roese sr@denx.de; Marek Behún marek.behun@nic.cz; u- boot@lists.denx.de Subject: Re: [PATCH v2] tools: mkimage: Call verify_header after writing image to disk
On Sat, Jan 22, 2022 at 05:31:18PM +0100, Pali Rohár wrote:
On Friday 21 January 2022 21:15:43 Tom Rini wrote:
On Sat, Jan 22, 2022 at 02:44:22AM +0100, Pali Rohár wrote:
On Friday 21 January 2022 16:21:33 Tom Rini wrote:
On Fri, Jan 14, 2022 at 06:34:43PM +0100, Pali Rohár wrote:
> If image backend provides verify_header callback then call it > after writing image to disk. This ensures that written image is correct. > > Signed-off-by: Pali Rohár pali@kernel.org > Reviewed-by: Stefan Roese sr@denx.de > Reviewed-by: Simon Glass sjg@chromium.org > --- > tools/mkimage.c | 41 > +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 41 insertions(+)
This breaks a number of platforms such as ls1021atwr_sdcard_qspi and it's not clear to me why exactly.
Maybe they were already broken and this patch just detected it? Or verify_header callback for particular image type is reject valid image?
Do you have some pointers to failed build logs?
Try building for ls1021atwr_sdcard_qspi with your patch applied, the only new thing that's shown in the logs is the error message.
So... I have tried following without this patch:
$ make ls1021atwr_sdcard_qspi_defconfig $ make CROSS_COMPILE=arm-linux-gnueabi- -j8
It generated file spl/u-boot-spl.pbl without error. Now I called -l on this generated file for type pblimage and I got following output:
$ ./tools/dumpimage -T pblimage -l spl/u-boot-spl.pbl GP Header: Size aa55aa55 LoadAddr 1ee0100
$ ./tools/mkimage -T pblimage -l spl/u-boot-spl.pbl GP Header: Size aa55aa55 LoadAddr 1ee0100
"GP Header:" line is from the TI OMAP image backend type gpimage or type omapimage (implemented in file gpimage-common.c).
So it means that files generated by ls1021atwr_sdcard_qspi are already broken and my patch just detected it. Or it is also possible that validation code in pblimage.c file is incorrect and broken.
What to do with it now?
Thanks for digging. This is a problem for a number of the ls1021, ls1043 and ls1046 platforms, so lets add some maintainers there.
-- Tom
I will ask NXP-platform owners to check on this.
Thanks Priyanka
Hello! Any news on this?
-----Original Message----- From: Pali Rohár pali@kernel.org Sent: Wednesday, February 16, 2022 1:22 AM To: Priyanka Jain priyanka.jain@nxp.com Cc: Tom Rini trini@konsulko.com; Alison Wang alison.wang@nxp.com; Mingkai Hu mingkai.hu@nxp.com; Rajesh Bhagat rajesh.bhagat@nxp.com; Jiafei Pan jiafei.pan@nxp.com; Simon Glass sjg@chromium.org; Alexandru Gagniuc mr.nuke.me@gmail.com; Yann Dirson yann@blade-group.com; Stefan Roese sr@denx.de; Marek Behún marek.behun@nic.cz; u- boot@lists.denx.de Subject: Re: [PATCH v2] tools: mkimage: Call verify_header after writing image to disk
On Wednesday 02 February 2022 09:06:30 Priyanka Jain wrote:
-----Original Message----- From: Tom Rini trini@konsulko.com Sent: Saturday, January 22, 2022 10:05 PM To: Pali Rohár pali@kernel.org; Alison Wang alison.wang@nxp.com; Priyanka Jain priyanka.jain@nxp.com; Mingkai Hu mingkai.hu@nxp.com; Rajesh Bhagat rajesh.bhagat@nxp.com Cc: Simon Glass sjg@chromium.org; Alexandru Gagniuc mr.nuke.me@gmail.com; Yann Dirson yann@blade-group.com; Stefan Roese sr@denx.de; Marek Behún marek.behun@nic.cz; u- boot@lists.denx.de Subject: Re: [PATCH v2] tools: mkimage: Call verify_header after writing image to disk
On Sat, Jan 22, 2022 at 05:31:18PM +0100, Pali Rohár wrote:
On Friday 21 January 2022 21:15:43 Tom Rini wrote:
On Sat, Jan 22, 2022 at 02:44:22AM +0100, Pali Rohár wrote:
On Friday 21 January 2022 16:21:33 Tom Rini wrote: > On Fri, Jan 14, 2022 at 06:34:43PM +0100, Pali Rohár wrote: > > > If image backend provides verify_header callback then call > > it after writing image to disk. This ensures that written image is
correct.
> > > > Signed-off-by: Pali Rohár pali@kernel.org > > Reviewed-by: Stefan Roese sr@denx.de > > Reviewed-by: Simon Glass sjg@chromium.org > > --- > > tools/mkimage.c | 41 > > +++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 41 insertions(+) > > This breaks a number of platforms such as > ls1021atwr_sdcard_qspi and it's not clear to me why exactly.
Maybe they were already broken and this patch just detected it? Or verify_header callback for particular image type is reject valid image?
Do you have some pointers to failed build logs?
Try building for ls1021atwr_sdcard_qspi with your patch applied, the only new thing that's shown in the logs is the error message.
So... I have tried following without this patch:
$ make ls1021atwr_sdcard_qspi_defconfig $ make CROSS_COMPILE=arm-linux-gnueabi- -j8
It generated file spl/u-boot-spl.pbl without error. Now I called -l on this generated file for type pblimage and I got following output:
$ ./tools/dumpimage -T pblimage -l spl/u-boot-spl.pbl GP Header: Size aa55aa55 LoadAddr 1ee0100
$ ./tools/mkimage -T pblimage -l spl/u-boot-spl.pbl GP Header: Size aa55aa55 LoadAddr 1ee0100
"GP Header:" line is from the TI OMAP image backend type gpimage or type omapimage (implemented in file gpimage-common.c).
So it means that files generated by ls1021atwr_sdcard_qspi are already broken and my patch just detected it. Or it is also possible that validation code in pblimage.c file is incorrect and broken.
What to do with it now?
Thanks for digging. This is a problem for a number of the ls1021, ls1043 and ls1046 platforms, so lets add some maintainers there.
-- Tom
I will ask NXP-platform owners to check on this.
Thanks Priyanka
Hello! Any news on this?
Not yet, Actually team was busy in some other critical work. I will try my best to get response as soon as possible.
Thanks Priyanka
-----Original Message----- From: Pali Rohár pali@kernel.org Sent: 2022年2月16日 3:52 To: Priyanka Jain priyanka.jain@nxp.com Cc: Tom Rini trini@konsulko.com; Alison Wang alison.wang@nxp.com; Mingkai Hu mingkai.hu@nxp.com; Rajesh Bhagat rajesh.bhagat@nxp.com; Jiafei Pan jiafei.pan@nxp.com; Simon Glass sjg@chromium.org; Alexandru Gagniuc mr.nuke.me@gmail.com; Yann Dirson yann@blade-group.com; Stefan Roese sr@denx.de; Marek Behún marek.behun@nic.cz; u-boot@lists.denx.de Subject: Re: [PATCH v2] tools: mkimage: Call verify_header after writing image to disk
On Wednesday 02 February 2022 09:06:30 Priyanka Jain wrote:
-----Original Message----- From: Tom Rini trini@konsulko.com Sent: Saturday, January 22, 2022 10:05 PM To: Pali Rohár pali@kernel.org; Alison Wang alison.wang@nxp.com; Priyanka Jain priyanka.jain@nxp.com; Mingkai Hu mingkai.hu@nxp.com; Rajesh Bhagat rajesh.bhagat@nxp.com Cc: Simon Glass sjg@chromium.org; Alexandru Gagniuc mr.nuke.me@gmail.com; Yann Dirson yann@blade-group.com;
Stefan
Roese sr@denx.de; Marek Behún marek.behun@nic.cz; u- boot@lists.denx.de Subject: Re: [PATCH v2] tools: mkimage: Call verify_header after writing image to disk
On Sat, Jan 22, 2022 at 05:31:18PM +0100, Pali Rohár wrote:
On Friday 21 January 2022 21:15:43 Tom Rini wrote:
On Sat, Jan 22, 2022 at 02:44:22AM +0100, Pali Rohár wrote:
On Friday 21 January 2022 16:21:33 Tom Rini wrote: > On Fri, Jan 14, 2022 at 06:34:43PM +0100, Pali Rohár wrote: > > > If image backend provides verify_header callback then call > > it after writing image to disk. This ensures that written image is
correct.
> > > > Signed-off-by: Pali Rohár pali@kernel.org > > Reviewed-by: Stefan Roese sr@denx.de > > Reviewed-by: Simon Glass sjg@chromium.org > > --- > > tools/mkimage.c | 41 > > +++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 41 insertions(+) > > This breaks a number of platforms such as > ls1021atwr_sdcard_qspi and it's not clear to me why exactly.
Maybe they were already broken and this patch just detected it? Or verify_header callback for particular image type is reject valid image?
Do you have some pointers to failed build logs?
Try building for ls1021atwr_sdcard_qspi with your patch applied, the only new thing that's shown in the logs is the error message.
So... I have tried following without this patch:
$ make ls1021atwr_sdcard_qspi_defconfig $ make CROSS_COMPILE=arm-linux-gnueabi- -j8
It generated file spl/u-boot-spl.pbl without error. Now I called -l on this generated file for type pblimage and I got following output:
$ ./tools/dumpimage -T pblimage -l spl/u-boot-spl.pbl GP Header: Size aa55aa55 LoadAddr 1ee0100
$ ./tools/mkimage -T pblimage -l spl/u-boot-spl.pbl GP Header: Size aa55aa55 LoadAddr 1ee0100
"GP Header:" line is from the TI OMAP image backend type gpimage or type omapimage (implemented in file gpimage-common.c).
So it means that files generated by ls1021atwr_sdcard_qspi are already broken and my patch just detected it. Or it is also possible that validation code in pblimage.c file is incorrect and broken.
What to do with it now?
Thanks for digging. This is a problem for a number of the ls1021, ls1043 and ls1046 platforms, so lets add some maintainers there.
-- Tom
I will ask NXP-platform owners to check on this.
Thanks Priyanka
Hello! Any news on this?
Indeed it is a historical problem and exposed by this patch. The problem is the RCW headers are different on PPC and ARM platforms, while ls1021a/ls1043a/ls1046a leveraged the PPC tool to generate PBL image, but didn’t handle this differentially, this result in the image header verification failed. I'll submit a patch to fix it.
Thanks, Zhiqiang
On Friday 21 January 2022 16:21:33 Tom Rini wrote:
On Fri, Jan 14, 2022 at 06:34:43PM +0100, Pali Rohár wrote:
If image backend provides verify_header callback then call it after writing image to disk. This ensures that written image is correct.
Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Simon Glass sjg@chromium.org
tools/mkimage.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
This breaks a number of platforms such as ls1021atwr_sdcard_qspi and it's not clear to me why exactly.
-- Tom
Hello Tom! Is something still blocking this change?
On Sun, Mar 06, 2022 at 12:50:24PM +0100, Pali Rohár wrote:
On Friday 21 January 2022 16:21:33 Tom Rini wrote:
On Fri, Jan 14, 2022 at 06:34:43PM +0100, Pali Rohár wrote:
If image backend provides verify_header callback then call it after writing image to disk. This ensures that written image is correct.
Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Simon Glass sjg@chromium.org
tools/mkimage.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
This breaks a number of platforms such as ls1021atwr_sdcard_qspi and it's not clear to me why exactly.
Hello Tom! Is something still blocking this change?
Nope, thanks for the reminder.
On Fri, Jan 14, 2022 at 06:34:43PM +0100, Pali Rohár wrote:
If image backend provides verify_header callback then call it after writing image to disk. This ensures that written image is correct.
Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Simon Glass sjg@chromium.org
tools/mkimage.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
diff --git a/tools/mkimage.c b/tools/mkimage.c index fbe883ce3620..d5ad0925225c 100644 --- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -336,6 +336,44 @@ static void process_args(int argc, char **argv) usage("Missing output filename"); }
+static void verify_image(const struct image_type_params *tparams) +{
- struct stat sbuf;
- void *ptr;
- int ifd;
- ifd = open(params.imagefile, O_RDONLY | O_BINARY);
- if (ifd < 0) {
fprintf(stderr, "%s: Can't open %s: %s\n",params.cmdname, params.imagefile,strerror(errno));exit(EXIT_FAILURE);- }
- if (fstat(ifd, &sbuf) < 0) {
fprintf(stderr, "%s: Can't stat %s: %s\n",params.cmdname, params.imagefile, strerror(errno));exit(EXIT_FAILURE);- }
- params.file_size = sbuf.st_size;
- ptr = mmap(0, params.file_size, PROT_READ, MAP_SHARED, ifd, 0);
- if (ptr == MAP_FAILED) {
fprintf(stderr, "%s: Can't map %s: %s\n",params.cmdname, params.imagefile, strerror(errno));exit(EXIT_FAILURE);- }
- if (tparams->verify_header((unsigned char *)ptr, params.file_size, ¶ms) != 0) {
fprintf(stderr, "%s: Failed to verify header of %s\n",params.cmdname, params.imagefile);exit(EXIT_FAILURE);- }
- (void)munmap(ptr, params.file_size);
- (void)close(ifd);
+}
int main(int argc, char **argv) { int ifd = -1; @@ -698,6 +736,9 @@ int main(int argc, char **argv) exit (EXIT_FAILURE); }
- if (tparams->verify_header)
verify_image(tparams);- exit (EXIT_SUCCESS);
}
I've added Joseph Chen to the thread as with this patch applied, evb-rk3568 fails to build now with: ./tools/mkimage: Failed to verify header of idbloader.img which is another issue to resolve.
Hi, The rk3568 uses V2 IDB structrure and the rkcommon_verify_header() has not cover V2. My teammate will fix it these days and kever.yang will submit his patch to mainline.
陈健洪 (Joseph Chen) E-mail：chenjh@rock-chips.com 瑞芯微电子股份有限公司 Rockchip Electronics Co.Ltd 福建省福州市铜盘路软件大道89号软件园A区21号楼 (350003) No. 21 Building, A District, No.89,software Boulevard Fuzhou,Fujian,PRC TEL：0591-83991906/07-8573
From: Tom Rini Date: 2022-03-08 21:42 To: PaliRohár; Joseph Chen CC: Simon Glass; Alexandru Gagniuc; Yann Dirson; Stefan Roese; MarekBehún; u-boot Subject: Re: [PATCH v2] tools: mkimage: Call verify_header after writing image to disk On Fri, Jan 14, 2022 at 06:34:43PM +0100, Pali Rohár wrote:
If image backend provides verify_header callback then call it after writing image to disk. This ensures that written image is correct.
Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Simon Glass sjg@chromium.org
tools/mkimage.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
diff --git a/tools/mkimage.c b/tools/mkimage.c index fbe883ce3620..d5ad0925225c 100644 --- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -336,6 +336,44 @@ static void process_args(int argc, char **argv) usage("Missing output filename"); }
+static void verify_image(const struct image_type_params *tparams) +{
- struct stat sbuf;
- void *ptr;
- int ifd;
- ifd = open(params.imagefile, O_RDONLY | O_BINARY);
- if (ifd < 0) {
- fprintf(stderr, "%s: Can't open %s: %s\n",
- params.cmdname, params.imagefile,
- strerror(errno));
- exit(EXIT_FAILURE);
- }
- if (fstat(ifd, &sbuf) < 0) {
- fprintf(stderr, "%s: Can't stat %s: %s\n",
- params.cmdname, params.imagefile, strerror(errno));
- exit(EXIT_FAILURE);
- }
- params.file_size = sbuf.st_size;
- ptr = mmap(0, params.file_size, PROT_READ, MAP_SHARED, ifd, 0);
- if (ptr == MAP_FAILED) {
- fprintf(stderr, "%s: Can't map %s: %s\n",
- params.cmdname, params.imagefile, strerror(errno));
- exit(EXIT_FAILURE);
- }
- if (tparams->verify_header((unsigned char *)ptr, params.file_size, ¶ms) != 0) {
- fprintf(stderr, "%s: Failed to verify header of %s\n",
- params.cmdname, params.imagefile);
- exit(EXIT_FAILURE);
- }
- (void)munmap(ptr, params.file_size);
- (void)close(ifd);
+}
int main(int argc, char **argv) { int ifd = -1; @@ -698,6 +736,9 @@ int main(int argc, char **argv) exit (EXIT_FAILURE); }
- if (tparams->verify_header)
- verify_image(tparams);
exit (EXIT_SUCCESS); }
I've added Joseph Chen to the thread as with this patch applied, evb-rk3568 fails to build now with: ./tools/mkimage: Failed to verify header of idbloader.img which is another issue to resolve.
Hi, The rk3568 uses V2 IDB structrure and the rkcommon_verify_header() has not cover V2. My teammate will fix it these days and kever.yang will submit his patch to mainline.
陈健洪 (Joseph Chen) E-mail：chenjh@rock-chips.com 瑞芯微电子股份有限公司 Rockchip Electronics Co.Ltd 福建省福州市铜盘路软件大道89号软件园A区21号楼 (350003) No. 21 Building, A District, No.89,software Boulevard Fuzhou,Fujian,PRC TEL：0591-83991906/07-8573
From: Tom Rini Date: 2022-03-08 21:42 To: PaliRohár; Joseph Chen CC: Simon Glass; Alexandru Gagniuc; Yann Dirson; Stefan Roese; MarekBehún; u-boot Subject: Re: [PATCH v2] tools: mkimage: Call verify_header after writing image to disk On Fri, Jan 14, 2022 at 06:34:43PM +0100, Pali Rohár wrote:
If image backend provides verify_header callback then call it after writing image to disk. This ensures that written image is correct.
Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Simon Glass sjg@chromium.org
tools/mkimage.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
diff --git a/tools/mkimage.c b/tools/mkimage.c index fbe883ce3620..d5ad0925225c 100644 --- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -336,6 +336,44 @@ static void process_args(int argc, char **argv) usage("Missing output filename"); }
+static void verify_image(const struct image_type_params *tparams) +{
- struct stat sbuf;
- void *ptr;
- int ifd;
- ifd = open(params.imagefile, O_RDONLY | O_BINARY);
- if (ifd < 0) {
- fprintf(stderr, "%s: Can't open %s: %s\n",
- params.cmdname, params.imagefile,
- strerror(errno));
- exit(EXIT_FAILURE);
- }
- if (fstat(ifd, &sbuf) < 0) {
- fprintf(stderr, "%s: Can't stat %s: %s\n",
- params.cmdname, params.imagefile, strerror(errno));
- exit(EXIT_FAILURE);
- }
- params.file_size = sbuf.st_size;
- ptr = mmap(0, params.file_size, PROT_READ, MAP_SHARED, ifd, 0);
- if (ptr == MAP_FAILED) {
- fprintf(stderr, "%s: Can't map %s: %s\n",
- params.cmdname, params.imagefile, strerror(errno));
- exit(EXIT_FAILURE);
- }
- if (tparams->verify_header((unsigned char *)ptr, params.file_size, ¶ms) != 0) {
- fprintf(stderr, "%s: Failed to verify header of %s\n",
- params.cmdname, params.imagefile);
- exit(EXIT_FAILURE);
- }
- (void)munmap(ptr, params.file_size);
- (void)close(ifd);
+}
int main(int argc, char **argv) { int ifd = -1; @@ -698,6 +736,9 @@ int main(int argc, char **argv) exit (EXIT_FAILURE); }
- if (tparams->verify_header)
- verify_image(tparams);
exit (EXIT_SUCCESS); }
I've added Joseph Chen to the thread as with this patch applied, evb-rk3568 fails to build now with: ./tools/mkimage: Failed to verify header of idbloader.img which is another issue to resolve.
On Fri, Jan 14, 2022 at 06:34:43PM +0100, Pali Rohár wrote:
If image backend provides verify_header callback then call it after writing image to disk. This ensures that written image is correct.
Signed-off-by: Pali Rohár pali@kernel.org Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
participants (7)
-
Pali Rohár -
Priyanka Jain -
Simon Glass -
Stefan Roese -
Tom Rini -
Z.Q. Hou -
陈健洪