Re: [PATCH 5/8] image: Adjust the workings of fit_check_format()

2021-02-17 Thread Jesper Schmitz Mouridsen 

On 17.02.2021 14.43, Tom Rini wrote:

On Wed, Feb 17, 2021 at 02:30:56PM +0100, Jesper Schmitz Mouridsen wrote:


Hi

Can you avoid the use of ENODATA since it is not defined in FreeBSD's

errno.h?

I like that FreeBSD has EDOOFUS, but I don't see an obvious replacement
for ENODATA, can you suggest something please?  Thanks!


Perhaps EBADMSG?

Re: [PATCH 5/8] image: Adjust the workings of fit_check_format()

2021-02-17 Thread Jesper Schmitz Mouridsen 

Hi

Can you avoid the use of ENODATA since it is not defined in FreeBSD's

errno.h?

Regards

Jesper Schmitz Mouridsen


On 16.02.2021 01.08, Simon Glass wrote:

At present this function does not accept a size for the FIT. This means
that it must be read from the FIT itself, introducing potential security
risk. Update the function to include a size parameter, which can be
invalid, in which case fit_check_format() calculates it.

For now no callers pass the size, but this can be updated later.

Also adjust the return value to an error code so that all the different
types of problems can be distinguished by the user.

Signed-off-by: Simon Glass 
Reported-by: Bruce Monroe 
Reported-by: Arie Haenel 
Reported-by: Julien Lenoir 
---

  arch/arm/cpu/armv8/sec_firmware.c  |  2 +-
  cmd/bootefi.c  |  2 +-
  cmd/bootm.c|  6 ++--
  cmd/disk.c |  2 +-
  cmd/fpga.c |  2 +-
  cmd/nand.c |  2 +-
  cmd/source.c   |  2 +-
  cmd/ximg.c |  2 +-
  common/image-fdt.c |  2 +-
  common/image-fit.c | 46 +-
  common/splash_source.c |  6 ++--
  common/update.c|  4 +--
  drivers/fpga/socfpga_arria10.c |  6 ++--
  drivers/net/fsl-mc/mc.c|  2 +-
  drivers/net/pfe_eth/pfe_firmware.c |  2 +-
  include/image.h| 21 +-
  tools/fit_common.c |  3 +-
  tools/fit_image.c  |  2 +-
  tools/mkimage.h|  2 ++
  19 files changed, 66 insertions(+), 50 deletions(-)

diff --git a/arch/arm/cpu/armv8/sec_firmware.c 
b/arch/arm/cpu/armv8/sec_firmware.c
index c6c4fcc7e07..267894fbcb3 100644
--- a/arch/arm/cpu/armv8/sec_firmware.c
+++ b/arch/arm/cpu/armv8/sec_firmware.c
@@ -317,7 +317,7 @@ __weak bool sec_firmware_is_valid(const void 
*sec_firmware_img)
return false;
}
  
-	if (!fit_check_format(sec_firmware_img)) {

+   if (fit_check_format(sec_firmware_img, IMAGE_SIZE_INVAL)) {
printf("SEC Firmware: Bad firmware image (bad FIT header)\n");
return false;
}
diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 1583a96be14..271b385edea 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -73,7 +73,7 @@ void efi_set_bootdev(const char *dev, const char *devnr, 
const char *path,
/* Remember only PE-COFF and FIT images */
if (efi_check_pe(buffer, buffer_size, NULL) != EFI_SUCCESS) {
  #ifdef CONFIG_FIT
-   if (!fit_check_format(buffer))
+   if (fit_check_format(buffer, IMAGE_SIZE_INVAL))
return;
/*
 * FIT images of type EFI_OS are started via command bootm.
diff --git a/cmd/bootm.c b/cmd/bootm.c
index 7732b97f635..81c6b939781 100644
--- a/cmd/bootm.c
+++ b/cmd/bootm.c
@@ -292,7 +292,7 @@ static int image_info(ulong addr)
case IMAGE_FORMAT_FIT:
puts("   FIT image found\n");
  
-		if (!fit_check_format(hdr)) {

+   if (fit_check_format(hdr, IMAGE_SIZE_INVAL)) {
puts("Bad FIT image format!\n");
unmap_sysmem(hdr);
return 1;
@@ -369,7 +369,7 @@ static int do_imls_nor(void)
  #endif
  #if defined(CONFIG_FIT)
case IMAGE_FORMAT_FIT:
-   if (!fit_check_format(hdr))
+   if (fit_check_format(hdr, IMAGE_SIZE_INVAL))
goto next_sector;
  
  printf("FIT Image at %08lX:\n", (ulong)hdr);

@@ -449,7 +449,7 @@ static int nand_imls_fitimage(struct mtd_info *mtd, int 
nand_dev, loff_t off,
return ret;
}
  
-	if (!fit_check_format(imgdata)) {

+   if (fit_check_format(imgdata, IMAGE_SIZE_INVAL)) {
free(imgdata);
return 0;
}
diff --git a/cmd/disk.c b/cmd/disk.c
index 0bc3808dfe2..2726115e855 100644
--- a/cmd/disk.c
+++ b/cmd/disk.c
@@ -114,7 +114,7 @@ int common_diskboot(struct cmd_tbl *cmdtp, const char 
*intf, int argc,
/* This cannot be done earlier,
 * we need complete FIT image in RAM first */
if (genimg_get_format((void *) addr) == IMAGE_FORMAT_FIT) {
-   if (!fit_check_format(fit_hdr)) {
+   if (fit_check_format(fit_hdr, IMAGE_SIZE_INVAL)) {
bootstage_error(BOOTSTAGE_ID_IDE_FIT_READ);
puts("** Bad FIT image format\n");
return 1;
diff --git a/cmd/fpga.c b/cmd/fpga.c
index 8ae1c936fbb..51410a8e424 100644
--- a/cmd/fpga.c
+++ b/cmd/fpga.c
@@ -330,7 +330,7 @@ static int do_fpga_loadmk(struct cmd_tbl *cmdtp, int flag, 
int argc,
return CMD_RET_FAILURE;
}
  
-		if (!fit_check_format(fit_hdr)) {

+   if

Re: [PATCH 5/8] image: Adjust the workings of fit_check_format()

2021-02-17 Thread Tom Rini 
On Wed, Feb 17, 2021 at 02:30:56PM +0100, Jesper Schmitz Mouridsen wrote:

> Hi
> 
> Can you avoid the use of ENODATA since it is not defined in FreeBSD's
> 
> errno.h?

I like that FreeBSD has EDOOFUS, but I don't see an obvious replacement
for ENODATA, can you suggest something please?  Thanks!

-- 
Tom


signature.asc
Description: PGP signature

Re: [PATCH 5/8] image: Adjust the workings of fit_check_format()

2021-02-15 Thread Tom Rini 
On Mon, Feb 15, 2021 at 05:08:09PM -0700, Simon Glass wrote:

> At present this function does not accept a size for the FIT. This means
> that it must be read from the FIT itself, introducing potential security
> risk. Update the function to include a size parameter, which can be
> invalid, in which case fit_check_format() calculates it.
> 
> For now no callers pass the size, but this can be updated later.
> 
> Also adjust the return value to an error code so that all the different
> types of problems can be distinguished by the user.
> 
> Signed-off-by: Simon Glass 
> Reported-by: Bruce Monroe 
> Reported-by: Arie Haenel 
> Reported-by: Julien Lenoir 

Applied to u-boot/master, thanks!

-- 
Tom


signature.asc
Description: PGP signature

[PATCH 5/8] image: Adjust the workings of fit_check_format()

2021-02-15 Thread Simon Glass 
At present this function does not accept a size for the FIT. This means
that it must be read from the FIT itself, introducing potential security
risk. Update the function to include a size parameter, which can be
invalid, in which case fit_check_format() calculates it.

For now no callers pass the size, but this can be updated later.

Also adjust the return value to an error code so that all the different
types of problems can be distinguished by the user.

Signed-off-by: Simon Glass 
Reported-by: Bruce Monroe 
Reported-by: Arie Haenel 
Reported-by: Julien Lenoir 
---

 arch/arm/cpu/armv8/sec_firmware.c  |  2 +-
 cmd/bootefi.c  |  2 +-
 cmd/bootm.c|  6 ++--
 cmd/disk.c |  2 +-
 cmd/fpga.c |  2 +-
 cmd/nand.c |  2 +-
 cmd/source.c   |  2 +-
 cmd/ximg.c |  2 +-
 common/image-fdt.c |  2 +-
 common/image-fit.c | 46 +-
 common/splash_source.c |  6 ++--
 common/update.c|  4 +--
 drivers/fpga/socfpga_arria10.c |  6 ++--
 drivers/net/fsl-mc/mc.c|  2 +-
 drivers/net/pfe_eth/pfe_firmware.c |  2 +-
 include/image.h| 21 +-
 tools/fit_common.c |  3 +-
 tools/fit_image.c  |  2 +-
 tools/mkimage.h|  2 ++
 19 files changed, 66 insertions(+), 50 deletions(-)

diff --git a/arch/arm/cpu/armv8/sec_firmware.c 
b/arch/arm/cpu/armv8/sec_firmware.c
index c6c4fcc7e07..267894fbcb3 100644
--- a/arch/arm/cpu/armv8/sec_firmware.c
+++ b/arch/arm/cpu/armv8/sec_firmware.c
@@ -317,7 +317,7 @@ __weak bool sec_firmware_is_valid(const void 
*sec_firmware_img)
return false;
}
 
-   if (!fit_check_format(sec_firmware_img)) {
+   if (fit_check_format(sec_firmware_img, IMAGE_SIZE_INVAL)) {
printf("SEC Firmware: Bad firmware image (bad FIT header)\n");
return false;
}
diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 1583a96be14..271b385edea 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -73,7 +73,7 @@ void efi_set_bootdev(const char *dev, const char *devnr, 
const char *path,
/* Remember only PE-COFF and FIT images */
if (efi_check_pe(buffer, buffer_size, NULL) != EFI_SUCCESS) {
 #ifdef CONFIG_FIT
-   if (!fit_check_format(buffer))
+   if (fit_check_format(buffer, IMAGE_SIZE_INVAL))
return;
/*
 * FIT images of type EFI_OS are started via command bootm.
diff --git a/cmd/bootm.c b/cmd/bootm.c
index 7732b97f635..81c6b939781 100644
--- a/cmd/bootm.c
+++ b/cmd/bootm.c
@@ -292,7 +292,7 @@ static int image_info(ulong addr)
case IMAGE_FORMAT_FIT:
puts("   FIT image found\n");
 
-   if (!fit_check_format(hdr)) {
+   if (fit_check_format(hdr, IMAGE_SIZE_INVAL)) {
puts("Bad FIT image format!\n");
unmap_sysmem(hdr);
return 1;
@@ -369,7 +369,7 @@ static int do_imls_nor(void)
 #endif
 #if defined(CONFIG_FIT)
case IMAGE_FORMAT_FIT:
-   if (!fit_check_format(hdr))
+   if (fit_check_format(hdr, IMAGE_SIZE_INVAL))
goto next_sector;
 
printf("FIT Image at %08lX:\n", (ulong)hdr);
@@ -449,7 +449,7 @@ static int nand_imls_fitimage(struct mtd_info *mtd, int 
nand_dev, loff_t off,
return ret;
}
 
-   if (!fit_check_format(imgdata)) {
+   if (fit_check_format(imgdata, IMAGE_SIZE_INVAL)) {
free(imgdata);
return 0;
}
diff --git a/cmd/disk.c b/cmd/disk.c
index 0bc3808dfe2..2726115e855 100644
--- a/cmd/disk.c
+++ b/cmd/disk.c
@@ -114,7 +114,7 @@ int common_diskboot(struct cmd_tbl *cmdtp, const char 
*intf, int argc,
/* This cannot be done earlier,
 * we need complete FIT image in RAM first */
if (genimg_get_format((void *) addr) == IMAGE_FORMAT_FIT) {
-   if (!fit_check_format(fit_hdr)) {
+   if (fit_check_format(fit_hdr, IMAGE_SIZE_INVAL)) {
bootstage_error(BOOTSTAGE_ID_IDE_FIT_READ);
puts("** Bad FIT image format\n");
return 1;
diff --git a/cmd/fpga.c b/cmd/fpga.c
index 8ae1c936fbb..51410a8e424 100644
--- a/cmd/fpga.c
+++ b/cmd/fpga.c
@@ -330,7 +330,7 @@ static int do_fpga_loadmk(struct cmd_tbl *cmdtp, int flag, 
int argc,
return CMD_RET_FAILURE;
}
 
-   if (!fit_check_format(fit_hdr)) {
+   if (fit_check_format(fit_hdr, IMAGE_SIZE_INVAL)) {
puts("Bad FIT image format\n");
return