FVP: Fix plat_set_nv_ctr() function

The Fast Models provide a non-volatile counter component, which is used
in the Trusted Board Boot implementation to protect against rollback
attacks.

This component comes in 2 versions (see [1]).

- Version 0 is the default and models a locked non-volatile counter,
  whose value is fixed.

- Version 1 of the counter may be incremented in a monotonic fashion.

plat_set_nv_ctr() must cope with both versions. This is achieved by:
1) Attempting to write the new value in the counter.
2) Reading the value back.
3) If there is a mismatch, we know the counter upgrade failed.

When using version 0 of the counter, no upgrade is possible so the
function is expected to fail all the time. However, the code is
missing a compiler barrier between the write operation and the next
read. Thus, the compiler may optimize and remove the read operation on
the basis that the counter value has not changed. With the default
optimization level used in TF-A (-Os), this is what's happening.

The fix introduced in this patch marks the write and subsequent read
accesses to the counter as volatile, such that the compiler makes no
assumption about the value of the counter.

Note that the comment above plat_set_nv_ctr() was clearly stating
that when using the read-only version of the non-volatile counter,
"we expect the values in the certificates to always match the RO
values so that this function is never called". However, the fact that
the counter value was read back seems to contradict this comment, as
it is implementing a counter-measure against misuse of the
function. The comment has been reworded to avoid any confusion.

Without this patch, this bug may be demonstrated on the Base AEM FVP:
- Using version 0 of the non-volatile counter (default version).
- With certificates embedding a revision number value of 32
  (compiling TF-A with TFW_NVCTR_VAL=32).

In this configuration, the non-volatile counter is tied to value 31 by
default. When BL1 loads the Trusted Boot Firmware certificate, it
notices that the two values do not match and tries to upgrade the
non-volatile counter. This write operation is expected to fail
(because the counter is locked) and the function is expected to return
an error but it succeeds instead.

As a result, the trusted boot does not abort as soon as it should and
incorrectly boots BL2. The boot is finally aborted when BL2 verifies
the BL31 image and figures out that the version of the SoC Firmware
Key Certificate does not match. On Arm platforms, only certificates
signed with the Root-of-Trust Key may trigger an upgrade of the
non-volatile Trusted counter.

[1] https://developer.arm.com/docs/100964/1160/fast-models-components/peripheral-components/nonvolatilecounter

Change-Id: I9979f29c23b47b338b9b484013d1fb86c59db92f
Signed-off-by: Sandrine Bailleux <sandrine.bailleux@arm.com>

diff --git a/plat/arm/board/fvp/fvp_trusted_boot.c b/plat/arm/board/fvp/fvp_trusted_boot.c
index 0d160cb1ddad33c1aca33cfec406db98d1db2ba9..dc50764356d8870dbbc704f101c032f6e9de8935 100644 (file)
--- a/plat/arm/board/fvp/fvp_trusted_boot.c
+++ b/plat/arm/board/fvp/fvp_trusted_boot.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016-2018, ARM Limited and Contributors. All rights reserved.
+ * Copyright (c) 2016-2019, ARM Limited and Contributors. All rights reserved.
  *
  * SPDX-License-Identifier: BSD-3-Clause
  */
@@ -8,39 +8,41 @@
 #include <stdint.h>
 #include <string.h>
 
+#include <lib/mmio.h>
+
 #include <plat/common/platform.h>
 #include <platform_def.h>
 #include <tools_share/tbbr_oid.h>
 
 /*
- * Store a new non-volatile counter value. On some FVP versions, the
- * non-volatile counters are RO. On these versions we expect the values in the
- * certificates to always match the RO values so that this function is never
- * called.
+ * Store a new non-volatile counter value.
+ *
+ * On some FVP versions, the non-volatile counters are read-only so this
+ * function will always fail.
  *
  * Return: 0 = success, Otherwise = error
  */
 int plat_set_nv_ctr(void *cookie, unsigned int nv_ctr)
 {
        const char *oid;
-       uint32_t *nv_ctr_addr;
+       uintptr_t nv_ctr_addr;
 
        assert(cookie != NULL);
 
        oid = (const char *)cookie;
        if (strcmp(oid, TRUSTED_FW_NVCOUNTER_OID) == 0) {
-               nv_ctr_addr = (uint32_t *)TFW_NVCTR_BASE;
+               nv_ctr_addr = TFW_NVCTR_BASE;
        } else if (strcmp(oid, NON_TRUSTED_FW_NVCOUNTER_OID) == 0) {
-               nv_ctr_addr = (uint32_t *)NTFW_CTR_BASE;
+               nv_ctr_addr = NTFW_CTR_BASE;
        } else {
                return 1;
        }
 
-       *(unsigned int *)nv_ctr_addr = nv_ctr;
-
-       /* Verify that the current value is the one we just wrote. */
-       if (nv_ctr != (unsigned int)(*nv_ctr_addr))
-               return 1;
+       mmio_write_32(nv_ctr_addr, nv_ctr);
 
-       return 0;
+       /*
+        * If the FVP models a locked counter then its value cannot be updated
+        * and the above write operation has been silently ignored.
+        */
+       return (mmio_read_32(nv_ctr_addr) == nv_ctr) ? 0 : 1;
 }