diff mbox series

[v2,1/3] vboot: add DTB policy for supporting multiple required conf keys

Message ID 0520ead9c2bac832480fbeb35c1ff9e8a464c61a.1595039992.git.thiruan@linux.microsoft.com
State Superseded
Delegated to: Tom Rini
Headers show
Series Add support for multiple required keys | expand

Commit Message

Thirupathaiah Annapureddy July 18, 2020, 3:20 a.m. UTC
Currently FIT image must be signed by all required conf keys. This means
Verified Boot fails if there is a signature verification failure
using any required key in U-boot DTB.

This patch introduces a new policy in DTB that can be set to any required
conf key. This means if verified boot passes with one of the required
keys, u-boot will continue the OS hand off.

There were prior attempts to address this:
https://lists.denx.de/pipermail/u-boot/2019-April/366047.html
The above patch was failing "make tests".
https://lists.denx.de/pipermail/u-boot/2020-January/396629.html

Signed-off-by: Thirupathaiah Annapureddy <thiruan@linux.microsoft.com>
---

Changes in v2:
- Modify fit_config_verify_required_sigs() to process required-mode
policy variable in U-boot DTB.

 common/image-fit-sig.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

Comments

Simon Glass July 28, 2020, 6:58 p.m. UTC | #1
Hi Thirupathaiah,

On Fri, 17 Jul 2020 at 21:20, Thirupathaiah Annapureddy
<thiruan@linux.microsoft.com> wrote:
>
> Currently FIT image must be signed by all required conf keys. This means
> Verified Boot fails if there is a signature verification failure
> using any required key in U-boot DTB.
>
> This patch introduces a new policy in DTB that can be set to any required
> conf key. This means if verified boot passes with one of the required
> keys, u-boot will continue the OS hand off.

U-Boot

>
> There were prior attempts to address this:
> https://lists.denx.de/pipermail/u-boot/2019-April/366047.html
> The above patch was failing "make tests".
> https://lists.denx.de/pipermail/u-boot/2020-January/396629.html
>
> Signed-off-by: Thirupathaiah Annapureddy <thiruan@linux.microsoft.com>
> ---
>
> Changes in v2:
> - Modify fit_config_verify_required_sigs() to process required-mode
> policy variable in U-boot DTB.
>
>  common/image-fit-sig.c | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Please see below

>
> diff --git a/common/image-fit-sig.c b/common/image-fit-sig.c
> index cc1967109e..d9b1c93a9b 100644
> --- a/common/image-fit-sig.c
> +++ b/common/image-fit-sig.c
> @@ -416,6 +416,10 @@ int fit_config_verify_required_sigs(const void *fit, int conf_noffset,
>  {
>         int noffset;
>         int sig_node;
> +       int verified = 0;
> +       int reqd_sigs = 0;
> +       bool reqd_policy_all = true;
> +       const char *reqd_mode;
>
>         /* Work out what we need to verify */
>         sig_node = fdt_subnode_offset(sig_blob, 0, FIT_SIG_NODENAME);
> @@ -425,6 +429,14 @@ int fit_config_verify_required_sigs(const void *fit, int conf_noffset,
>                 return 0;
>         }
>
> +       /* Get required-mode policy property from DTB */
> +       reqd_mode = fdt_getprop(sig_blob, sig_node, "required-mode", NULL);
> +       if (reqd_mode && !strcmp(reqd_mode, "any"))
> +               reqd_policy_all = false;
> +
> +       debug("%s: required-mode policy set to '%s'\n", __func__,
> +             reqd_policy_all ? "all" : "any");
> +
>         fdt_for_each_subnode(noffset, sig_blob, sig_node) {
>                 const char *required;
>                 int ret;
> @@ -433,15 +445,27 @@ int fit_config_verify_required_sigs(const void *fit, int conf_noffset,
>                                        NULL);
>                 if (!required || strcmp(required, "conf"))
>                         continue;
> +
> +               reqd_sigs++;
> +
>                 ret = fit_config_verify_sig(fit, conf_noffset, sig_blob,
>                                             noffset);
>                 if (ret) {
> -                       printf("Failed to verify required signature '%s'\n",
> -                              fit_get_name(sig_blob, noffset, NULL));
> -                       return ret;
> +                       if (reqd_policy_all) {
> +                               printf("Failed to verify required signature '%s'\n",
> +                                      fit_get_name(sig_blob, noffset, NULL));

I think we should keep this message outside the if(). Otherwise there
is no indication what is going on.

> +                               return ret;
> +                       }
> +               } else {
> +                       verified++;
> +                       if (!reqd_policy_all)
> +                               break;
>                 }
>         }
>
> +       if (reqd_sigs && !verified)

I think we need a message here, indicating that no signature was verified.


> +               return -EPERM;
> +
>         return 0;
>  }
>
> --
> 2.25.2
>
diff mbox series

Patch

diff --git a/common/image-fit-sig.c b/common/image-fit-sig.c
index cc1967109e..d9b1c93a9b 100644
--- a/common/image-fit-sig.c
+++ b/common/image-fit-sig.c
@@ -416,6 +416,10 @@  int fit_config_verify_required_sigs(const void *fit, int conf_noffset,
 {
 	int noffset;
 	int sig_node;
+	int verified = 0;
+	int reqd_sigs = 0;
+	bool reqd_policy_all = true;
+	const char *reqd_mode;
 
 	/* Work out what we need to verify */
 	sig_node = fdt_subnode_offset(sig_blob, 0, FIT_SIG_NODENAME);
@@ -425,6 +429,14 @@  int fit_config_verify_required_sigs(const void *fit, int conf_noffset,
 		return 0;
 	}
 
+	/* Get required-mode policy property from DTB */
+	reqd_mode = fdt_getprop(sig_blob, sig_node, "required-mode", NULL);
+	if (reqd_mode && !strcmp(reqd_mode, "any"))
+		reqd_policy_all = false;
+
+	debug("%s: required-mode policy set to '%s'\n", __func__,
+	      reqd_policy_all ? "all" : "any");
+
 	fdt_for_each_subnode(noffset, sig_blob, sig_node) {
 		const char *required;
 		int ret;
@@ -433,15 +445,27 @@  int fit_config_verify_required_sigs(const void *fit, int conf_noffset,
 				       NULL);
 		if (!required || strcmp(required, "conf"))
 			continue;
+
+		reqd_sigs++;
+
 		ret = fit_config_verify_sig(fit, conf_noffset, sig_blob,
 					    noffset);
 		if (ret) {
-			printf("Failed to verify required signature '%s'\n",
-			       fit_get_name(sig_blob, noffset, NULL));
-			return ret;
+			if (reqd_policy_all) {
+				printf("Failed to verify required signature '%s'\n",
+				       fit_get_name(sig_blob, noffset, NULL));
+				return ret;
+			}
+		} else {
+			verified++;
+			if (!reqd_policy_all)
+				break;
 		}
 	}
 
+	if (reqd_sigs && !verified)
+		return -EPERM;
+
 	return 0;
 }