| Message ID | 20220809194203.29903-1-pali@kernel.org |
|---|---|
| State | Superseded |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | [v2,1/2] cmd: mvebu/bubt: Check for A38x image data checksum | expand |
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 <pali@kernel.org> Reviewed-by: Stefan Roese <sr@denx.de> Thanks, Stefan > --- > 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); > + > + 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
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 <pali@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? 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
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 <pali@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: sr@denx.de
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); + + 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; }
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 <pali@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(-)