[PATCH master] imx-usb-loader: Don't try to verify more data than contained in the image

[PATCH master] imx-usb-loader: Don't try to verify more data than contained in the image

[Ahmad Fatoum]

On platforms that don't have a 2nd stage (in my case i.MX6 without
imx6_barebox_start_usb), it usually happens that the transfer limit for
the first (and only) upload is bigger than the actual file length.
Then the right thing to do is processing the complete image (minus its
header), but not more. This was broken by recent refactoring and fixed
for the transfer case with commit 3cf4bcd86419 ("imx-usb-loader: Don't
try to transfer more data than contained in the image").

The same bug persisted in the verification code though, breaking
imx-usb-loader -c:

	verifying file...
	mismatch at offset 0x000999c0. expected:
	[ hexdump of last bytes of barebox binary ]

A jump to the binary will then be skipped and subsequent imx-usb-loader
invocations will have their DCD writes unanswered leading to the
dreaded:

	main dcd length 328
	DCD write: sub dcd length: 0x0324, flags: 0x04w3 in err=-7, last_trans=0  00 00 00 00
	addr=0x021b001c, val=0x04088032               w4 in err=-7, last_trans=0  00 00 00 00
	!!perform_dcd returned -7
	4 in err=-7, last_trans=0  00 00 00 00

Applying the same fix as in 3cf4bcd86419 fixes this issue as well.

Fixes: 3367ebc55ebe ("scripts: imx-usb-loader: simplify code flow for file size calculations")
Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Reported-by: Roland Hieber <r.hieber@pengutronix.de>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 scripts/imx/imx-usb-loader.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/imx/imx-usb-loader.c b/scripts/imx/imx-usb-loader.c
index 5f9c7ff3a458..676f077c2557 100644
--- a/scripts/imx/imx-usb-loader.c
+++ b/scripts/imx/imx-usb-loader.c
@@ -1415,7 +1415,7 @@ static int do_irom_download(struct usb_work *curr, int verify)
 	if (verify) {
 		printf("verifying file...\n");
 
-		ret = verify_memory(image, firststage_len, header_addr);
+		ret = verify_memory(image, min(fsize, firststage_len), header_addr);
 		if (ret < 0) {
 			printf("verifying failed\n");
 			goto cleanup;
-- 
2.30.2

Re: [PATCH master] imx-usb-loader: Don't try to verify more data than contained in the image

[Sascha Hauer]

On Tue, May 23, 2023 at 09:16:21AM +0200, Ahmad Fatoum wrote:
> On platforms that don't have a 2nd stage (in my case i.MX6 without
> imx6_barebox_start_usb), it usually happens that the transfer limit for
> the first (and only) upload is bigger than the actual file length.
> Then the right thing to do is processing the complete image (minus its
> header), but not more. This was broken by recent refactoring and fixed
> for the transfer case with commit 3cf4bcd86419 ("imx-usb-loader: Don't
> try to transfer more data than contained in the image").
> 
> The same bug persisted in the verification code though, breaking
> imx-usb-loader -c:
> 
> 	verifying file...
> 	mismatch at offset 0x000999c0. expected:
> 	[ hexdump of last bytes of barebox binary ]
> 
> A jump to the binary will then be skipped and subsequent imx-usb-loader
> invocations will have their DCD writes unanswered leading to the
> dreaded:
> 
> 	main dcd length 328
> 	DCD write: sub dcd length: 0x0324, flags: 0x04w3 in err=-7, last_trans=0  00 00 00 00
> 	addr=0x021b001c, val=0x04088032               w4 in err=-7, last_trans=0  00 00 00 00
> 	!!perform_dcd returned -7
> 	4 in err=-7, last_trans=0  00 00 00 00
> 
> Applying the same fix as in 3cf4bcd86419 fixes this issue as well.
> 
> Fixes: 3367ebc55ebe ("scripts: imx-usb-loader: simplify code flow for file size calculations")
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Reported-by: Roland Hieber <r.hieber@pengutronix.de>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Applied, thanks

Sascha

> ---
>  scripts/imx/imx-usb-loader.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/imx/imx-usb-loader.c b/scripts/imx/imx-usb-loader.c
> index 5f9c7ff3a458..676f077c2557 100644
> --- a/scripts/imx/imx-usb-loader.c
> +++ b/scripts/imx/imx-usb-loader.c
> @@ -1415,7 +1415,7 @@ static int do_irom_download(struct usb_work *curr, int verify)
>  	if (verify) {
>  		printf("verifying file...\n");
>  
> -		ret = verify_memory(image, firststage_len, header_addr);
> +		ret = verify_memory(image, min(fsize, firststage_len), header_addr);
>  		if (ret < 0) {
>  			printf("verifying failed\n");
>  			goto cleanup;
> -- 
> 2.30.2
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH master] imx-usb-loader: Don't try to verify more data than contained in the image

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2529 bytes --]

hello,

On Tue, May 23, 2023 at 09:16:21AM +0200, Ahmad Fatoum wrote:
> On platforms that don't have a 2nd stage (in my case i.MX6 without
> imx6_barebox_start_usb), it usually happens that the transfer limit for
> the first (and only) upload is bigger than the actual file length.
> Then the right thing to do is processing the complete image (minus its
> header), but not more. This was broken by recent refactoring and fixed
> for the transfer case with commit 3cf4bcd86419 ("imx-usb-loader: Don't
> try to transfer more data than contained in the image").
> 
> The same bug persisted in the verification code though, breaking
> imx-usb-loader -c:
> 
> 	verifying file...
> 	mismatch at offset 0x000999c0. expected:
> 	[ hexdump of last bytes of barebox binary ]
> 
> A jump to the binary will then be skipped and subsequent imx-usb-loader
> invocations will have their DCD writes unanswered leading to the
> dreaded:
> 
> 	main dcd length 328
> 	DCD write: sub dcd length: 0x0324, flags: 0x04w3 in err=-7, last_trans=0  00 00 00 00
> 	addr=0x021b001c, val=0x04088032               w4 in err=-7, last_trans=0  00 00 00 00
> 	!!perform_dcd returned -7
> 	4 in err=-7, last_trans=0  00 00 00 00
> 
> Applying the same fix as in 3cf4bcd86419 fixes this issue as well.
> 
> Fixes: 3367ebc55ebe ("scripts: imx-usb-loader: simplify code flow for file size calculations")
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Reported-by: Roland Hieber <r.hieber@pengutronix.de>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  scripts/imx/imx-usb-loader.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/imx/imx-usb-loader.c b/scripts/imx/imx-usb-loader.c
> index 5f9c7ff3a458..676f077c2557 100644
> --- a/scripts/imx/imx-usb-loader.c
> +++ b/scripts/imx/imx-usb-loader.c
> @@ -1415,7 +1415,7 @@ static int do_irom_download(struct usb_work *curr, int verify)
>  	if (verify) {
>  		printf("verifying file...\n");
>  
> -		ret = verify_memory(image, firststage_len, header_addr);
> +		ret = verify_memory(image, min(fsize, firststage_len), header_addr);
>  		if (ret < 0) {
>  			printf("verifying failed\n");
>  			goto cleanup;

Ah, I see. Looks right.

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

(hmm, already too late.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH master] imx-usb-loader: Don't try to verify more data than contained in the image

[Roland Hieber]

On Tue, May 23, 2023 at 09:55:24AM +0200, Uwe Kleine-König wrote:
> hello,
> 
> On Tue, May 23, 2023 at 09:16:21AM +0200, Ahmad Fatoum wrote:
> > On platforms that don't have a 2nd stage (in my case i.MX6 without
> > imx6_barebox_start_usb), it usually happens that the transfer limit for
> > the first (and only) upload is bigger than the actual file length.
> > Then the right thing to do is processing the complete image (minus its
> > header), but not more. This was broken by recent refactoring and fixed
> > for the transfer case with commit 3cf4bcd86419 ("imx-usb-loader: Don't
> > try to transfer more data than contained in the image").
> > 
> > The same bug persisted in the verification code though, breaking
> > imx-usb-loader -c:
> > 
> > 	verifying file...
> > 	mismatch at offset 0x000999c0. expected:
> > 	[ hexdump of last bytes of barebox binary ]
> > 
> > A jump to the binary will then be skipped and subsequent imx-usb-loader
> > invocations will have their DCD writes unanswered leading to the
> > dreaded:
> > 
> > 	main dcd length 328
> > 	DCD write: sub dcd length: 0x0324, flags: 0x04w3 in err=-7, last_trans=0  00 00 00 00
> > 	addr=0x021b001c, val=0x04088032               w4 in err=-7, last_trans=0  00 00 00 00
> > 	!!perform_dcd returned -7
> > 	4 in err=-7, last_trans=0  00 00 00 00
> > 
> > Applying the same fix as in 3cf4bcd86419 fixes this issue as well.
> > 
> > Fixes: 3367ebc55ebe ("scripts: imx-usb-loader: simplify code flow for file size calculations")
> > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Reported-by: Roland Hieber <r.hieber@pengutronix.de>
> > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> > ---
> >  scripts/imx/imx-usb-loader.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/scripts/imx/imx-usb-loader.c b/scripts/imx/imx-usb-loader.c
> > index 5f9c7ff3a458..676f077c2557 100644
> > --- a/scripts/imx/imx-usb-loader.c
> > +++ b/scripts/imx/imx-usb-loader.c
> > @@ -1415,7 +1415,7 @@ static int do_irom_download(struct usb_work *curr, int verify)
> >  	if (verify) {
> >  		printf("verifying file...\n");
> >  
> > -		ret = verify_memory(image, firststage_len, header_addr);
> > +		ret = verify_memory(image, min(fsize, firststage_len), header_addr);
> >  		if (ret < 0) {
> >  			printf("verifying failed\n");
> >  			goto cleanup;
> 
> Ah, I see. Looks right.
> 
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> (hmm, already too late.)

Tested-by: Roland Hieber <rhi@pengutronix.de>

(hmm, also too late. Thanks anyway!)

 - Roland

> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |



-- 
Roland Hieber, Pengutronix e.K.          | r.hieber@pengutronix.de     |
Steuerwalder Str. 21                     | https://www.pengutronix.de/ |
31137 Hildesheim, Germany                | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686         | Fax:   +49-5121-206917-5555 |