[RFC PATCH 0/1] Anti rollback protection for FIT Images

[RFC PATCH 0/1] Anti rollback protection for FIT Images

[Thirupathaiah Annapureddy]

Anti rollback protection is required when there is a need to retire
previous versions of FIT images due to security flaws in them.
Currently U-Boot Verified boot does not have rollback protection to
protect against known security flaws.

This RFC introduces a proposal to add anti-rollback protection for
FIT images. This protection feature prevents U-Boot from accepting
an image if it has ever successfully loaded an image with a larger
anti-rollback version number.

Each sub-image node inside /images node has an
anti-rollback version number(arbvn) similar to rollback_index in
Android Verified Boot. This version number is part of signed data and
it is incremented as security flaws are discovered and fixed.
U-Boot stores the last seen arbvn for a given image type in platform
specific tamper-evident storage.

As part of signature verification, U-Boot enfroces arvbn based
protection if enabled. arvbn stored in secure storage is validated with
arbvn in the sub-image node. If the counter in the FIT image is lower than
the counter in platform secure storage, image validation has failed
i.e. verified boot failed. If both counters match or the image counter is
higher than that in the platform secure storage, the image validation is
successful. In the higher case, U-Boot stores the new counter in platform
secure storage.

Pseudo code is as follows:

ret = board_get_arbvn(type, &plat_arbvn);
...
if (image_arbvn < plat_arbvn) {
	return -EPERM;
} else if (image_arbvn > plat_arbvn) {
	ret = board_set_arbvn(type, image_arbvn);
	return ret;
} else {
	return 0;
}

The following board specific hooks are required to get/set arbvn
from platform specific tamper-evident storage.
int board_get_arbvn(uint8_t ih_type, uint32_t *arbvn);
int board_set_arbvn(uint8_t ih_type, uint32_t arbvn);

As an example, consider this FIT:
/ {
	images {
		kernel-1 {
			data = <data for kernel1>
			arbvn = <1>;
			hash-1 {
				algo = "sha1";
				value = <...kernel hash 1...>
			};
		};
		fdt-1 {
			data = <data for fdt1>;
			hash-1 {
				algo = "sha1";
				value = <...fdt hash 1...>
			};
		};
	};
	configurations {
		default = "conf-1";
		conf-1 {
			kernel = "kernel-1";
			fdt = "fdt-1";
			signature-1 {
				algo = "sha1,rsa2048";
				sign-images = "fdt", "kernel";
				value = <...conf 1 signature...>;
			};
		};
	};
};

In the above example, kernel-1 image has an arbvn of 1.
if plat_arbvn is 1, the system will boot with this FIT image.
if plat_arbvn is 2 or more, U-Boot will prevent the system from booting
with this FIT image.

Thirupathaiah Annapureddy (1):
  image: add anti rollback protection for FIT Images

 Kconfig                |  9 +++++
 common/image-fit-sig.c | 79 ++++++++++++++++++++++++++++++++++++++++++
 common/image-fit.c     | 24 +++++++++++++
 include/image.h        | 23 ++++++++++++
 4 files changed, 135 insertions(+)

-- 
2.25.2

[RFC PATCH 1/1] image: add anti rollback protection for FIT Images

[Thirupathaiah Annapureddy]

Anti rollback protection is required when there is a need to retire
previous versions of FIT images due to security flaws in them.
Currently U-Boot Verified boot does not have rollback protection to
protect against known security flaws.

This change adds anti-rollback protection for FIT images.

Signed-off-by: Thirupathaiah Annapureddy <thiruan@linux.microsoft.com>
---
 Kconfig                |  9 +++++
 common/image-fit-sig.c | 79 ++++++++++++++++++++++++++++++++++++++++++
 common/image-fit.c     | 24 +++++++++++++
 include/image.h        | 23 ++++++++++++
 4 files changed, 135 insertions(+)

diff --git a/Kconfig b/Kconfig
index 883e3f71d0..3959a6592c 100644
--- a/Kconfig
+++ b/Kconfig
@@ -533,6 +533,15 @@ config FIT_CIPHER
 	  Enable the feature of data ciphering/unciphering in the tool mkimage
 	  and in the u-boot support of the FIT image.
 
+config FIT_ARBP
+	bool "Enable Anti rollback version check for FIT images"
+	depends on FIT_SIGNATURE
+	default n
+	help
+	  Enable this to activate anti-rollback protection. This is required
+	  when a platform needs to retire previous versions of FIT images due to
+	  security flaws in in them.
+
 config FIT_VERBOSE
 	bool "Show verbose messages when FIT images fail"
 	help
diff --git a/common/image-fit-sig.c b/common/image-fit-sig.c
index cc1967109e..5103508894 100644
--- a/common/image-fit-sig.c
+++ b/common/image-fit-sig.c
@@ -78,6 +78,35 @@ struct image_region *fit_region_make_list(const void *fit,
 	return region;
 }
 
+#if !defined(USE_HOSTCC)
+static int fit_image_verify_arbvn(const void *fit, int image_noffset)
+{
+	uint8_t type;
+	uint32_t image_arbvn;
+	uint32_t plat_arbvn = 0;
+	int ret;
+
+	ret = fit_image_get_arbvn(fit, image_noffset, &image_arbvn);
+	if (ret)
+		return 0;
+
+	ret = fit_image_get_type(fit, image_noffset, &type);
+	if (ret)
+		return 0;
+
+	ret = board_get_arbvn(type, &plat_arbvn);
+
+	if (image_arbvn < plat_arbvn) {
+		return -EPERM;
+	} else if (image_arbvn > plat_arbvn) {
+		ret = board_set_arbvn(type, image_arbvn);
+		return ret;
+	}
+
+	return 0;
+}
+#endif
+
 static int fit_image_setup_verify(struct image_sign_info *info,
 				  const void *fit, int noffset,
 				  int required_keynode, char **err_msgp)
@@ -181,6 +210,14 @@ static int fit_image_verify_sig(const void *fit, int image_noffset,
 		goto error;
 	}
 
+#if !defined(USE_HOSTCC)
+	if (IMAGE_ENABLE_ARBP && verified) {
+		ret = fit_image_verify_arbvn(fit, image_noffset);
+		if (ret)
+			verified = 0;
+	}
+#endif
+
 	return verified ? 0 : -EPERM;
 
 error:
@@ -370,6 +407,40 @@ static int fit_config_check_sig(const void *fit, int noffset,
 	return 0;
 }
 
+#if !defined(USE_HOSTCC)
+static int fit_config_verify_arbvn(const void *fit, int conf_noffset,
+				   int sig_offset)
+{
+	static const char default_list[] = FIT_KERNEL_PROP "\0"
+			FIT_FDT_PROP;
+	int ret, len;
+	const char *prop, *iname, *end;
+	int image_noffset;
+
+	/* If there is "sign-images" property, use that */
+	prop = fdt_getprop(fit, sig_offset, "sign-images", &len);
+	if (!prop) {
+		prop = default_list;
+		len = sizeof(default_list);
+	}
+
+	/* Locate the images */
+	end = prop + len;
+	for (iname = prop; iname < end; iname += strlen(iname) + 1) {
+		image_noffset = fit_conf_get_prop_node(fit, conf_noffset,
+						       iname);
+		if (image_noffset < 0)
+			return -ENOENT;
+
+		ret = fit_image_verify_arbvn(fit, image_noffset);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+#endif
+
 static int fit_config_verify_sig(const void *fit, int conf_noffset,
 				 const void *sig_blob, int sig_offset)
 {
@@ -401,6 +472,14 @@ static int fit_config_verify_sig(const void *fit, int conf_noffset,
 		goto error;
 	}
 
+#if !defined(USE_HOSTCC)
+	if (IMAGE_ENABLE_ARBP && verified) {
+		ret = fit_config_verify_arbvn(fit, conf_noffset, noffset);
+		if (ret)
+			verified = 0;
+	}
+#endif
+
 	if (verified)
 		return 0;
 
diff --git a/common/image-fit.c b/common/image-fit.c
index d54eff9033..97029853b9 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -1025,6 +1025,30 @@ int fit_image_get_data_and_size(const void *fit, int noffset,
 	return ret;
 }
 
+/**
+ * Get 'arbvn' property from a given image node.
+ *
+ * @fit: pointer to the FIT image header
+ * @noffset: component image node offset
+ * @arbvn: holds the arbvn property value
+ *
+ * returns:
+ *     0, on success
+ *     -ENOENT if the property could not be found
+ */
+int fit_image_get_arbvn(const void *fit, int noffset, uint32_t *arbvn)
+{
+	const fdt32_t *val;
+
+	val = fdt_getprop(fit, noffset, FIT_ARBVN_PROP, NULL);
+	if (!val)
+		return -ENOENT;
+
+	*arbvn = fdt32_to_cpu(*val);
+
+	return 0;
+}
+
 /**
  * fit_image_hash_get_algo - get hash algorithm name
  * @fit: pointer to the FIT format image header
diff --git a/include/image.h b/include/image.h
index 9a5a87dbf8..72a963cf27 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1005,6 +1005,7 @@ int booti_setup(ulong image, ulong *relocated_addr, ulong *size,
 #define FIT_COMP_PROP		"compression"
 #define FIT_ENTRY_PROP		"entry"
 #define FIT_LOAD_PROP		"load"
+#define FIT_ARBVN_PROP		"arbvn"
 
 /* configuration node */
 #define FIT_KERNEL_PROP		"kernel"
@@ -1085,6 +1086,7 @@ int fit_image_get_data_size_unciphered(const void *fit, int noffset,
 				       size_t *data_size);
 int fit_image_get_data_and_size(const void *fit, int noffset,
 				const void **data, size_t *size);
+int fit_image_get_arbvn(const void *fit, int noffset, uint32_t *arbvn);
 
 int fit_image_hash_get_algo(const void *fit, int noffset, char **algo);
 int fit_image_hash_get_value(const void *fit, int noffset, uint8_t **value,
@@ -1186,6 +1188,7 @@ int calculate_hash(const void *data, int data_len, const char *algo,
  * device
  */
 #if defined(USE_HOSTCC)
+# define IMAGE_ENABLE_ARBP	0
 # if defined(CONFIG_FIT_SIGNATURE)
 #  define IMAGE_ENABLE_SIGN	1
 #  define IMAGE_ENABLE_VERIFY	1
@@ -1200,6 +1203,7 @@ int calculate_hash(const void *data, int data_len, const char *algo,
 # define IMAGE_ENABLE_SIGN	0
 # define IMAGE_ENABLE_VERIFY		CONFIG_IS_ENABLED(RSA_VERIFY)
 # define FIT_IMAGE_ENABLE_VERIFY	CONFIG_IS_ENABLED(FIT_SIGNATURE)
+# define IMAGE_ENABLE_ARBP		CONFIG_IS_ENABLED(FIT_ARBP)
 #endif
 
 #if IMAGE_ENABLE_FIT
@@ -1544,6 +1548,25 @@ int board_fit_config_name_match(const char *name);
 void board_fit_image_post_process(void **p_image, size_t *p_size);
 #endif /* CONFIG_SPL_FIT_IMAGE_POST_PROCESS */
 
+#if defined(CONFIG_FIT_ARBP)
+/**
+ * board_get_arbvn() - get the arbvn stored in the platform secure storage.
+ *
+ * @ih_type: image type
+ * @arbvn: pointer to the arbvn
+ * @return 0 if upon successful retrieval, non-zero upon failure.
+ */
+int board_get_arbvn(uint8_t ih_type, uint32_t *arbvn);
+/**
+ * board_set_arbvn() - set the arbvn stored in the platform secure storage.
+ *
+ * @ih_type: image type
+ * @arbvn: new arbvn value to store in the platform secure storage.
+ * @return 0 if stored successfully, non-zero upon failure.
+ */
+int board_set_arbvn(uint8_t ih_type, uint32_t arbvn);
+#endif /* CONFIG_FIT_ARBP */
+
 #define FDT_ERROR	((ulong)(-1))
 
 ulong fdt_getprop_u32(const void *fdt, int node, const char *prop);
-- 
2.25.2

[RFC PATCH 0/1] Anti rollback protection for FIT Images

[Rasmus Villemoes]

On 01/09/2020 22.48, Thirupathaiah Annapureddy wrote:
> Anti rollback protection is required when there is a need to retire
> previous versions of FIT images due to security flaws in them.
> Currently U-Boot Verified boot does not have rollback protection to
> protect against known security flaws.

This is definitely something we've had on our todo-list/wishlist. But we
haven't had the time to sit down and work out the semantics and
implementation, so thanks for doing this.

I think most of this cover letter should be in the commit log instead.

> This RFC introduces a proposal to add anti-rollback protection for
> FIT images. This protection feature prevents U-Boot from accepting
> an image if it has ever successfully loaded an image with a larger
> anti-rollback version number.
> 
> Each sub-image node inside /images node has an
> anti-rollback version number(arbvn) similar to rollback_index in
> Android Verified Boot. This version number is part of signed data and
> it is incremented as security flaws are discovered and fixed.
> U-Boot stores the last seen arbvn for a given image type in platform
> specific tamper-evident storage.

Hmm. What is the purpose of per-image-type version numbers? That seems
to be wrong. For example, we use the the "loadables" property in the
U-Boot FIT to get the SPL to load some firmware blob to a known memory
address where U-Boot proper then knows it to be. If we happened to have
two such blobs for different pieces of hardware, they'd have the same
"type" in the FIT image, but that doesn't mean they should follow the
same versioning.

The way I imagined rollback protection to work (but I really haven't
given this too much thought) was to have an extra property in the
configuration:

	configurations {
		default = "conf-1";
		conf-1 {
			description = "...";
			kernel = "kernel-1";
			fdt = "fdt-1";
			ramdisk = "ramdisk-1";
			arvbn = "arvbn"; /* <-- */

			hash-1 {
				algo = "sha1";
			};
			signature-foo {
				algo = "sha1,rsa2048";
				sign-images = "kernel", "fdt", "ramdisk", "arvbn";
				key-name-hint = "foo";
			};
		};
	};

with arvbn simply being a small extra "image" node

		arvbn {
			description = "BSP version";
			data = <0x02 0x03 0x01>;
		}

(which should of course not be loaded to memory). This requires use of
signed configurations rather than individually signed images, but that's
anyway required for proper security.

The board callbacks would simply be given a pointer to the data part of
that node; that would make the versioning scheme rather flexible instead
of being limited to a single monotonically increasing u32 (hence also
the comparison logic should be in the board callbacks, instead of a
"get/set" interface). For example, one could have a version composed as
$major.$minor;$security, the board logic could prevent both downgrading
to another major version and another security version. I.e., suppose
we've had several BSP releases (in chronological order):

1.0;0
1.1;0
2.0;0
1.2;0
2.1;0
<major bug found and fixed>
1.3;1
2.2;1
2.3;1
1.4;1
<another one...>
2.4;2
1.5;2

With this, a user on 1.3 cannot update to 2.0; he must go at least to
2.2, or he would expose himself to a flaw that had already been fixed.
On the other hand, there should be nothing wrong with downgrading from
1.2 to 1.0. A user on 2.0 must not downgrade to 1.x because the old
major version cannot read the data written by the 2.0 application.

This sort of thing is hard to implement with just a single rollback
number: At the time 2.0 is released, what rollback number should it get?
If we give it 10, we can only produce fixes for nine security bugs in
the 1.x series before that series ends up with a rollback version larger
than the 2.0 one, after which a user on 2.0 would wrongly be able to
downgrade to 1.12.


tl;dr: I'd like this to not only be about rollback protection for
security bugs, but also be able to set other policies for rollback. And
therefore I'd also like it simply to be known as "version" instead of
the somewhat odd arvbn acronym. Nothing prevents the board developer
from just putting a single u32 in the version field and use that as a
single monotonically increasing version.


> As part of signature verification, U-Boot enfroces arvbn based
> protection if enabled. arvbn stored in secure storage is validated with
> arbvn in the sub-image node. If the counter in the FIT image is lower than
> the counter in platform secure storage, image validation has failed
> i.e. verified boot failed. If both counters match or the image counter is
> higher than that in the platform secure storage, the image validation is
> successful. In the higher case, U-Boot stores the new counter in platform
> secure storage.



> Pseudo code is as follows:
> 
> ret = board_get_arbvn(type, &plat_arbvn);
> ...
> if (image_arbvn < plat_arbvn) {
> 	return -EPERM;
> } else if (image_arbvn > plat_arbvn) {
> 	ret = board_set_arbvn(type, image_arbvn);
> 	return ret;
> } else {
> 	return 0;
> }

Hm, I think you should post-pone writing back the new version number
until all other checks have passed, i.e. until just before we pass
control to the new image. That's also a bit simpler if there's only one
version for the whole FIT image since you then wouldn't need to revisit
each image node.


> The following board specific hooks are required to get/set arbvn
> from platform specific tamper-evident storage.
> int board_get_arbvn(uint8_t ih_type, uint32_t *arbvn);
> int board_set_arbvn(uint8_t ih_type, uint32_t arbvn);
> 
> As an example, consider this FIT:
> / {
> 	images {
> 		kernel-1 {
> 			data = <data for kernel1>
> 			arbvn = <1>;
> 			hash-1 {
> 				algo = "sha1";
> 				value = <...kernel hash 1...>
> 			};
> 		};
> 		fdt-1 {
> 			data = <data for fdt1>;
> 			hash-1 {
> 				algo = "sha1";
> 				value = <...fdt hash 1...>
> 			};
> 		};
> 	};
> 	configurations {
> 		default = "conf-1";
> 		conf-1 {
> 			kernel = "kernel-1";
> 			fdt = "fdt-1";
> 			signature-1 {
> 				algo = "sha1,rsa2048";
> 				sign-images = "fdt", "kernel";
> 				value = <...conf 1 signature...>;
> 			};
> 		};
> 	};
> };
> 
> In the above example, kernel-1 image has an arbvn of 1.
> if plat_arbvn is 1, the system will boot with this FIT image.
> if plat_arbvn is 2 or more, U-Boot will prevent the system from booting
> with this FIT image.

Yeah, this seems a bit arbitrary. Why does the kernel have an arvbn, but
not the device tree or (perhaps more importantly) an initramfs? Which
images must have such a number? As I said above, I think the version
number should really be a property of the whole configuration. And of
course, as you also emphasize, the version must be part of what gets
signed (which is the only reason for the indirection to a pseudo-image
node, otherwise it could just be a version=<> property in the
configuration node).

Rasmus

[RFC PATCH 1/1] image: add anti rollback protection for FIT Images

[Simon Glass]

Hi Thirupathaiah,

On Tue, 1 Sep 2020 at 14:48, Thirupathaiah Annapureddy
<thiruan@linux.microsoft.com> wrote:
>
> Anti rollback protection is required when there is a need to retire
> previous versions of FIT images due to security flaws in them.
> Currently U-Boot Verified boot does not have rollback protection to
> protect against known security flaws.
>
> This change adds anti-rollback protection for FIT images.
>
> Signed-off-by: Thirupathaiah Annapureddy <thiruan@linux.microsoft.com>
> ---
>  Kconfig                |  9 +++++
>  common/image-fit-sig.c | 79 ++++++++++++++++++++++++++++++++++++++++++
>  common/image-fit.c     | 24 +++++++++++++
>  include/image.h        | 23 ++++++++++++
>  4 files changed, 135 insertions(+)

Good to see this. I have a few comments though. This needs to be done
very carefully as it affects security.

>
> diff --git a/Kconfig b/Kconfig
> index 883e3f71d0..3959a6592c 100644
> --- a/Kconfig
> +++ b/Kconfig
> @@ -533,6 +533,15 @@ config FIT_CIPHER
>           Enable the feature of data ciphering/unciphering in the tool mkimage
>           and in the u-boot support of the FIT image.
>
> +config FIT_ARBP

How about using ROLLBACK instead of ARBP. It is easier to understand.

> +       bool "Enable Anti rollback version check for FIT images"

anti-rollback (add hyphen)

> +       depends on FIT_SIGNATURE
> +       default n
> +       help
> +         Enable this to activate anti-rollback protection. This is required
> +         when a platform needs to retire previous versions of FIT images due to
> +         security flaws in in them.

in

> +
>  config FIT_VERBOSE
>         bool "Show verbose messages when FIT images fail"
>         help
> diff --git a/common/image-fit-sig.c b/common/image-fit-sig.c
> index cc1967109e..5103508894 100644
> --- a/common/image-fit-sig.c
> +++ b/common/image-fit-sig.c
> @@ -78,6 +78,35 @@ struct image_region *fit_region_make_list(const void *fit,
>         return region;
>  }
>
> +#if !defined(USE_HOSTCC)
> +static int fit_image_verify_arbvn(const void *fit, int image_noffset)

Please add a comment as to what this does and what it checks

> +{
> +       uint8_t type;
> +       uint32_t image_arbvn;
> +       uint32_t plat_arbvn = 0;

Those three can be uint.

> +       int ret;
> +
> +       ret = fit_image_get_arbvn(fit, image_noffset, &image_arbvn);
> +       if (ret)
> +               return 0;
> +
> +       ret = fit_image_get_type(fit, image_noffset, &type);
> +       if (ret)
> +               return 0;
> +
> +       ret = board_get_arbvn(type, &plat_arbvn);
> +
> +       if (image_arbvn < plat_arbvn) {
> +               return -EPERM;
> +       } else if (image_arbvn > plat_arbvn) {
> +               ret = board_set_arbvn(type, image_arbvn);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +#endif
> +
>  static int fit_image_setup_verify(struct image_sign_info *info,
>                                   const void *fit, int noffset,
>                                   int required_keynode, char **err_msgp)
> @@ -181,6 +210,14 @@ static int fit_image_verify_sig(const void *fit, int image_noffset,
>                 goto error;
>         }
>
> +#if !defined(USE_HOSTCC)
> +       if (IMAGE_ENABLE_ARBP && verified) {
> +               ret = fit_image_verify_arbvn(fit, image_noffset);
> +               if (ret)
> +                       verified = 0;
> +       }
> +#endif
> +
>         return verified ? 0 : -EPERM;
>
>  error:
> @@ -370,6 +407,40 @@ static int fit_config_check_sig(const void *fit, int noffset,
>         return 0;
>  }
>
> +#if !defined(USE_HOSTCC)
> +static int fit_config_verify_arbvn(const void *fit, int conf_noffset,
> +                                  int sig_offset)
> +{
> +       static const char default_list[] = FIT_KERNEL_PROP "\0"
> +                       FIT_FDT_PROP;
> +       int ret, len;
> +       const char *prop, *iname, *end;
> +       int image_noffset;
> +
> +       /* If there is "sign-images" property, use that */
> +       prop = fdt_getprop(fit, sig_offset, "sign-images", &len);
> +       if (!prop) {
> +               prop = default_list;
> +               len = sizeof(default_list);
> +       }
> +
> +       /* Locate the images */
> +       end = prop + len;
> +       for (iname = prop; iname < end; iname += strlen(iname) + 1) {
> +               image_noffset = fit_conf_get_prop_node(fit, conf_noffset,
> +                                                      iname);
> +               if (image_noffset < 0)
> +                       return -ENOENT;
> +
> +               ret = fit_image_verify_arbvn(fit, image_noffset);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       return 0;
> +}
> +#endif
> +
>  static int fit_config_verify_sig(const void *fit, int conf_noffset,
>                                  const void *sig_blob, int sig_offset)
>  {
> @@ -401,6 +472,14 @@ static int fit_config_verify_sig(const void *fit, int conf_noffset,
>                 goto error;
>         }
>
> +#if !defined(USE_HOSTCC)

Do we need this ?ifdef, or can we rely on IMAGE_ENABLE_ARBP?

> +       if (IMAGE_ENABLE_ARBP && verified) {
> +               ret = fit_config_verify_arbvn(fit, conf_noffset, noffset);
> +               if (ret)
> +                       verified = 0;
> +       }
> +#endif
> +
>         if (verified)
>                 return 0;
>
> diff --git a/common/image-fit.c b/common/image-fit.c
> index d54eff9033..97029853b9 100644
> --- a/common/image-fit.c
> +++ b/common/image-fit.c
> @@ -1025,6 +1025,30 @@ int fit_image_get_data_and_size(const void *fit, int noffset,
>         return ret;
>  }
>
> +/**
> + * Get 'arbvn' property from a given image node.
> + *
> + * @fit: pointer to the FIT image header
> + * @noffset: component image node offset
> + * @arbvn: holds the arbvn property value
> + *
> + * returns:
> + *     0, on success
> + *     -ENOENT if the property could not be found
> + */
> +int fit_image_get_arbvn(const void *fit, int noffset, uint32_t *arbvn)

fit_image_get_rollback()

s/arbvn/versionp/

> +{
> +       const fdt32_t *val;
> +
> +       val = fdt_getprop(fit, noffset, FIT_ARBVN_PROP, NULL);
> +       if (!val)
> +               return -ENOENT;
> +
> +       *arbvn = fdt32_to_cpu(*val);
> +
> +       return 0;
> +}
> +
>  /**
>   * fit_image_hash_get_algo - get hash algorithm name
>   * @fit: pointer to the FIT format image header
> diff --git a/include/image.h b/include/image.h
> index 9a5a87dbf8..72a963cf27 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -1005,6 +1005,7 @@ int booti_setup(ulong image, ulong *relocated_addr, ulong *size,
>  #define FIT_COMP_PROP          "compression"
>  #define FIT_ENTRY_PROP         "entry"
>  #define FIT_LOAD_PROP          "load"
> +#define FIT_ARBVN_PROP         "arbvn"

ROLLBACK / "rollback"

>
>  /* configuration node */
>  #define FIT_KERNEL_PROP                "kernel"
> @@ -1085,6 +1086,7 @@ int fit_image_get_data_size_unciphered(const void *fit, int noffset,
>                                        size_t *data_size);
>  int fit_image_get_data_and_size(const void *fit, int noffset,
>                                 const void **data, size_t *size);
> +int fit_image_get_arbvn(const void *fit, int noffset, uint32_t *arbvn);

Please add a full function comment

>
>  int fit_image_hash_get_algo(const void *fit, int noffset, char **algo);
>  int fit_image_hash_get_value(const void *fit, int noffset, uint8_t **value,
> @@ -1186,6 +1188,7 @@ int calculate_hash(const void *data, int data_len, const char *algo,
>   * device
>   */
>  #if defined(USE_HOSTCC)
> +# define IMAGE_ENABLE_ARBP     0
>  # if defined(CONFIG_FIT_SIGNATURE)
>  #  define IMAGE_ENABLE_SIGN    1
>  #  define IMAGE_ENABLE_VERIFY  1
> @@ -1200,6 +1203,7 @@ int calculate_hash(const void *data, int data_len, const char *algo,
>  # define IMAGE_ENABLE_SIGN     0
>  # define IMAGE_ENABLE_VERIFY           CONFIG_IS_ENABLED(RSA_VERIFY)
>  # define FIT_IMAGE_ENABLE_VERIFY       CONFIG_IS_ENABLED(FIT_SIGNATURE)
> +# define IMAGE_ENABLE_ARBP             CONFIG_IS_ENABLED(FIT_ARBP)
>  #endif
>
>  #if IMAGE_ENABLE_FIT
> @@ -1544,6 +1548,25 @@ int board_fit_config_name_match(const char *name);
>  void board_fit_image_post_process(void **p_image, size_t *p_size);
>  #endif /* CONFIG_SPL_FIT_IMAGE_POST_PROCESS */
>
> +#if defined(CONFIG_FIT_ARBP)
> +/**
> + * board_get_arbvn() - get the arbvn stored in the platform secure storage.
> + *
> + * @ih_type: image type
> + * @arbvn: pointer to the arbvn
> + * @return 0 if upon successful retrieval, non-zero upon failure.
> + */
> +int board_get_arbvn(uint8_t ih_type, uint32_t *arbvn);

This needs a driver since the rollback counter may be implemented by a
TPM or anything. If you want to use the board, add a new
get_rollback() to UCLASS_BOARD (board.h). Or you could create a new
UCLASS_SECURITY which includes these two API calls.

> +/**
> + * board_set_arbvn() - set the arbvn stored in the platform secure storage.
> + *
> + * @ih_type: image type
> + * @arbvn: new arbvn value to store in the platform secure storage.
> + * @return 0 if stored successfully, non-zero upon failure.
> + */
> +int board_set_arbvn(uint8_t ih_type, uint32_t arbvn);
> +#endif /* CONFIG_FIT_ARBP */
> +
>  #define FDT_ERROR      ((ulong)(-1))
>
>  ulong fdt_getprop_u32(const void *fdt, int node, const char *prop);
> --
> 2.25.2
>

Also please update the vboot test to add a check for rollback.

Regards,
Simon

[RFC PATCH 0/1] Anti rollback protection for FIT Images

[Rasmus Villemoes]

On 02/09/2020 09.58, Rasmus Villemoes wrote:
> On 01/09/2020 22.48, Thirupathaiah Annapureddy wrote:
>> Anti rollback protection is required when there is a need to retire
>> previous versions of FIT images due to security flaws in them.
>> Currently U-Boot Verified boot does not have rollback protection to
>> protect against known security flaws.
> 
> This is definitely something we've had on our todo-list/wishlist. But we
> haven't had the time to sit down and work out the semantics and
> implementation, so thanks for doing this.

...

> The board callbacks would simply be given a pointer to the data part of
> that node; that would make the versioning scheme rather flexible instead
> of being limited to a single monotonically increasing u32 (hence also
> the comparison logic should be in the board callbacks, instead of a
> "get/set" interface).

Oh, and another reason for having the board callbacks being responsible
for the Yay/Nay verdict is that that makes it possible to hook up a gpio
that can be used to say "ignore rollback version check" - immensely
useful during development, and might also come in handy for the end
products.

Rasmus

[RFC PATCH 0/1] Anti rollback protection for FIT Images

[Thirupathaiah Annapureddy]

Hi Rasmus,

Thanks for the review. Please see in-line:

On 9/2/2020 12:58 AM, Rasmus Villemoes wrote:
> On 01/09/2020 22.48, Thirupathaiah Annapureddy wrote:
>> Anti rollback protection is required when there is a need to retire
>> previous versions of FIT images due to security flaws in them.
>> Currently U-Boot Verified boot does not have rollback protection to
>> protect against known security flaws.
> 
> This is definitely something we've had on our todo-list/wishlist. But we
> haven't had the time to sit down and work out the semantics and
> implementation, so thanks for doing this.
> 
> I think most of this cover letter should be in the commit log instead.
Yes, will do in the formal patch proposal.

> 
>> This RFC introduces a proposal to add anti-rollback protection for
>> FIT images. This protection feature prevents U-Boot from accepting
>> an image if it has ever successfully loaded an image with a larger
>> anti-rollback version number.
>>
>> Each sub-image node inside /images node has an
>> anti-rollback version number(arbvn) similar to rollback_index in
>> Android Verified Boot. This version number is part of signed data and
>> it is incremented as security flaws are discovered and fixed.
>> U-Boot stores the last seen arbvn for a given image type in platform
>> specific tamper-evident storage.
> 
> Hmm. What is the purpose of per-image-type version numbers? That seems
> to be wrong. For example, we use the the "loadables" property in the
> U-Boot FIT to get the SPL to load some firmware blob to a known memory
> address where U-Boot proper then knows it to be. If we happened to have
> two such blobs for different pieces of hardware, they'd have the same
> "type" in the FIT image, but that doesn't mean they should follow the
> same versioning.
I received similar feedback internally to enhance the board specific hooks
to take sub-image node name as another input argument. Board specific code
can use this additional input to distinguish different images of the same
type.

> 
> The way I imagined rollback protection to work (but I really haven't
> given this too much thought) was to have an extra property in the
> configuration:
> 
> 	configurations {
> 		default = "conf-1";
> 		conf-1 {
> 			description = "...";
> 			kernel = "kernel-1";
> 			fdt = "fdt-1";
> 			ramdisk = "ramdisk-1";
> 			arvbn = "arvbn"; /* <-- */
> 
> 			hash-1 {
> 				algo = "sha1";
> 			};
> 			signature-foo {
> 				algo = "sha1,rsa2048";
> 				sign-images = "kernel", "fdt", "ramdisk", "arvbn";
> 				key-name-hint = "foo";
> 			};
> 		};
> 	};
> 
> with arvbn simply being a small extra "image" node
> 
> 		arvbn {
> 			description = "BSP version";
> 			data = <0x02 0x03 0x01>;
> 		}
> 
> (which should of course not be loaded to memory). This requires use of
> signed configurations rather than individually signed images, but that's
> anyway required for proper security.
The current proposal applies to both signed images and signed configurations.

> 
> The board callbacks would simply be given a pointer to the data part of
> that node; that would make the versioning scheme rather flexible instead
> of being limited to a single monotonically increasing u32 (hence also
> the comparison logic should be in the board callbacks, instead of a
The comparison logic should be in the common code to enforce the checks in
a consistent way. 

> "get/set" interface). For example, one could have a version composed as
> $major.$minor;$security, the board logic could prevent both downgrading
> to another major version and another security version. I.e., suppose
> we've had several BSP releases (in chronological order):
A BSP could be composed of multiple images and each of them will have their
own normal versioning scheme. Anti-rollback version number(arbvn) is an
additional complimentary version that can be incremented as 
security flaws are discovered and fixed in a given image. This does not replace
existing versioning scheme for a given image. 

> 
> 1.0;0
> 1.1;0
> 2.0;0
> 1.2;0
> 2.1;0
> <major bug found and fixed>
> 1.3;1
> 2.2;1
> 2.3;1
> 1.4;1
> <another one...>
> 2.4;2
> 1.5;2
> 
> With this, a user on 1.3 cannot update to 2.0; he must go at least to
> 2.2, or he would expose himself to a flaw that had already been fixed.
> On the other hand, there should be nothing wrong with downgrading from
> 1.2 to 1.0. A user on 2.0 must not downgrade to 1.x because the old
> major version cannot read the data written by the 2.0 application.
Please revisit your normal versioning scheme. If possible try to follow
https://semver.org/. As I mentioned earlier, 
Anti-rollback version number(arbvn) is an additional version that can be 
incremented as security flaws are discovered and fixed in a given image.
In other words, it does not replace your existing versioning scheme.

> 
> This sort of thing is hard to implement with just a single rollback
> number: At the time 2.0 is released, what rollback number should it get?
> If we give it 10, we can only produce fixes for nine security bugs in
> the 1.x series before that series ends up with a rollback version larger
> than the 2.0 one, after which a user on 2.0 would wrongly be able to
> downgrade to 1.12.
> 
> 
> tl;dr: I'd like this to not only be about rollback protection for
> security bugs, 
But this RFC is for rollback protection from security point of view.

> but also be able to set other policies for rollback. And
> therefore I'd also like it simply to be known as "version" instead of
> the somewhat odd arvbn acronym. 
This is clarified in the previous comments. 
Images will continue to maintain their existing normal versioning scheme
and you can continue to apply existing policies around that.

>> Pseudo code is as follows:
>>
>> ret = board_get_arbvn(type, &plat_arbvn);
>> ...
>> if (image_arbvn < plat_arbvn) {
>> 	return -EPERM;
>> } else if (image_arbvn > plat_arbvn) {
>> 	ret = board_set_arbvn(type, image_arbvn);
>> 	return ret;
>> } else {
>> 	return 0;
>> }
> 
> Hm, I think you should post-pone writing back the new version number
> until all other checks have passed, i.e. until just before we pass
> control to the new image. 
I do not think U-Boot will pass control to each and every image in the 
FIT container. ex: ramdisk.
Board specific code can still have the flexibility to defer and write new
version number at a later time ex: board_preboot_os(). 

>> The following board specific hooks are required to get/set arbvn
>> from platform specific tamper-evident storage.
>> int board_get_arbvn(uint8_t ih_type, uint32_t *arbvn);
>> int board_set_arbvn(uint8_t ih_type, uint32_t arbvn);
>>
>> As an example, consider this FIT:
>> / {
>> 	images {
>> 		kernel-1 {
>> 			data = <data for kernel1>
>> 			arbvn = <1>;
>> 			hash-1 {
>> 				algo = "sha1";
>> 				value = <...kernel hash 1...>
>> 			};
>> 		};
>> 		fdt-1 {
>> 			data = <data for fdt1>;
>> 			hash-1 {
>> 				algo = "sha1";
>> 				value = <...fdt hash 1...>
>> 			};
>> 		};
>> 	};
>> 	configurations {
>> 		default = "conf-1";
>> 		conf-1 {
>> 			kernel = "kernel-1";
>> 			fdt = "fdt-1";
>> 			signature-1 {
>> 				algo = "sha1,rsa2048";
>> 				sign-images = "fdt", "kernel";
>> 				value = <...conf 1 signature...>;
>> 			};
>> 		};
>> 	};
>> };
>>
>> In the above example, kernel-1 image has an arbvn of 1.
>> if plat_arbvn is 1, the system will boot with this FIT image.
>> if plat_arbvn is 2 or more, U-Boot will prevent the system from booting
>> with this FIT image.
> 
> Yeah, this seems a bit arbitrary. 
This is just an example. 

> Why does the kernel have an arvbn, but
> not the device tree or (perhaps more importantly) an initramfs? 
device tree and initramfs can also have their own arbvn. Have you checked
the code patch that came with this RFC? There is nothing in the code that 
prevents these images from having their own arbvn.

> Which images must have such a number?
Any image can have their own arbvn.

> As I said above, I think the version
> number should really be a property of the whole configuration. 
I have considered the above simpler option. But I believe image specific arbvn
is more appropriate as the security features and security bug fixes are implemented
in images than in config sub-nodes.

Best Regards,
Thiru

[RFC PATCH 1/1] image: add anti rollback protection for FIT Images

[Thirupathaiah Annapureddy]

Hi Simon,

Thanks for the review.

On 9/6/2020 6:43 PM, Simon Glass wrote:
>>
>> diff --git a/Kconfig b/Kconfig
>> index 883e3f71d0..3959a6592c 100644
>> --- a/Kconfig
>> +++ b/Kconfig
>> @@ -533,6 +533,15 @@ config FIT_CIPHER
>>           Enable the feature of data ciphering/unciphering in the tool mkimage
>>           and in the u-boot support of the FIT image.
>>
>> +config FIT_ARBP
> 
> How about using ROLLBACK instead of ARBP. It is easier to understand.Looks good to me. I will change it in the next version of the patch.

>> +{
>> +       uint8_t type;
>> +       uint32_t image_arbvn;
>> +       uint32_t plat_arbvn = 0;
> 
> Those three can be uint.
fit_image_get_type() returns type as uint8_t. 
I can change it for the other two variables. 

>>  static int fit_config_verify_sig(const void *fit, int conf_noffset,
>>                                  const void *sig_blob, int sig_offset)
>>  {
>> @@ -401,6 +472,14 @@ static int fit_config_verify_sig(const void *fit, int conf_noffset,
>>                 goto error;
>>         }
>>
>> +#if !defined(USE_HOSTCC)
> 
> Do we need this ?ifdef, or can we rely on IMAGE_ENABLE_ARBP?
I believe we can rely on just IMAGE_ENABLE_ARBP.

>>  #define FIT_LOAD_PROP          "load"
>> +#define FIT_ARBVN_PROP         "arbvn"
> 
> ROLLBACK / "rollback"
I will fix it in the next version.

> 
>>
>>  /* configuration node */
>>  #define FIT_KERNEL_PROP                "kernel"
>> @@ -1085,6 +1086,7 @@ int fit_image_get_data_size_unciphered(const void *fit, int noffset,
>>                                        size_t *data_size);
>>  int fit_image_get_data_and_size(const void *fit, int noffset,
>>                                 const void **data, size_t *size);
>> +int fit_image_get_arbvn(const void *fit, int noffset, uint32_t *arbvn);
> 
> Please add a full function comment
comment was added before the function definition to be consistent
with other functions.

>> +int board_get_arbvn(uint8_t ih_type, uint32_t *arbvn);
> 
> This needs a driver since the rollback counter may be implemented by a
> TPM or anything. 
Board specific hooks can leverage TPM library functions in that case.
May I know why a driver is needed?

> If you want to use the board, add a new
> get_rollback() to UCLASS_BOARD (board.h). Or you could create a new
> UCLASS_SECURITY which includes these two API calls.
I explored the option of using UCLASS_BOARD. But it does not have "set"
interfaces and the "id" parameter used in "get" functions seem to be
board specific. We can look into the option of UCLASS_SECURITY for these
two API calls.

> 
> Also please update the vboot test to add a check for rollback.

Yes, will do in the next version of the patch series.

Best Regards,
Thiru

[RFC PATCH 0/1] Anti rollback protection for FIT Images

[Thirupathaiah Annapureddy]

On 9/7/2020 11:15 PM, Rasmus Villemoes wrote:
> On 02/09/2020 09.58, Rasmus Villemoes wrote:
>> On 01/09/2020 22.48, Thirupathaiah Annapureddy wrote:
>>> Anti rollback protection is required when there is a need to retire
>>> previous versions of FIT images due to security flaws in them.
>>> Currently U-Boot Verified boot does not have rollback protection to
>>> protect against known security flaws.
>>
>> This is definitely something we've had on our todo-list/wishlist. But we
>> haven't had the time to sit down and work out the semantics and
>> implementation, so thanks for doing this.
> 
> ...
> 
>> The board callbacks would simply be given a pointer to the data part of
>> that node; that would make the versioning scheme rather flexible instead
>> of being limited to a single monotonically increasing u32 (hence also
>> the comparison logic should be in the board callbacks, instead of a
>> "get/set" interface).
> 
> Oh, and another reason for having the board callbacks being responsible
> for the Yay/Nay verdict is that that makes it possible to hook up a gpio
> that can be used to say "ignore rollback version check" - immensely
> useful during development, and might also come in handy for the end
> products.
This is not a good idea from security point of view as that would lead
to physical present attacks.

> 
> Rasmus
> 

[RFC PATCH 0/1] Anti rollback protection for FIT Images

[Rasmus Villemoes]

On 15/09/2020 08.20, Thirupathaiah Annapureddy wrote:
> 
> On 9/7/2020 11:15 PM, Rasmus Villemoes wrote:
>> On 02/09/2020 09.58, Rasmus Villemoes wrote:
>>> On 01/09/2020 22.48, Thirupathaiah Annapureddy wrote:
>>>> Anti rollback protection is required when there is a need to retire
>>>> previous versions of FIT images due to security flaws in them.
>>>> Currently U-Boot Verified boot does not have rollback protection to
>>>> protect against known security flaws.
>>>
>>> This is definitely something we've had on our todo-list/wishlist. But we
>>> haven't had the time to sit down and work out the semantics and
>>> implementation, so thanks for doing this.
>>
>> ...
>>
>>> The board callbacks would simply be given a pointer to the data part of
>>> that node; that would make the versioning scheme rather flexible instead
>>> of being limited to a single monotonically increasing u32 (hence also
>>> the comparison logic should be in the board callbacks, instead of a
>>> "get/set" interface).
>>
>> Oh, and another reason for having the board callbacks being responsible
>> for the Yay/Nay verdict is that that makes it possible to hook up a gpio
>> that can be used to say "ignore rollback version check" - immensely
>> useful during development, and might also come in handy for the end
>> products.
> This is not a good idea from security point of view as that would lead
> to physical present attacks.

That's why I said "might". For some products physical presence is not
really a relevant attack scenario (e.g. stuff that sits in an offshore
windmill), but giving a service technician the possibility of overriding
a rollback check can be quite valuable. But yes, in the majority of
cases I'd expect such a feature to be disabled on the final product
(say, by burning some e-fuse, hardwiring the gpio or whatnot). That
doesn't diminish the usefulness of having it during development.

Rasmus

[RFC PATCH 0/1] Anti rollback protection for FIT Images

[Rasmus Villemoes]

On 15/09/2020 07.22, Thirupathaiah Annapureddy wrote:
> Hi Rasmus,
> 
>>
>>> This RFC introduces a proposal to add anti-rollback protection for
>>> FIT images. This protection feature prevents U-Boot from accepting
>>> an image if it has ever successfully loaded an image with a larger
>>> anti-rollback version number.
>>>
>>> Each sub-image node inside /images node has an
>>> anti-rollback version number(arbvn) similar to rollback_index in
>>> Android Verified Boot. This version number is part of signed data and
>>> it is incremented as security flaws are discovered and fixed.
>>> U-Boot stores the last seen arbvn for a given image type in platform
>>> specific tamper-evident storage.
>>
>> Hmm. What is the purpose of per-image-type version numbers? That seems
>> to be wrong. For example, we use the the "loadables" property in the
>> U-Boot FIT to get the SPL to load some firmware blob to a known memory
>> address where U-Boot proper then knows it to be. If we happened to have
>> two such blobs for different pieces of hardware, they'd have the same
>> "type" in the FIT image, but that doesn't mean they should follow the
>> same versioning.
> I received similar feedback internally to enhance the board specific hooks
> to take sub-image node name as another input argument. Board specific code
> can use this additional input to distinguish different images of the same
> type.

OK. Are those node-names tamper-proof? I.e., can I take a valid signed
FIT, modify node names (and the references to those) and make one
LOADABLE image appear to be another LOADABLE from the POV of the board hook?

>>
>> The way I imagined rollback protection to work (but I really haven't
>> given this too much thought) was to have an extra property in the
>> configuration:
>>
>> 	configurations {
>> 		default = "conf-1";
>> 		conf-1 {
>> 			description = "...";
>> 			kernel = "kernel-1";
>> 			fdt = "fdt-1";
>> 			ramdisk = "ramdisk-1";
>> 			arvbn = "arvbn"; /* <-- */
>>
>> 			hash-1 {
>> 				algo = "sha1";
>> 			};
>> 			signature-foo {
>> 				algo = "sha1,rsa2048";
>> 				sign-images = "kernel", "fdt", "ramdisk", "arvbn";
>> 				key-name-hint = "foo";
>> 			};
>> 		};
>> 	};
>>
>> with arvbn simply being a small extra "image" node
>>
>> 		arvbn {
>> 			description = "BSP version";
>> 			data = <0x02 0x03 0x01>;
>> 		}
>>
>> (which should of course not be loaded to memory). This requires use of
>> signed configurations rather than individually signed images, but that's
>> anyway required for proper security.
> The current proposal applies to both signed images and signed configurations.

Perhaps, but did you read the "required for proper security" part? Why
not mandate use of signed configurations and move towards getting rid of
possibility of mix-n-match attacks once and for all?

>>
>> The board callbacks would simply be given a pointer to the data part of
>> that node; that would make the versioning scheme rather flexible instead
>> of being limited to a single monotonically increasing u32 (hence also
>> the comparison logic should be in the board callbacks, instead of a
> The comparison logic should be in the common code to enforce the checks in
> a consistent way. 

Well, I disagree. Storage/retrieval of the rollback version in a secure
and robust way has to be done in a board-specific manner anyway, so I
don't think leaving the comparison logic to board code adds much risk of
getting the whole thing wrong.

>> "get/set" interface). For example, one could have a version composed as
>> $major.$minor;$security, the board logic could prevent both downgrading
>> to another major version and another security version. I.e., suppose
>> we've had several BSP releases (in chronological order):
> A BSP could be composed of multiple images and each of them will have their
> own normal versioning scheme. Anti-rollback version number(arbvn) is an
> additional complimentary version that can be incremented as 
> security flaws are discovered and fixed in a given image. This does not replace
> existing versioning scheme for a given image. 
> 
>>
>> 1.0;0
>> 1.1;0
>> 2.0;0
>> 1.2;0
>> 2.1;0
>> <major bug found and fixed>
>> 1.3;1
>> 2.2;1
>> 2.3;1
>> 1.4;1
>> <another one...>
>> 2.4;2
>> 1.5;2
>>
>> With this, a user on 1.3 cannot update to 2.0; he must go at least to
>> 2.2, or he would expose himself to a flaw that had already been fixed.
>> On the other hand, there should be nothing wrong with downgrading from
>> 1.2 to 1.0. A user on 2.0 must not downgrade to 1.x because the old
>> major version cannot read the data written by the 2.0 application.
> Please revisit your normal versioning scheme. If possible try to follow
> https://semver.org/.

That was just a simple example (which is actually somwhat semver-like, I
just dropped the .bugfix to keep it simple), and in any case I'm a
consultant, whatever versioning scheme any given customer uses is
completely out of my hands and is usually not (only) decided on
technical merits.

As I mentioned earlier,
> Anti-rollback version number(arbvn) is an additional version that can be 
> incremented as security flaws are discovered and fixed in a given image.
> In other words, it does not replace your existing versioning scheme.

No, and I definitely want it to work that way; that's why I separated
the "security version" from the normal one by a semi-colon above, to
stress its orthogonality. We're completely in agreement on that point.

My point is that there is currently no simple, or at least standard, way
to enforce the "you can't downgrade from one major version to a lower
one" - simply because there's no concept of versioning in current FIT
images. IMO, having semi-generic code doing the "can't downgrade due to
CVE2020-12345" and having some board_preboot_os do another version check
is silly - especially as wherever the latter version might be stored in
the FIT is almost certainly not going to be automatically signed,
opening up all kinds of questions of "can we actually trust that version
number".

So I'm asking you to consider hitting two flies with one swat, and make
this thing a generic "version", that can be used for whatever purpose
the current product might want to enforce regarding downgrades. Which,
as I said, in no way prevents your use case, the "version" would just be
your arbvn, and the comparison is just your comparison of two u32s.

>>
>> This sort of thing is hard to implement with just a single rollback
>> number: At the time 2.0 is released, what rollback number should it get?
>> If we give it 10, we can only produce fixes for nine security bugs in
>> the 1.x series before that series ends up with a rollback version larger
>> than the 2.0 one, after which a user on 2.0 would wrongly be able to
>> downgrade to 1.12.
>>
>>
>> tl;dr: I'd like this to not only be about rollback protection for
>> security bugs, 
> But this RFC is for rollback protection from security point of view.

See above, I don't think it's more complicated to allow use cases beyond
merely security weaknesses. But once you set "arbvn is one monotonically
increasing u32" in stone, getting those other use cases implemented and
upstreamable becomes much much harder.

>> but also be able to set other policies for rollback. And
>> therefore I'd also like it simply to be known as "version" instead of
>> the somewhat odd arvbn acronym. 
> This is clarified in the previous comments. 
> Images will continue to maintain their existing normal versioning scheme
> and you can continue to apply existing policies around that.

Except that such a policy cannot actually be implemented currently.

>>> Pseudo code is as follows:
>>>
>>> ret = board_get_arbvn(type, &plat_arbvn);
>>> ...
>>> if (image_arbvn < plat_arbvn) {
>>> 	return -EPERM;
>>> } else if (image_arbvn > plat_arbvn) {
>>> 	ret = board_set_arbvn(type, image_arbvn);
>>> 	return ret;
>>> } else {
>>> 	return 0;
>>> }
>>
>> Hm, I think you should post-pone writing back the new version number
>> until all other checks have passed, i.e. until just before we pass
>> control to the new image. 
> I do not think U-Boot will pass control to each and every image in the 
> FIT container. ex: ramdisk
No, of course not, what I meant was that before control is handed over
to linux (or whatever OS is loaded), there should be another loop over
the loaded images and have their arbvn written back.

But here we actually see another argument for the rollback version
applying to the FIT image (the chosen configuration) as a whole:
atomicity. If storing the new version number fails for one of the later
seen images, we've already updated the arbvn for some of the previous
images, which is very likely to brick the device.

> Board specific code can still have the flexibility to defer and write new
> version number at a later time ex: board_preboot_os(). 

So you would have board_set_arbvn() be a mostly empty hook that perhaps
just stashes the new version in some temporary location, for each
image/image type, and then have a "now really write out the new
versions" called from board_preboot_os()? That could work, and could
perhaps also partially mitigate the atomicity problem (by having the
"write things out" function also be able to attempt to revert
already-changed values) but I think that should be part of the
architecture of this rather than something random board developers might
remember to implement.

>>> The following board specific hooks are required to get/set arbvn
>>> from platform specific tamper-evident storage.
>>> int board_get_arbvn(uint8_t ih_type, uint32_t *arbvn);
>>> int board_set_arbvn(uint8_t ih_type, uint32_t arbvn);
>>>
>>> As an example, consider this FIT:
>>> / {
>>> 	images {
>>> 		kernel-1 {
>>> 			data = <data for kernel1>
>>> 			arbvn = <1>;
>>> 			hash-1 {
>>> 				algo = "sha1";
>>> 				value = <...kernel hash 1...>
>>> 			};
>>> 		};
>>> 		fdt-1 {
>>> 			data = <data for fdt1>;
>>> 			hash-1 {
>>> 				algo = "sha1";
>>> 				value = <...fdt hash 1...>
>>> 			};
>>> 		};
>>> 	};
>>> 	configurations {
>>> 		default = "conf-1";
>>> 		conf-1 {
>>> 			kernel = "kernel-1";
>>> 			fdt = "fdt-1";
>>> 			signature-1 {
>>> 				algo = "sha1,rsa2048";
>>> 				sign-images = "fdt", "kernel";
>>> 				value = <...conf 1 signature...>;
>>> 			};
>>> 		};
>>> 	};
>>> };
>>>
>>> In the above example, kernel-1 image has an arbvn of 1.
>>> if plat_arbvn is 1, the system will boot with this FIT image.
>>> if plat_arbvn is 2 or more, U-Boot will prevent the system from booting
>>> with this FIT image.
>>
>> Yeah, this seems a bit arbitrary. 
> This is just an example. 
> 
>> Why does the kernel have an arvbn, but
>> not the device tree or (perhaps more importantly) an initramfs? 
> device tree and initramfs can also have their own arbvn. Have you checked
> the code patch that came with this RFC? There is nothing in the code that 
> prevents these images from having their own arbvn.

OK, I was mostly confused by this being tied to the image type, which
seemed to ignore that a FIT/configuration can validly contain multiple
images with the same type.

>> Which images must have such a number?
> Any image can have their own arbvn.

And what enforces that e.g. the kernel image has an arbvn? IOW, what
prevents me from skipping that check by deleting that property? The
cover letter says that the arbvn is part of the signed data, but I can't
find where that is actually implemented.

>> As I said above, I think the version
>> number should really be a property of the whole configuration. 
> I have considered the above simpler option. But I believe image specific arbvn
> is more appropriate as the security features and security bug fixes are implemented
> in images than in config sub-nodes.

Yes, I can see that in the vast majority of cases (there are also cases
where it would be the combination of one kernel version with one
specific device tree, but that's of course quite rare). And I don't
really care that much if the version lives with each image or in the
conf node (though ideally I'd want to be able to, when using signed
configurations, just having one version apply to that configuration as a
whole - but I guess I could do that by having all configurations include
a pseudo-image which just carries that global version number). What I do
care about is that the "version" format and semantics can be defined by
the individual product.

Rasmus

[RFC PATCH 1/1] image: add anti rollback protection for FIT Images

[Tom Rini]

On Mon, Sep 14, 2020 at 11:18:25PM -0700, Thirupathaiah Annapureddy wrote:
> Hi Simon,
> 
> Thanks for the review.
> 
> On 9/6/2020 6:43 PM, Simon Glass wrote:
> >>
> >> diff --git a/Kconfig b/Kconfig
> >> index 883e3f71d0..3959a6592c 100644
> >> --- a/Kconfig
> >> +++ b/Kconfig
> >> @@ -533,6 +533,15 @@ config FIT_CIPHER
> >>           Enable the feature of data ciphering/unciphering in the tool mkimage
> >>           and in the u-boot support of the FIT image.
> >>
> >> +config FIT_ARBP
> > 
> > How about using ROLLBACK instead of ARBP. It is easier to understand.Looks good to me. I will change it in the next version of the patch.
> 
> >> +{
> >> +       uint8_t type;
> >> +       uint32_t image_arbvn;
> >> +       uint32_t plat_arbvn = 0;
> > 
> > Those three can be uint.
> fit_image_get_type() returns type as uint8_t. 
> I can change it for the other two variables. 
> 
> >>  static int fit_config_verify_sig(const void *fit, int conf_noffset,
> >>                                  const void *sig_blob, int sig_offset)
> >>  {
> >> @@ -401,6 +472,14 @@ static int fit_config_verify_sig(const void *fit, int conf_noffset,
> >>                 goto error;
> >>         }
> >>
> >> +#if !defined(USE_HOSTCC)
> > 
> > Do we need this ?ifdef, or can we rely on IMAGE_ENABLE_ARBP?
> I believe we can rely on just IMAGE_ENABLE_ARBP.
> 
> >>  #define FIT_LOAD_PROP          "load"
> >> +#define FIT_ARBVN_PROP         "arbvn"
> > 
> > ROLLBACK / "rollback"
> I will fix it in the next version.
> 
> > 
> >>
> >>  /* configuration node */
> >>  #define FIT_KERNEL_PROP                "kernel"
> >> @@ -1085,6 +1086,7 @@ int fit_image_get_data_size_unciphered(const void *fit, int noffset,
> >>                                        size_t *data_size);
> >>  int fit_image_get_data_and_size(const void *fit, int noffset,
> >>                                 const void **data, size_t *size);
> >> +int fit_image_get_arbvn(const void *fit, int noffset, uint32_t *arbvn);
> > 
> > Please add a full function comment
> comment was added before the function definition to be consistent
> with other functions.
> 
> >> +int board_get_arbvn(uint8_t ih_type, uint32_t *arbvn);
> > 
> > This needs a driver since the rollback counter may be implemented by a
> > TPM or anything. 
> Board specific hooks can leverage TPM library functions in that case.
> May I know why a driver is needed?

Sorry for not getting in to this series sooner.  One thing that I think
would be very helpful is to see is a full demonstration on say a
Raspberry Pi.  I know I have a TPM2 module that supports Pi sitting
around here.  I assume you've also tested this on some HW platform.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200915/b25f6483/attachment.sig>

[RFC PATCH 1/1] image: add anti rollback protection for FIT Images

[Thirupathaiah Annapureddy]

Hi Tom,

Please see my comment(s) in-line.

On 9/15/2020 6:40 AM, Tom Rini wrote:
> On Mon, Sep 14, 2020 at 11:18:25PM -0700, Thirupathaiah Annapureddy wrote:
>> Hi Simon,
>>
>> Thanks for the review.
>>
>> On 9/6/2020 6:43 PM, Simon Glass wrote:
>>>>
>>>> diff --git a/Kconfig b/Kconfig
>>>> index 883e3f71d0..3959a6592c 100644
>>>> --- a/Kconfig
>>>> +++ b/Kconfig
>>>> @@ -533,6 +533,15 @@ config FIT_CIPHER
>>>>           Enable the feature of data ciphering/unciphering in the tool mkimage
>>>>           and in the u-boot support of the FIT image.
>>>>
>>>> +config FIT_ARBP
>>>
>>> How about using ROLLBACK instead of ARBP. It is easier to understand.Looks good to me. I will change it in the next version of the patch.
>>
>>>> +{
>>>> +       uint8_t type;
>>>> +       uint32_t image_arbvn;
>>>> +       uint32_t plat_arbvn = 0;
>>>
>>> Those three can be uint.
>> fit_image_get_type() returns type as uint8_t. 
>> I can change it for the other two variables. 
>>
>>>>  static int fit_config_verify_sig(const void *fit, int conf_noffset,
>>>>                                  const void *sig_blob, int sig_offset)
>>>>  {
>>>> @@ -401,6 +472,14 @@ static int fit_config_verify_sig(const void *fit, int conf_noffset,
>>>>                 goto error;
>>>>         }
>>>>
>>>> +#if !defined(USE_HOSTCC)
>>>
>>> Do we need this ?ifdef, or can we rely on IMAGE_ENABLE_ARBP?
>> I believe we can rely on just IMAGE_ENABLE_ARBP.
>>
>>>>  #define FIT_LOAD_PROP          "load"
>>>> +#define FIT_ARBVN_PROP         "arbvn"
>>>
>>> ROLLBACK / "rollback"
>> I will fix it in the next version.
>>
>>>
>>>>
>>>>  /* configuration node */
>>>>  #define FIT_KERNEL_PROP                "kernel"
>>>> @@ -1085,6 +1086,7 @@ int fit_image_get_data_size_unciphered(const void *fit, int noffset,
>>>>                                        size_t *data_size);
>>>>  int fit_image_get_data_and_size(const void *fit, int noffset,
>>>>                                 const void **data, size_t *size);
>>>> +int fit_image_get_arbvn(const void *fit, int noffset, uint32_t *arbvn);
>>>
>>> Please add a full function comment
>> comment was added before the function definition to be consistent
>> with other functions.
>>
>>>> +int board_get_arbvn(uint8_t ih_type, uint32_t *arbvn);
>>>
>>> This needs a driver since the rollback counter may be implemented by a
>>> TPM or anything. 
>> Board specific hooks can leverage TPM library functions in that case.
>> May I know why a driver is needed?
> 
> Sorry for not getting in to this series sooner.  One thing that I think
> would be very helpful is to see is a full demonstration on say a
> Raspberry Pi.  I know I have a TPM2 module that supports Pi sitting
> around here.  I assume you've also tested this on some HW platform.
> 

We test patches on our own hardware. But I agree demonstration of this
feature on more widely available hardware is useful. I will include
board (ex: Raspberry Pi) specific changes in the next version of the patch
series.

Best Regards,
Thiru

[RFC PATCH 1/1] image: add anti rollback protection for FIT Images

[Simon Glass]

Hi Thirupathaiah,

On Tue, 15 Sep 2020 at 00:18, Thirupathaiah Annapureddy
<thiruan@linux.microsoft.com> wrote:
>
> Hi Simon,
>
> Thanks for the review.
>
> On 9/6/2020 6:43 PM, Simon Glass wrote:
> >>
> >> diff --git a/Kconfig b/Kconfig
> >> index 883e3f71d0..3959a6592c 100644
> >> --- a/Kconfig
> >> +++ b/Kconfig
> >> @@ -533,6 +533,15 @@ config FIT_CIPHER
> >>           Enable the feature of data ciphering/unciphering in the tool mkimage
> >>           and in the u-boot support of the FIT image.
> >>
> >> +config FIT_ARBP
> >
> > How about using ROLLBACK instead of ARBP. It is easier to understand.Looks good to me. I will change it in the next version of the patch.
>
> >> +{
> >> +       uint8_t type;
> >> +       uint32_t image_arbvn;
> >> +       uint32_t plat_arbvn = 0;
> >
> > Those three can be uint.
> fit_image_get_type() returns type as uint8_t.
> I can change it for the other two variables.

My point here is just that you are declaring variables which end up
being in registers. It doesn't make sense to try to choose a small
type just because it will fit. The machine registers are 32/64-bits.
Using an 8-bit variable can only increase code size as the compiler
may have to mask things off before storing and passing values.

It is fine to use these sorts of types in a data structure to save
memory, or in a protocol to ensure the bits are correct, but it really
doesn't make sense for local variables and parameters.

[..]

> >> +int board_get_arbvn(uint8_t ih_type, uint32_t *arbvn);
> >
> > This needs a driver since the rollback counter may be implemented by a
> > TPM or anything.
> Board specific hooks can leverage TPM library functions in that case.
> May I know why a driver is needed?

U-Boot uses driver model to deal with the complexity of the system. It
allows the system to be described by devicetree, it allows different
drivers to be created to implement the same functionality. It is more
discoverable than weak functions and less error-prone.

>
> > If you want to use the board, add a new
> > get_rollback() to UCLASS_BOARD (board.h). Or you could create a new
> > UCLASS_SECURITY which includes these two API calls.
> I explored the option of using UCLASS_BOARD. But it does not have "set"
> interfaces and the "id" parameter used in "get" functions seem to be
> board specific. We can look into the option of UCLASS_SECURITY for these
> two API calls.

OK that sounds good.

[..]

Regards,
Simon