All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Enable strict signature verification for FIT
@ 2021-09-16 13:09 Oleksandr Suvorov
  2021-09-16 13:09 ` [PATCH 1/2] spl: Add CONFIG_SPL_FIT_SIGNATURE_STRICT Oleksandr Suvorov
  0 siblings, 1 reply; 6+ messages in thread
From: Oleksandr Suvorov @ 2021-09-16 13:09 UTC (permalink / raw)
  To: u-boot
  Cc: Oleksandr Suvorov, Alexandru Gagniuc, Bin Meng, Henry Beberman,
	Klaus Heinrich Kiwi, Marek Vasut, Masahisa Kojima, Michal Simek,
	Philippe Reynes, Ricardo Salveti, Simon Glass, Steffen Jaeckel


FIT load checks the signature on loadable images, but just continues
in the case of a failure. This is undesirable behavior because the boot
process depends on the authenticity of every loadable part.

Add a check that verifies the FIT's configuration block, and fails if
it's not present or the signature doesn't match.


Henry Beberman (1):
  spl: Add CONFIG_SPL_FIT_SIGNATURE_STRICT

Ricardo Salveti (1):
  cmd: Add CONFIG_FIT_SIGNATURE_STRICT

 cmd/fpga.c           | 14 ++++++++++++++
 cmd/source.c         | 14 ++++++++++++++
 cmd/ximg.c           | 14 ++++++++++++++
 common/Kconfig.boot  | 11 +++++++++++
 common/spl/spl_fit.c | 21 ++++++++++++++++++++-
 5 files changed, 73 insertions(+), 1 deletion(-)

-- 
2.31.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] spl: Add CONFIG_SPL_FIT_SIGNATURE_STRICT
  2021-09-16 13:09 [PATCH 0/2] Enable strict signature verification for FIT Oleksandr Suvorov
@ 2021-09-16 13:09 ` Oleksandr Suvorov
  2021-09-16 13:09   ` [PATCH 2/2] cmd: Add CONFIG_FIT_SIGNATURE_STRICT Oleksandr Suvorov
  2021-09-16 18:30   ` [PATCH 1/2] spl: Add CONFIG_SPL_FIT_SIGNATURE_STRICT Alex G.
  0 siblings, 2 replies; 6+ messages in thread
From: Oleksandr Suvorov @ 2021-09-16 13:09 UTC (permalink / raw)
  To: u-boot
  Cc: Henry Beberman, Ricardo Salveti, Oleksandr Suvorov,
	Alexandru Gagniuc, Bin Meng, Klaus Heinrich Kiwi, Marek Vasut,
	Michal Simek, Philippe Reynes, Simon Glass, Steffen Jaeckel

From: Henry Beberman <hebeberm@microsoft.com>

SPL FIT load checks the signature on loadable images but just continues
in the case of a failure. This is undesirable behavior because the boot
process depends on the authenticity of each loadable part.

Adding CONFIG_SPL_FIT_SIGNATURE_STRICT to halt the platform when any
image fails its signature check, including loadable parts.

SPL already supports image signature verification but had no mechanism
to check that the FIT's configuration block was signed correctly.

Add a check near the start of spl_load_simple_fit that verifies the
FIT's configuration block, and fails if it's not present or the
signature doesn't match what's stored in the SPL DTB.

Signed-off-by: Henry Beberman <hebeberm@microsoft.com>
Signed-off-by: Ricardo Salveti <ricardo@foundries.io>
Co-developed-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
---

 common/Kconfig.boot  |  7 +++++++
 common/spl/spl_fit.c | 21 ++++++++++++++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/common/Kconfig.boot b/common/Kconfig.boot
index 902a5b8fbea..6f95d009dfa 100644
--- a/common/Kconfig.boot
+++ b/common/Kconfig.boot
@@ -166,6 +166,13 @@ config SPL_FIT_SIGNATURE
 	select SPL_IMAGE_SIGN_INFO
 	select SPL_FIT_FULL_CHECK
 
+config SPL_FIT_SIGNATURE_STRICT
+	bool "Halt if loadables or firmware don't pass FIT signature verification"
+	select SPL_FIT_SIGNATURE
+	help
+	  Strictly requires each loadable or firmware in a FIT image to be
+	  passed verification. Halt if any loadable fails to be verified.
+
 config SPL_LOAD_FIT
 	bool "Enable SPL loading U-Boot as a FIT (basic fitImage features)"
 	select SPL_FIT
diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index f41abca0ccb..e7eaaa4cb9e 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -315,7 +315,12 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
 		printf("## Checking hash(es) for Image %s ... ",
 		       fit_get_name(fit, node, NULL));
 		if (!fit_image_verify_with_data(fit, node, src, length))
-			return -EPERM;
+			if (CONFIG_IS_ENABLED(FIT_SIGNATURE_STRICT)) {
+				puts("Invalid FIT signature found in a required image.\n");
+				hang();
+			} else {
+				return -EPERM;
+			}
 		puts("OK\n");
 	}
 
@@ -681,6 +686,20 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	if (ret < 0)
 		return ret;
 
+	if (CONFIG_IS_ENABLED(FIT_SIGNATURE_STRICT)) {
+		int cfg_noffset = fit_conf_get_node(fit, NULL);
+
+		if (cfg_noffset >= 0) {
+			if (fit_config_verify(fit, cfg_noffset)) {
+				puts("Unable to verify the required FIT config.\n");
+				hang();
+			}
+		} else {
+			puts("SPL_FIT_SIGNATURE_STRICT needs a config node in FIT\n");
+			hang();
+		}
+	}
+
 	/* skip further processing if requested to enable load-only use cases */
 	if (spl_load_simple_fit_skip_processing())
 		return 0;
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] cmd: Add CONFIG_FIT_SIGNATURE_STRICT
  2021-09-16 13:09 ` [PATCH 1/2] spl: Add CONFIG_SPL_FIT_SIGNATURE_STRICT Oleksandr Suvorov
@ 2021-09-16 13:09   ` Oleksandr Suvorov
  2021-09-16 15:09     ` Igor Opaniuk
  2021-09-16 18:30   ` [PATCH 1/2] spl: Add CONFIG_SPL_FIT_SIGNATURE_STRICT Alex G.
  1 sibling, 1 reply; 6+ messages in thread
From: Oleksandr Suvorov @ 2021-09-16 13:09 UTC (permalink / raw)
  To: u-boot
  Cc: Ricardo Salveti, Oleksandr Suvorov, Alexandru Gagniuc, Bin Meng,
	Klaus Heinrich Kiwi, Masahisa Kojima, Michal Simek, Simon Glass,
	Steffen Jaeckel

From: Ricardo Salveti <ricardo@foundries.io>

Add CONFIG_FIT_SIGNATURE_STRICT to require a valid FIT configuration
signature for each command that is able to manipulate FIT images.

Signed-off-by: Ricardo Salveti <ricardo@foundries.io>
Co-developed-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
---

 cmd/fpga.c          | 14 ++++++++++++++
 cmd/source.c        | 14 ++++++++++++++
 cmd/ximg.c          | 14 ++++++++++++++
 common/Kconfig.boot |  4 ++++
 4 files changed, 46 insertions(+)

diff --git a/cmd/fpga.c b/cmd/fpga.c
index 3fdd0b35e80..16d329590fa 100644
--- a/cmd/fpga.c
+++ b/cmd/fpga.c
@@ -335,6 +335,20 @@ static int do_fpga_loadmk(struct cmd_tbl *cmdtp, int flag, int argc,
 			return CMD_RET_FAILURE;
 		}
 
+		if (CONFIG_IS_ENABLED(FIT_SIGNATURE_STRICT)) {
+			/* validate required fit config entry */
+			noffset = fit_conf_get_node(fit_hdr, NULL);
+			if (noffset >= 0) {
+				if (fit_config_verify(fit_hdr, noffset)) {
+					puts("Cannot verify FIT config node\n");
+					return CMD_RET_FAILURE;
+				}
+			} else {
+				puts("FIT_SIGNATURE_STRICT requires a config node\n");
+				return CMD_RET_FAILURE;
+			}
+		}
+
 		/* get fpga component image node offset */
 		noffset = fit_image_get_node(fit_hdr, fit_uname);
 		if (noffset < 0) {
diff --git a/cmd/source.c b/cmd/source.c
index 81e015b64ef..b08406dfcbf 100644
--- a/cmd/source.c
+++ b/cmd/source.c
@@ -112,6 +112,20 @@ int image_source_script(ulong addr, const char *fit_uname)
 			return 1;
 		}
 
+		if (CONFIG_IS_ENABLED(FIT_SIGNATURE_STRICT)) {
+			/* validate required fit config entry */
+			noffset = fit_conf_get_node(fit_hdr, NULL);
+			if (noffset >= 0) {
+				if (fit_config_verify(fit_hdr, noffset)) {
+					puts("Cannot verify FIT config node\n");
+					return 1;
+				}
+			} else {
+				puts("FIT_SIGNATURE_STRICT requires a config node\n");
+				return 1;
+			}
+		}
+
 		if (!fit_uname)
 			fit_uname = get_default_image(fit_hdr);
 
diff --git a/cmd/ximg.c b/cmd/ximg.c
index 65ba41320a0..39fccd8179c 100644
--- a/cmd/ximg.c
+++ b/cmd/ximg.c
@@ -141,6 +141,20 @@ do_imgextract(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 			return 1;
 		}
 
+		if (CONFIG_IS_ENABLED(FIT_SIGNATURE_STRICT)) {
+			/* validate required fit config entry */
+			noffset = fit_conf_get_node(fit_hdr, NULL);
+			if (noffset >= 0) {
+				if (fit_config_verify(fit_hdr, noffset)) {
+					puts("Cannot verify FIT config node\n");
+					return 1;
+				}
+			} else {
+				puts("FIT_SIGNATURE_STRICT requires a config node\n");
+				return 1;
+			}
+		}
+
 		/* get subimage node offset */
 		noffset = fit_image_get_node(fit_hdr, uname);
 		if (noffset < 0) {
diff --git a/common/Kconfig.boot b/common/Kconfig.boot
index 6f95d009dfa..ca7d9a8d971 100644
--- a/common/Kconfig.boot
+++ b/common/Kconfig.boot
@@ -77,6 +77,10 @@ config FIT_SIGNATURE_MAX_SIZE
 	  device memory. Assure this size does not extend past expected storage
 	  space.
 
+config FIT_SIGNATURE_STRICT
+	bool "Requires a valid FIT configuration signature for every image"
+	select FIT_SIGNATURE
+
 config FIT_RSASSA_PSS
 	bool "Support rsassa-pss signature scheme of FIT image contents"
 	depends on FIT_SIGNATURE
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] cmd: Add CONFIG_FIT_SIGNATURE_STRICT
  2021-09-16 13:09   ` [PATCH 2/2] cmd: Add CONFIG_FIT_SIGNATURE_STRICT Oleksandr Suvorov
@ 2021-09-16 15:09     ` Igor Opaniuk
  2021-09-16 17:54       ` Oleksandr Suvorov
  0 siblings, 1 reply; 6+ messages in thread
From: Igor Opaniuk @ 2021-09-16 15:09 UTC (permalink / raw)
  To: Oleksandr Suvorov
  Cc: U-Boot Mailing List, Ricardo Salveti, Alexandru Gagniuc,
	Bin Meng, Klaus Heinrich Kiwi, Masahisa Kojima, Michal Simek,
	Simon Glass, Steffen Jaeckel

Hi Oleksandr,

On Thu, Sep 16, 2021 at 4:10 PM Oleksandr Suvorov
<oleksandr.suvorov@foundries.io> wrote:
>
> From: Ricardo Salveti <ricardo@foundries.io>
>
> Add CONFIG_FIT_SIGNATURE_STRICT to require a valid FIT configuration
> signature for each command that is able to manipulate FIT images.
>
> Signed-off-by: Ricardo Salveti <ricardo@foundries.io>
> Co-developed-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
> Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
> ---
>
>  cmd/fpga.c          | 14 ++++++++++++++
>  cmd/source.c        | 14 ++++++++++++++
>  cmd/ximg.c          | 14 ++++++++++++++
>  common/Kconfig.boot |  4 ++++
>  4 files changed, 46 insertions(+)
>
> diff --git a/cmd/fpga.c b/cmd/fpga.c
> index 3fdd0b35e80..16d329590fa 100644
> --- a/cmd/fpga.c
> +++ b/cmd/fpga.c
> @@ -335,6 +335,20 @@ static int do_fpga_loadmk(struct cmd_tbl *cmdtp, int flag, int argc,
>                         return CMD_RET_FAILURE;
>                 }
>
> +               if (CONFIG_IS_ENABLED(FIT_SIGNATURE_STRICT)) {
> +                       /* validate required fit config entry */
> +                       noffset = fit_conf_get_node(fit_hdr, NULL);
> +                       if (noffset >= 0) {
> +                               if (fit_config_verify(fit_hdr, noffset)) {
> +                                       puts("Cannot verify FIT config node\n");
> +                                       return CMD_RET_FAILURE;
> +                               }
> +                       } else {
> +                               puts("FIT_SIGNATURE_STRICT requires a config node\n");
> +                               return CMD_RET_FAILURE;
> +                       }
> +               }
> +
>                 /* get fpga component image node offset */
>                 noffset = fit_image_get_node(fit_hdr, fit_uname);
>                 if (noffset < 0) {
> diff --git a/cmd/source.c b/cmd/source.c
> index 81e015b64ef..b08406dfcbf 100644
> --- a/cmd/source.c
> +++ b/cmd/source.c
> @@ -112,6 +112,20 @@ int image_source_script(ulong addr, const char *fit_uname)
>                         return 1;
>                 }
>
> +               if (CONFIG_IS_ENABLED(FIT_SIGNATURE_STRICT)) {
> +                       /* validate required fit config entry */
> +                       noffset = fit_conf_get_node(fit_hdr, NULL);
> +                       if (noffset >= 0) {
> +                               if (fit_config_verify(fit_hdr, noffset)) {
> +                                       puts("Cannot verify FIT config node\n");
> +                                       return 1;
> +                               }
> +                       } else {
> +                               puts("FIT_SIGNATURE_STRICT requires a config node\n");
> +                               return 1;
> +                       }
> +               }
> +
>                 if (!fit_uname)
>                         fit_uname = get_default_image(fit_hdr);
>
> diff --git a/cmd/ximg.c b/cmd/ximg.c
> index 65ba41320a0..39fccd8179c 100644
> --- a/cmd/ximg.c
> +++ b/cmd/ximg.c
> @@ -141,6 +141,20 @@ do_imgextract(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>                         return 1;
>                 }
>
> +               if (CONFIG_IS_ENABLED(FIT_SIGNATURE_STRICT)) {
> +                       /* validate required fit config entry */
> +                       noffset = fit_conf_get_node(fit_hdr, NULL);
> +                       if (noffset >= 0) {
> +                               if (fit_config_verify(fit_hdr, noffset)) {
> +                                       puts("Cannot verify FIT config node\n");
> +                                       return 1;
> +                               }
> +                       } else {
> +                               puts("FIT_SIGNATURE_STRICT requires a config node\n");
> +                               return 1;
> +                       }
> +               }
> +
>                 /* get subimage node offset */
>                 noffset = fit_image_get_node(fit_hdr, uname);
>                 if (noffset < 0) {
> diff --git a/common/Kconfig.boot b/common/Kconfig.boot
> index 6f95d009dfa..ca7d9a8d971 100644
> --- a/common/Kconfig.boot
> +++ b/common/Kconfig.boot
> @@ -77,6 +77,10 @@ config FIT_SIGNATURE_MAX_SIZE
>           device memory. Assure this size does not extend past expected storage
>           space.
>
> +config FIT_SIGNATURE_STRICT
> +       bool "Requires a valid FIT configuration signature for every image"
> +       select FIT_SIGNATURE
> +
>  config FIT_RSASSA_PSS
>         bool "Support rsassa-pss signature scheme of FIT image contents"
>         depends on FIT_SIGNATURE
> --
> 2.31.1
>

There is duplication of the same piece of code in 3 different files,
maybe you could rework
that and move to some common function?

-- 
Best regards - Freundliche Grüsse - Meilleures salutations

Igor Opaniuk
Embedded Software Engineer
T:  +380 938364067
E: igor.opaniuk@foundries.io
W: www.foundries.io

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] cmd: Add CONFIG_FIT_SIGNATURE_STRICT
  2021-09-16 15:09     ` Igor Opaniuk
@ 2021-09-16 17:54       ` Oleksandr Suvorov
  0 siblings, 0 replies; 6+ messages in thread
From: Oleksandr Suvorov @ 2021-09-16 17:54 UTC (permalink / raw)
  To: Igor Opaniuk
  Cc: U-Boot Mailing List, Ricardo Salveti, Alexandru Gagniuc,
	Bin Meng, Klaus Heinrich Kiwi, Masahisa Kojima, Michal Simek,
	Simon Glass, Steffen Jaeckel

Hi Igor,

On Thu, Sep 16, 2021 at 6:09 PM Igor Opaniuk <igor.opaniuk@foundries.io> wrote:
>
> Hi Oleksandr,
>
> On Thu, Sep 16, 2021 at 4:10 PM Oleksandr Suvorov
> <oleksandr.suvorov@foundries.io> wrote:
> >
> > From: Ricardo Salveti <ricardo@foundries.io>
> >
> > Add CONFIG_FIT_SIGNATURE_STRICT to require a valid FIT configuration
> > signature for each command that is able to manipulate FIT images.
> >
> > Signed-off-by: Ricardo Salveti <ricardo@foundries.io>
> > Co-developed-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
> > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
> > ---
> >
> >  cmd/fpga.c          | 14 ++++++++++++++
> >  cmd/source.c        | 14 ++++++++++++++
> >  cmd/ximg.c          | 14 ++++++++++++++
> >  common/Kconfig.boot |  4 ++++
> >  4 files changed, 46 insertions(+)
> >
> > diff --git a/cmd/fpga.c b/cmd/fpga.c
> > index 3fdd0b35e80..16d329590fa 100644
> > --- a/cmd/fpga.c
> > +++ b/cmd/fpga.c
> > @@ -335,6 +335,20 @@ static int do_fpga_loadmk(struct cmd_tbl *cmdtp, int flag, int argc,
> >                         return CMD_RET_FAILURE;
> >                 }
> >
> > +               if (CONFIG_IS_ENABLED(FIT_SIGNATURE_STRICT)) {
> > +                       /* validate required fit config entry */
> > +                       noffset = fit_conf_get_node(fit_hdr, NULL);
> > +                       if (noffset >= 0) {
> > +                               if (fit_config_verify(fit_hdr, noffset)) {
> > +                                       puts("Cannot verify FIT config node\n");
> > +                                       return CMD_RET_FAILURE;
> > +                               }
> > +                       } else {
> > +                               puts("FIT_SIGNATURE_STRICT requires a config node\n");
> > +                               return CMD_RET_FAILURE;
> > +                       }
> > +               }
> > +
> >                 /* get fpga component image node offset */
> >                 noffset = fit_image_get_node(fit_hdr, fit_uname);
> >                 if (noffset < 0) {
> > diff --git a/cmd/source.c b/cmd/source.c
> > index 81e015b64ef..b08406dfcbf 100644
> > --- a/cmd/source.c
> > +++ b/cmd/source.c
> > @@ -112,6 +112,20 @@ int image_source_script(ulong addr, const char *fit_uname)
> >                         return 1;
> >                 }
> >
> > +               if (CONFIG_IS_ENABLED(FIT_SIGNATURE_STRICT)) {
> > +                       /* validate required fit config entry */
> > +                       noffset = fit_conf_get_node(fit_hdr, NULL);
> > +                       if (noffset >= 0) {
> > +                               if (fit_config_verify(fit_hdr, noffset)) {
> > +                                       puts("Cannot verify FIT config node\n");
> > +                                       return 1;
> > +                               }
> > +                       } else {
> > +                               puts("FIT_SIGNATURE_STRICT requires a config node\n");
> > +                               return 1;
> > +                       }
> > +               }
> > +
> >                 if (!fit_uname)
> >                         fit_uname = get_default_image(fit_hdr);
> >
> > diff --git a/cmd/ximg.c b/cmd/ximg.c
> > index 65ba41320a0..39fccd8179c 100644
> > --- a/cmd/ximg.c
> > +++ b/cmd/ximg.c
> > @@ -141,6 +141,20 @@ do_imgextract(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> >                         return 1;
> >                 }
> >
> > +               if (CONFIG_IS_ENABLED(FIT_SIGNATURE_STRICT)) {
> > +                       /* validate required fit config entry */
> > +                       noffset = fit_conf_get_node(fit_hdr, NULL);
> > +                       if (noffset >= 0) {
> > +                               if (fit_config_verify(fit_hdr, noffset)) {
> > +                                       puts("Cannot verify FIT config node\n");
> > +                                       return 1;
> > +                               }
> > +                       } else {
> > +                               puts("FIT_SIGNATURE_STRICT requires a config node\n");
> > +                               return 1;
> > +                       }
> > +               }
> > +
> >                 /* get subimage node offset */
> >                 noffset = fit_image_get_node(fit_hdr, uname);
> >                 if (noffset < 0) {
> > diff --git a/common/Kconfig.boot b/common/Kconfig.boot
> > index 6f95d009dfa..ca7d9a8d971 100644
> > --- a/common/Kconfig.boot
> > +++ b/common/Kconfig.boot
> > @@ -77,6 +77,10 @@ config FIT_SIGNATURE_MAX_SIZE
> >           device memory. Assure this size does not extend past expected storage
> >           space.
> >
> > +config FIT_SIGNATURE_STRICT
> > +       bool "Requires a valid FIT configuration signature for every image"
> > +       select FIT_SIGNATURE
> > +
> >  config FIT_RSASSA_PSS
> >         bool "Support rsassa-pss signature scheme of FIT image contents"
> >         depends on FIT_SIGNATURE
> > --
> > 2.31.1
> >
>
> There is duplication of the same piece of code in 3 different files,
> maybe you could rework
> that and move to some common function?

Yeah, makes sense. I'm reworking this patch. Thanks!

> --
> Best regards - Freundliche Grüsse - Meilleures salutations
>
> Igor Opaniuk
> Embedded Software Engineer
> T:  +380 938364067
> E: igor.opaniuk@foundries.io
> W: www.foundries.io



-- 
Best regards,

Oleksandr Suvorov
Software Engineer
W: www.foundries.io

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] spl: Add CONFIG_SPL_FIT_SIGNATURE_STRICT
  2021-09-16 13:09 ` [PATCH 1/2] spl: Add CONFIG_SPL_FIT_SIGNATURE_STRICT Oleksandr Suvorov
  2021-09-16 13:09   ` [PATCH 2/2] cmd: Add CONFIG_FIT_SIGNATURE_STRICT Oleksandr Suvorov
@ 2021-09-16 18:30   ` Alex G.
  1 sibling, 0 replies; 6+ messages in thread
From: Alex G. @ 2021-09-16 18:30 UTC (permalink / raw)
  To: Oleksandr Suvorov, u-boot
  Cc: Henry Beberman, Ricardo Salveti, Bin Meng, Klaus Heinrich Kiwi,
	Marek Vasut, Michal Simek, Philippe Reynes, Simon Glass,
	Steffen Jaeckel

Hi Oleksandr

On 9/16/21 8:09 AM, Oleksandr Suvorov wrote:
> From: Henry Beberman <hebeberm@microsoft.com>
> 
> SPL FIT load checks the signature on loadable images but just continues
> in the case of a failure. This is undesirable behavior because the boot
> process depends on the authenticity of each loadable part.
> 
> Adding CONFIG_SPL_FIT_SIGNATURE_STRICT to halt the platform when any
> image fails its signature check, including loadable parts.

This sounds very similar to what FIT config verification already does, 
so I don't understand the motivation behind this patch.

> SPL already supports image signature verification but had no mechanism
> to check that the FIT's configuration block was signed correctly.

This statement is incorrect. This is exactly what required = "conf";
in u-boot's devicetree is intended to do.

NAK

> Add a check near the start of spl_load_simple_fit that verifies the
> FIT's configuration block, and fails if it's not present or the
> signature doesn't match what's stored in the SPL DTB.
> 
> Signed-off-by: Henry Beberman <hebeberm@microsoft.com>
> Signed-off-by: Ricardo Salveti <ricardo@foundries.io>
> Co-developed-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
> Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
> ---
> 
>   common/Kconfig.boot  |  7 +++++++
>   common/spl/spl_fit.c | 21 ++++++++++++++++++++-
>   2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/common/Kconfig.boot b/common/Kconfig.boot
> index 902a5b8fbea..6f95d009dfa 100644
> --- a/common/Kconfig.boot
> +++ b/common/Kconfig.boot
> @@ -166,6 +166,13 @@ config SPL_FIT_SIGNATURE
>   	select SPL_IMAGE_SIGN_INFO
>   	select SPL_FIT_FULL_CHECK
>   
> +config SPL_FIT_SIGNATURE_STRICT
> +	bool "Halt if loadables or firmware don't pass FIT signature verification"
> +	select SPL_FIT_SIGNATURE
> +	help
> +	  Strictly requires each loadable or firmware in a FIT image to be
> +	  passed verification. Halt if any loadable fails to be verified.
> +
>   config SPL_LOAD_FIT
>   	bool "Enable SPL loading U-Boot as a FIT (basic fitImage features)"
>   	select SPL_FIT
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> index f41abca0ccb..e7eaaa4cb9e 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -315,7 +315,12 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
>   		printf("## Checking hash(es) for Image %s ... ",
>   		       fit_get_name(fit, node, NULL));
>   		if (!fit_image_verify_with_data(fit, node, src, length))
> -			return -EPERM;
> +			if (CONFIG_IS_ENABLED(FIT_SIGNATURE_STRICT)) {
> +				puts("Invalid FIT signature found in a required image.\n");
> +				hang();

hang() is rarely the appropriate thing to do. Once you get -EPERM you 
could choose to either
   a) drop to a lower privilege level
   b) enter some sort of recovery mode
   c) outright hang.

spl_load_fit_image() isn't the right place to decide (a) or (b), so by 
extension, it's the wrong place to decide (c).

> +			} else {
> +				return -EPERM;
> +			}
>   		puts("OK\n");
>   	}
>   
> @@ -681,6 +686,20 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
>   	if (ret < 0)
>   		return ret;
>   
> +	if (CONFIG_IS_ENABLED(FIT_SIGNATURE_STRICT)) {
> +		int cfg_noffset = fit_conf_get_node(fit, NULL);
> +
> +		if (cfg_noffset >= 0) {
> +			if (fit_config_verify(fit, cfg_noffset)) {
> +				puts("Unable to verify the required FIT config.\n");
> +				hang();
> +			}
> +		} else {
> +			puts("SPL_FIT_SIGNATURE_STRICT needs a config node in FIT\n");
> +			hang();
> +		}
> +	}
> +
>   	/* skip further processing if requested to enable load-only use cases */
>   	if (spl_load_simple_fit_skip_processing())
>   		return 0;
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-09-16 18:30 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-16 13:09 [PATCH 0/2] Enable strict signature verification for FIT Oleksandr Suvorov
2021-09-16 13:09 ` [PATCH 1/2] spl: Add CONFIG_SPL_FIT_SIGNATURE_STRICT Oleksandr Suvorov
2021-09-16 13:09   ` [PATCH 2/2] cmd: Add CONFIG_FIT_SIGNATURE_STRICT Oleksandr Suvorov
2021-09-16 15:09     ` Igor Opaniuk
2021-09-16 17:54       ` Oleksandr Suvorov
2021-09-16 18:30   ` [PATCH 1/2] spl: Add CONFIG_SPL_FIT_SIGNATURE_STRICT Alex G.

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.