On Tuesday 23 August 2022 12:28:48 Stefan Roese wrote:
> Hi Pali,
> 
> On 09.08.22 21:42, Pali Rohár wrote:
> > Currently for A38x image is checked only header checksum.
> > So check also for image data checksum to prevent flashing broken image.
> > 
> > Signed-off-by: Pali Rohár <p...@kernel.org>
> > 
> > ---
> > Changes in v2:
> > * Compile fix (move code chunk from patch 2/2 to 1/2)
> > ---
> >   cmd/mvebu/bubt.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 44 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
> > index 2136af64163c..3b6ffb7ffd1f 100644
> > --- a/cmd/mvebu/bubt.c
> > +++ b/cmd/mvebu/bubt.c
> > @@ -688,9 +688,24 @@ static uint8_t image_checksum8(const void *start, 
> > size_t len)
> >     return csum;
> >   }
> > +static uint32_t image_checksum32(const void *start, size_t len)
> > +{
> > +   u32 csum = 0;
> > +   const u32 *p = start;
> > +
> > +   while (len) {
> > +           csum += *p;
> > +           ++p;
> > +           len -= sizeof(u32);
> > +   }
> > +
> > +   return csum;
> > +}
> > +
> >   static int check_image_header(void)
> >   {
> >     u8 checksum;
> > +   u32 checksum32, exp_checksum32;
> >     const struct a38x_main_hdr_v1 *hdr =
> >             (struct a38x_main_hdr_v1 *)get_load_addr();
> >     const size_t image_size = a38x_header_size(hdr);
> > @@ -701,11 +716,39 @@ static int check_image_header(void)
> >     checksum = image_checksum8(hdr, image_size);
> >     checksum -= hdr->checksum;
> >     if (checksum != hdr->checksum) {
> > -           printf("Error: Bad A38x image checksum. 0x%x != 0x%x\n",
> > +           printf("Error: Bad A38x image header checksum. 0x%x != 0x%x\n",
> >                    checksum, hdr->checksum);
> >             return -ENOEXEC;
> >     }
> > +   offset = le32_to_cpu(hdr->srcaddr);
> > +   size = le32_to_cpu(hdr->blocksize);
> 
> While running a world Azure CI build, I get these errors:
> 
> $ make clearfog_defconfig
> $ make -sj
> ...
> cmd/mvebu/bubt.c: In function 'check_image_header':
> cmd/mvebu/bubt.c:724:9: error: 'offset' undeclared (first use in this
> function); did you mean 'off_t'?
>   724 |         offset = le32_to_cpu(hdr->srcaddr);
>       |         ^~~~~~
>       |         off_t
> cmd/mvebu/bubt.c:724:9: note: each undeclared identifier is reported only
> once for each function it appears in
> cmd/mvebu/bubt.c:725:9: error: 'size' undeclared (first use in this
> function); did you mean 'size_t'?
>   725 |         size = le32_to_cpu(hdr->blocksize);
>       |         ^~~~
>       |         size_t
> make[2]: *** [scripts/Makefile.build:258: cmd/mvebu/bubt.o] Error 1
> make[1]: *** [scripts/Makefile.build:398: cmd/mvebu] Error 2
> make[1]: *** Waiting for unfinished jobs....
> make: *** [Makefile:1916: cmd] Error 2
> make: *** Waiting for unfinished jobs....
> 
> Could you please take a look and fix these issues?

It took me some time to find where is the issue, because I was not able
to reproduce it. But finally I was able to trigger it. The issue is that
changes are incorrectly split into patch 1/2 and 2/2. So if you apply
both patches, there is no issue.

I will try to fix it and send a v3.

> Thanks,
> Stefan
> 
> > +
> > +   if (hdr->blockid == 0x78) { /* SATA id */
> > +           if (offset < 1) {
> > +                   printf("Error: Bad A38x image srcaddr.\n");
> > +                   return -ENOEXEC;
> > +           }
> > +           offset -= 1;
> > +           offset *= 512;
> > +   }
> > +
> > +   if (hdr->blockid == 0xAE) /* SDIO id */
> > +           offset *= 512;
> > +
> > +   if (offset % 4 != 0 || size < 4 || size % 4 != 0) {
> > +           printf("Error: Bad A38x image blocksize.\n");
> > +           return -ENOEXEC;
> > +   }
> > +
> > +   checksum32 = image_checksum32((u8 *)hdr + offset, size - 4);
> > +   exp_checksum32 = *(u32 *)((u8 *)hdr + offset + size - 4);
> > +   if (checksum32 != exp_checksum32) {
> > +           printf("Error: Bad A38x image data checksum. 0x%08x != 
> > 0x%08x\n",
> > +                  checksum32, exp_checksum32);
> > +           return -ENOEXEC;
> > +   }
> > +
> >     printf("Image checksum...OK!\n");
> >     return 0;
> >   }
> 
> Viele Grüße,
> Stefan Roese
> 
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: s...@denx.de

Reply via email to