Xen Security Advisory 437 v2 (CVE-2023-34321) - arm32: The cache may not be properly cleaned/invalidated

Xen.org security team posted 1 patch 7 months, 3 weeks ago
Failed in applying to current master (apply log)
xen/arch/arm/include/asm/page.h | 33 ++++++++++++++++++++-------------
1 file changed, 20 insertions(+), 13 deletions(-)
Xen Security Advisory 437 v2 (CVE-2023-34321) - arm32: The cache may not be properly cleaned/invalidated
Posted by Xen.org security team 7 months, 3 weeks ago
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

            Xen Security Advisory CVE-2023-34321 / XSA-437
                               version 2

            arm32: The cache may not be properly cleaned/invalidated

UPDATES IN VERSION 2
====================

Public release.

ISSUE DESCRIPTION
=================

Arm provides multiple helpers to clean & invalidate the cache
for a given region.  This is, for instance, used when allocating
guest memory to ensure any writes (such as the ones during scrubbing)
have reached memory before handing over the page to a guest.

Unfortunately, the arithmetics in the helpers can overflow and would
then result to skip the cache cleaning/invalidation.  Therefore there
is no guarantee when all the writes will reach the memory.

IMPACT
======

A malicious guest may be able to read sensitive data from memory that
previously belonged to another guest.

VULNERABLE SYSTEMS
==================

Systems running all version of Xen are affected.

Only systems running Xen on Arm 32-bit are vulnerable.  Xen on Arm 64-bit
is not affected.

MITIGATION
==========

There is no known mitigation.

CREDITS
=======

This issue was discovered by Julien Grall of Amazon.

RESOLUTION
==========

Applying the appropriate attached patch resolves this issue.

Note that patches for released versions are generally prepared to
apply to the stable branches, and may not apply cleanly to the most
recent release tarball.  Downstreams are encouraged to update to the
tip of the stable branch before applying these patches.

xsa437/xsa437.patch           xen-unstable - Xen 4.17.x
xsa437/xsa437-4.16.patch      Xen 4.16.x - Xen 4.15.x

$ sha256sum xsa437* xsa437*/*
259b872275d9d77fc1744df886ffe611d933889bb5ea2833f3c7d8f554eff061  xsa437.meta
31b1a4050403fc83d4ea7619155105001cfd2f739ceb0b0cc7212ab7d0b9d559  xsa437/xsa437.patch
ada8ba64e8562ff6016d456e08b7a171ef356cf476c643df9f66b8650009115c  xsa437/xsa437-4.16.patch
$

DEPLOYMENT DURING EMBARGO
=========================

Deployment of the patches and/or mitigations described above (or
others which are substantially similar) is permitted during the
embargo, even on public-facing systems with untrusted guest users and
administrators.

But: Distribution of updated software is prohibited (except to other
members of the predisclosure list).

Predisclosure list members who wish to deploy significantly different
patches and/or mitigations, please contact the Xen Project Security
Team.

(Note: this during-embargo deployment notice is retained in
post-embargo publicly released Xen Project advisories, even though it
is then no longer applicable.  This is to enable the community to have
oversight of the Xen Project Security Team's decisionmaking.)

For more information about permissible uses of embargoed information,
consult the Xen Project community's agreed Security Policy:
  http://www.xenproject.org/security-policy.html
-----BEGIN PGP SIGNATURE-----

iQFABAEBCAAqFiEEI+MiLBRfRHX6gGCng/4UyVfoK9kFAmTorfoMHHBncEB4ZW4u
b3JnAAoJEIP+FMlX6CvZIv8H/1Grce6f0aytYn0WTXyMdEXtUCkaHQd/pkNkXTe4
uOfNTBM0z2m6MUBATFNUyTiBqm+I8ywZWDp5UVW8nD2YF2hEIGrhdkDMK+cQg98q
iZ+RW4W0cIjZFTbYXRRUm6RPhp31cx4kvTHKk2+imD1bTa/4SVFyDy2ps5ybim9b
1QnPw2+Kbvd2orx6VHpCjnpTqsElRRA1phN9t87UZhgFBCeeatYizHNNqUrvBZXg
UPsB3ERyxAyMqET82jGboUfwmjpctr1I+p9UvEvY9aViSXy+SMnNi84fFSzBrOXr
EaKUg0glvV3uaNwbvJQfmgkhDUOwXN/ySO7Hcu7QpfmUn70=
=2wxR
-----END PGP SIGNATURE-----
{
  "XSA": 437,
  "SupportedVersions": [
    "master",
    "4.17",
    "4.16",
    "4.15"
  ],
  "Trees": [
    "xen"
  ],
  "Recipes": {
    "4.15": {
      "Recipes": {
        "xen": {
          "StableRef": "df3395f6b2d759aba39fb67a7bc0fe49147c8b39",
          "Prereqs": [],
          "Patches": [
            "xsa437/xsa437-4.16.patch"
          ]
        }
      }
    },
    "4.16": {
      "Recipes": {
        "xen": {
          "StableRef": "89fe6d0edea841d1d2690cf3f5173e334c687823",
          "Prereqs": [],
          "Patches": [
            "xsa437/xsa437-4.16.patch"
          ]
        }
      }
    },
    "4.17": {
      "Recipes": {
        "xen": {
          "StableRef": "3141a0b85c37b76e069ec7dcb906ff202f5c4075",
          "Prereqs": [],
          "Patches": [
            "xsa437/xsa437.patch"
          ]
        }
      }
    },
    "master": {
      "Recipes": {
        "xen": {
          "StableRef": "01ca29f0b17a50a94b0e232ba276c32e95d80ae3",
          "Prereqs": [],
          "Patches": [
            "xsa437/xsa437.patch"
          ]
        }
      }
    }
  }
}
From 7fac5971340a13ca9458195305bcfe14df2e52d2 Mon Sep 17 00:00:00 2001
From: Stefano Stabellini <stefano.stabellini@amd.com>
Date: Thu, 17 Aug 2023 13:41:35 +0100
Subject: [PATCH] xen/arm: page: Handle cache flush of an element at the top of
 the address space

The region that needs to be cleaned/invalidated may be at the top
of the address space. This means that 'end' (i.e. 'p + size') will
be 0 and therefore nothing will be cleaned/invalidated as the check
in the loop will always be false.

On Arm64, we only support we only support up to 48-bit Virtual
address space. So this is not a concern there. However, for 32-bit,
the mapcache is using the last 2GB of the address space. Therefore
we may not clean/invalidate properly some pages. This could lead
to memory corruption or data leakage (the scrubbed value may
still sit in the cache when the guest could read directly the memory
and therefore read the old content).

Rework invalidate_dcache_va_range(), clean_dcache_va_range(),
clean_and_invalidate_dcache_va_range() to handle a cache flush
with an element at the top of the address space.

This is CVE-2023-34321 / XSA-437.

Reported-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>
Acked-by: Bertrand Marquis <bertrand.marquis@arm.com>

---
 xen/arch/arm/include/asm/page.h | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/include/asm/page.h b/xen/arch/arm/include/asm/page.h
index e7cd62190c7f..d7fe770a5e49 100644
--- a/xen/arch/arm/include/asm/page.h
+++ b/xen/arch/arm/include/asm/page.h
@@ -160,26 +160,25 @@ static inline size_t read_dcache_line_bytes(void)
 
 static inline int invalidate_dcache_va_range(const void *p, unsigned long size)
 {
-    const void *end = p + size;
     size_t cacheline_mask = dcache_line_bytes - 1;
 
     dsb(sy);           /* So the CPU issues all writes to the range */
 
     if ( (uintptr_t)p & cacheline_mask )
     {
+        size -= dcache_line_bytes - ((uintptr_t)p & cacheline_mask);
         p = (void *)((uintptr_t)p & ~cacheline_mask);
         asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (p));
         p += dcache_line_bytes;
     }
-    if ( (uintptr_t)end & cacheline_mask )
-    {
-        end = (void *)((uintptr_t)end & ~cacheline_mask);
-        asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (end));
-    }
 
-    for ( ; p < end; p += dcache_line_bytes )
+    for ( ; size >= dcache_line_bytes;
+            p += dcache_line_bytes, size -= dcache_line_bytes )
         asm volatile (__invalidate_dcache_one(0) : : "r" (p));
 
+    if ( size > 0 )
+        asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (p));
+
     dsb(sy);           /* So we know the flushes happen before continuing */
 
     return 0;
@@ -187,10 +186,14 @@ static inline int invalidate_dcache_va_range(const void *p, unsigned long size)
 
 static inline int clean_dcache_va_range(const void *p, unsigned long size)
 {
-    const void *end = p + size;
+    size_t cacheline_mask = dcache_line_bytes - 1;
+
     dsb(sy);           /* So the CPU issues all writes to the range */
-    p = (void *)((uintptr_t)p & ~(dcache_line_bytes - 1));
-    for ( ; p < end; p += dcache_line_bytes )
+    size += (uintptr_t)p & cacheline_mask;
+    size = (size + cacheline_mask) & ~cacheline_mask;
+    p = (void *)((uintptr_t)p & ~cacheline_mask);
+    for ( ; size >= dcache_line_bytes;
+            p += dcache_line_bytes, size -= dcache_line_bytes )
         asm volatile (__clean_dcache_one(0) : : "r" (p));
     dsb(sy);           /* So we know the flushes happen before continuing */
     /* ARM callers assume that dcache_* functions cannot fail. */
@@ -200,10 +203,14 @@ static inline int clean_dcache_va_range(const void *p, unsigned long size)
 static inline int clean_and_invalidate_dcache_va_range
     (const void *p, unsigned long size)
 {
-    const void *end = p + size;
+    size_t cacheline_mask = dcache_line_bytes - 1;
+
     dsb(sy);         /* So the CPU issues all writes to the range */
-    p = (void *)((uintptr_t)p & ~(dcache_line_bytes - 1));
-    for ( ; p < end; p += dcache_line_bytes )
+    size += (uintptr_t)p & cacheline_mask;
+    size = (size + cacheline_mask) & ~cacheline_mask;
+    p = (void *)((uintptr_t)p & ~cacheline_mask);
+    for ( ; size >= dcache_line_bytes;
+            p += dcache_line_bytes, size -= dcache_line_bytes )
         asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (p));
     dsb(sy);         /* So we know the flushes happen before continuing */
     /* ARM callers assume that dcache_* functions cannot fail. */
-- 
2.40.1

From 2b21319eecd8623078f27f0bccbbfdeb26606ff1 Mon Sep 17 00:00:00 2001
From: Stefano Stabellini <stefano.stabellini@amd.com>
Date: Thu, 17 Aug 2023 13:41:35 +0100
Subject: [PATCH] xen/arm: page: Handle cache flush of an element at the top of
 the address space

The region that needs to be cleaned/invalidated may be at the top
of the address space. This means that 'end' (i.e. 'p + size') will
be 0 and therefore nothing will be cleaned/invalidated as the check
in the loop will always be false.

On Arm64, we only support we only support up to 48-bit Virtual
address space. So this is not a concern there. However, for 32-bit,
the mapcache is using the last 2GB of the address space. Therefore
we may not clean/invalidate properly some pages. This could lead
to memory corruption or data leakage (the scrubbed value may
still sit in the cache when the guest could read directly the memory
and therefore read the old content).

Rework invalidate_dcache_va_range(), clean_dcache_va_range(),
clean_and_invalidate_dcache_va_range() to handle a cache flush
with an element at the top of the address space.

This is CVE-2023-34321 / XSA-437.

Reported-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>
Acked-by: Bertrand Marquis <bertrand.marquis@arm.com>

---
 xen/include/asm-arm/page.h | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index c6f9fb0d4e0c..eff5883ef87b 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -152,26 +152,25 @@ static inline size_t read_dcache_line_bytes(void)
 
 static inline int invalidate_dcache_va_range(const void *p, unsigned long size)
 {
-    const void *end = p + size;
     size_t cacheline_mask = dcache_line_bytes - 1;
 
     dsb(sy);           /* So the CPU issues all writes to the range */
 
     if ( (uintptr_t)p & cacheline_mask )
     {
+        size -= dcache_line_bytes - ((uintptr_t)p & cacheline_mask);
         p = (void *)((uintptr_t)p & ~cacheline_mask);
         asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (p));
         p += dcache_line_bytes;
     }
-    if ( (uintptr_t)end & cacheline_mask )
-    {
-        end = (void *)((uintptr_t)end & ~cacheline_mask);
-        asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (end));
-    }
 
-    for ( ; p < end; p += dcache_line_bytes )
+    for ( ; size >= dcache_line_bytes;
+            p += dcache_line_bytes, size -= dcache_line_bytes )
         asm volatile (__invalidate_dcache_one(0) : : "r" (p));
 
+    if ( size > 0 )
+        asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (p));
+
     dsb(sy);           /* So we know the flushes happen before continuing */
 
     return 0;
@@ -179,10 +178,14 @@ static inline int invalidate_dcache_va_range(const void *p, unsigned long size)
 
 static inline int clean_dcache_va_range(const void *p, unsigned long size)
 {
-    const void *end = p + size;
+    size_t cacheline_mask = dcache_line_bytes - 1;
+
     dsb(sy);           /* So the CPU issues all writes to the range */
-    p = (void *)((uintptr_t)p & ~(dcache_line_bytes - 1));
-    for ( ; p < end; p += dcache_line_bytes )
+    size += (uintptr_t)p & cacheline_mask;
+    size = (size + cacheline_mask) & ~cacheline_mask;
+    p = (void *)((uintptr_t)p & ~cacheline_mask);
+    for ( ; size >= dcache_line_bytes;
+            p += dcache_line_bytes, size -= dcache_line_bytes )
         asm volatile (__clean_dcache_one(0) : : "r" (p));
     dsb(sy);           /* So we know the flushes happen before continuing */
     /* ARM callers assume that dcache_* functions cannot fail. */
@@ -192,10 +195,14 @@ static inline int clean_dcache_va_range(const void *p, unsigned long size)
 static inline int clean_and_invalidate_dcache_va_range
     (const void *p, unsigned long size)
 {
-    const void *end = p + size;
+    size_t cacheline_mask = dcache_line_bytes - 1;
+
     dsb(sy);         /* So the CPU issues all writes to the range */
-    p = (void *)((uintptr_t)p & ~(dcache_line_bytes - 1));
-    for ( ; p < end; p += dcache_line_bytes )
+    size += (uintptr_t)p & cacheline_mask;
+    size = (size + cacheline_mask) & ~cacheline_mask;
+    p = (void *)((uintptr_t)p & ~cacheline_mask);
+    for ( ; size >= dcache_line_bytes;
+            p += dcache_line_bytes, size -= dcache_line_bytes )
         asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (p));
     dsb(sy);         /* So we know the flushes happen before continuing */
     /* ARM callers assume that dcache_* functions cannot fail. */
-- 
2.40.1