Re: [PATCH 3/5] common: Add OS anti-rollback validation using security devices

Hi Sean,

On Fri, 11 Aug 2023 at 18:28, seanedmond@linux.microsoft.com wrote:

From: Stephen Carlson stcarlso@linux.microsoft.com

New config CONFIG_ARBP to enable enforcement of OS anti-rollback counter during image loading.

Images with an anti-rollback counter value "arbvn" declared in the FDT will be compared against the current device anti-rollback counter value, and older images will not pass signature validation. If the image is newer, the device anti-rollback counter value will be updated.

Signed-off-by: Stephen Carlson stcarlso@linux.microsoft.com

boot/Kconfig | 9 +++++ boot/image-fit-sig.c | 89 ++++++++++++++++++++++++++++++++++++++++++++ boot/image-fit.c | 23 ++++++++++++ include/image.h | 4 ++ 4 files changed, 125 insertions(+)

diff --git a/boot/Kconfig b/boot/Kconfig index e8fb03b801..e08c274b7c 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -103,6 +103,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

FIT_ROLLBACK would be better

arbp is really horrible :-)

•   bool "Enable Anti rollback version check for FIT images"
  

•   depends on FIT_SIGNATURE
  

•   default n
  

•   help
  

•     Enables FIT image anti-rollback protection. This feature is required
  

•     when a platform needs to retire previous versions of FIT images due to
  

•     security flaws and prevent devices from being reverted to them.
  

config FIT_VERBOSE bool "Show verbose messages when FIT images fail" depends on FIT diff --git a/boot/image-fit-sig.c b/boot/image-fit-sig.c index 12369896fe..bf3b81a3a3 100644 --- a/boot/image-fit-sig.c +++ b/boot/image-fit-sig.c @@ -11,6 +11,8 @@ #include <log.h> #include <malloc.h> #include <asm/global_data.h> +#include <dm.h> +#include <dm-security.h>

You don't need dm- in your headerfiles. I think this should be rolllback.h and that should be the name of your uclass.

DECLARE_GLOBAL_DATA_PTR; #endif /* !USE_HOSTCC*/ #include <fdt_region.h> @@ -63,6 +65,39 @@ struct image_region *fit_region_make_list(const void *fit, return region; }

+#if !defined(USE_HOSTCC)

Can we drop that?

+static int fit_image_verify_arbvn(const void *fit, int image_noffset) +{

•   u64 image_arbvn;
  

•   u64 plat_arbvn = 0ULL;
  

•   struct udevice *dev;
  

•   int ret;
  

•   ret = fit_image_get_arbvn(fit, image_noffset, &image_arbvn);
  

•   if (ret)
  

•           return 0;
  

?? Isn't this an error?

•   ret = uclass_first_device_err(UCLASS_SECURITY, &dev);
  

•   if (ret)
  

•           return -ENODEV;
  

return ret

•   ret = dm_security_arbvn_get(dev, &plat_arbvn);
  

•   if (ret)
  

•           return -EIO;
  

•   if (image_arbvn < plat_arbvn) {
  

•           return -EPERM;
  

•   } else if (image_arbvn > plat_arbvn) {
  

•           ret = dm_security_arbvn_set(dev, image_arbvn);
  

•           printf(" Updating OS anti-rollback to %llu from %llu\n",
  

•                  image_arbvn, plat_arbvn);
  

So the update happens in U-Boot? Don't we want to update it when we know it boots?

•           return ret;
  

•   }
  

•   return 0;
  

+} +#endif

static int fit_image_setup_verify(struct image_sign_info *info, const void *fit, int noffset, const void *key_blob, int required_keynode, @@ -175,6 +210,16 @@ static int fit_image_verify_sig(const void *fit, int image_noffset, goto error; }

+#if !defined(USE_HOSTCC)

Can you use

if (!tools_build())

?

This seems to be adding to FIT so the FIT docs should be updated.

Regards, Simon