Xen Security Advisory 361 v4 (CVE-2021-26932) - Linux: grant mapping error handling issues

Xen Security Advisory 361 v4 (CVE-2021-26932) - Linux: grant mapping error handling issues

[Xen.org security team]

[-- Attachment #1: Type: text/plain, Size: 4874 bytes --]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

            Xen Security Advisory CVE-2021-26932 / XSA-361
                               version 4

                Linux: grant mapping error handling issues

UPDATES IN VERSION 4
====================

Public release.

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

Grant mapping operations often occur in batch hypercalls, where a
number of operations are done in a single hypercall, the success or
failure of each one reported to the backend driver, and the backend
driver then loops over the results, performing follow-up actions based
on the success or failure of each operation.

Unfortunately, when running in PV mode, the Linux backend drivers
mishandle this: Some errors are ignored, effectively implying their
success from the success of related batch elements.  In other cases,
errors resulting from one batch element lead to further batch elements
not being inspected, and hence successful ones to not be possible to
properly unmap upon error recovery.

IMPACT
======

A malicious or buggy frontend driver may be able to crash the
corresponding backend driver, causing a denial of service potentially
affecting the entire domain running the backend driver.

A malicious or buggy frontend driver may be able to cause resource
leaks in the domain running the corresponding backend driver, leading
to a denial of service.

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

All Linux versions back to at least 3.2 are vulnerable, when running in
PV mode on x86 or when running on Arm.

On x86, only systems with Linux backends running in PV mode are
vulnerable.  Linux backends run in HVM / PVH modes are not vulnerable.

MITIGATION
==========

On x86, running the backends in HVM or PVH domains will avoid the
vulnerability.

For protocols where other, e.g. non-kernel-based backends are available,
reconfiguring guests to use alternative (e.g. qemu-based) backends may
allow to avoid the vulnerability as long as these backends don't rely on
similar functionality provided by the xen-gntdev (/dev/gntdev) driver.

In all other cases there is no known mitigation.

CREDITS
=======

This issue was discovered by Jan Beulich of SUSE.

RESOLUTION
==========

Applying the attached patches resolves this issue.

xsa361-linux-1.patch           Linux 5.11-rc - 3.19
xsa361-linux-2.patch           Linux 5.11-rc - 3.15
xsa361-linux-3.patch           Linux 5.11-rc - 4.19
xsa361-linux-4.patch           Linux 5.11-rc - 4.19
xsa361-linux-5.patch           Linux 5.11-rc - 4.4

$ sha256sum xsa361*
bb00ab6319b4fc536566af50c73e064f10f8b99eaa6b0f0b35a8d174c285a905  xsa361-linux-1.patch
73b6a54aa3773ce11f0de6b9aa1d80dd7f4c297dc71924b1a3886bc3b99ac859  xsa361-linux-2.patch
8e554cfab8cdb4fe1b74601a9432ea4c570f74a952ad757f9294ba1666cbeaea  xsa361-linux-3.patch
8c290895d10fc148f99e2a6587811b3037f29c3a0201d69d448ff520cea6f96d  xsa361-linux-4.patch
231ae3e1b9bec1b75dbbbee4b5acff620ef7ac2853332aa7b3c4957c6ca7f341  xsa361-linux-5.patch
$

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

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

Deployment of the mitigation to switch to HVM / PVH backend domains is
also permitted during the embargo, even on public-facing systems with
untrusted guest users and administrators.

HOWEVER, deployment of the non-kernel-based backends mitigation
described above is NOT permitted during the embargo on public-facing
systems with untrusted guest users and administrators.  This is because
such a configuration change may be recognizable by the affected guests.

AND: 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/4UyVfoK9kFAmAru/QMHHBncEB4ZW4u
b3JnAAoJEIP+FMlX6CvZmFkH/Ay1RoZbbcA4ywdhy9xdnpt0DHMFLjZSbE4sNTi+
J+m9rn69UTK01VDD0RUohTcmWO0nv8ZD+jKETsSq31GiYhVk7XnSmCJkzILGujr8
cf+7jUWWJPcqBmN7xcLBaor9lhpKfMpYlMLBG7twIRHfqOSw6Sm+iD4YC23nkGKF
Cb8tpkYCpX3dPMMP74nX00Wta2rqd1BrpAGvAnt9hrHIBfTcpwWE8A4H1eFL/7Dv
5+pVvrSMkyzaR5kI/QBeriXsuOP509CiafUBpeXU85pGWpLgZAqD+puodEVQ2fpT
/MqATdNRhgnCzqSqh/ElN/1ZdB7406DbdCnErJiyDdN/OCE=
=DUXr
-----END PGP SIGNATURE-----

[-- Attachment #2: xsa361-linux-1.patch --]
[-- Type: application/octet-stream, Size: 1669 bytes --]

From: Jan Beulich <jbeulich@suse.com>
Subject: Xen/x86: don't bail early from clear_foreign_p2m_mapping()

Its sibling (set_foreign_p2m_mapping()) as well as the sibling of its
only caller (gnttab_map_refs()) don't clean up after themselves in case
of error. Higher level callers are expected to do so. However, in order
for that to really clean up any partially set up state, the operation
should not terminate upon encountering an entry in unexpected state. It
is particularly relevant to notice here that set_foreign_p2m_mapping()
would skip setting up a p2m entry if its grant mapping failed, but it
would continue to set up further p2m entries as long as their mappings
succeeded.

Arguably down the road set_foreign_p2m_mapping() may want its page state
related WARN_ON() also converted to an error return.

This is part of XSA-361.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Cc: stable@vger.kernel.org
Reviewed-by: Juergen Gross <jgross@suse.com>

--- a/arch/x86/xen/p2m.c
+++ b/arch/x86/xen/p2m.c
@@ -750,17 +750,15 @@ int clear_foreign_p2m_mapping(struct gnt
 		unsigned long mfn = __pfn_to_mfn(page_to_pfn(pages[i]));
 		unsigned long pfn = page_to_pfn(pages[i]);
 
-		if (mfn == INVALID_P2M_ENTRY || !(mfn & FOREIGN_FRAME_BIT)) {
+		if (mfn != INVALID_P2M_ENTRY && (mfn & FOREIGN_FRAME_BIT))
+			set_phys_to_machine(pfn, INVALID_P2M_ENTRY);
+		else
 			ret = -EINVAL;
-			goto out;
-		}
-
-		set_phys_to_machine(pfn, INVALID_P2M_ENTRY);
 	}
 	if (kunmap_ops)
 		ret = HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref,
-						kunmap_ops, count);
-out:
+						kunmap_ops, count) ?: ret;
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(clear_foreign_p2m_mapping);

[-- Attachment #3: xsa361-linux-2.patch --]
[-- Type: application/octet-stream, Size: 823 bytes --]

From: Jan Beulich <jbeulich@suse.com>
Subject: Xen/x86: also check kernel mapping in set_foreign_p2m_mapping()

We should not set up further state if either mapping failed; paying
attention to just the user mapping's status isn't enough.

Also use GNTST_okay instead of implying its value (zero).

This is part of XSA-361.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Cc: stable@vger.kernel.org
Reviewed-by: Juergen Gross <jgross@suse.com>

--- a/arch/x86/xen/p2m.c
+++ b/arch/x86/xen/p2m.c
@@ -712,7 +712,8 @@ int set_foreign_p2m_mapping(struct gntta
 		unsigned long mfn, pfn;
 
 		/* Do not add to override if the map failed. */
-		if (map_ops[i].status)
+		if (map_ops[i].status != GNTST_okay ||
+		    (kmap_ops && kmap_ops[i].status != GNTST_okay))
 			continue;
 
 		if (map_ops[i].flags & GNTMAP_contains_pte) {

[-- Attachment #4: xsa361-linux-3.patch --]
[-- Type: application/octet-stream, Size: 2641 bytes --]

From: Jan Beulich <jbeulich@suse.com>
Subject: Xen/gntdev: correct dev_bus_addr handling in gntdev_map_grant_pages()

We may not skip setting the field in the unmap structure when
GNTMAP_device_map is in use - such an unmap would fail to release the
respective resources (a page ref in the hypervisor). Otoh the field
doesn't need setting at all when GNTMAP_device_map is not in use.

To record the value for unmapping, we also better don't use our local
p2m: In particular after a subsequent change it may not have got updated
for all the batch elements. Instead it can simply be taken from the
respective map's results.

We can additionally avoid playing this game altogether for the kernel
part of the mappings in (x86) PV mode.

This is part of XSA-361.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Cc: stable@vger.kernel.org
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
v4: Split from subsequent patch.

--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -309,18 +309,25 @@ int gntdev_map_grant_pages(struct gntdev
 		 * to the kernel linear addresses of the struct pages.
 		 * These ptes are completely different from the user ptes dealt
 		 * with find_grant_ptes.
+		 * Note that GNTMAP_device_map isn't needed here: The
+		 * dev_bus_addr output field gets consumed only from ->map_ops,
+		 * and by not requesting it when mapping we also avoid needing
+		 * to mirror dev_bus_addr into ->unmap_ops (and holding an extra
+		 * reference to the page in the hypervisor).
 		 */
+		unsigned int flags = (map->flags & ~GNTMAP_device_map) |
+				     GNTMAP_host_map;
+
 		for (i = 0; i < map->count; i++) {
 			unsigned long address = (unsigned long)
 				pfn_to_kaddr(page_to_pfn(map->pages[i]));
 			BUG_ON(PageHighMem(map->pages[i]));
 
-			gnttab_set_map_op(&map->kmap_ops[i], address,
-				map->flags | GNTMAP_host_map,
+			gnttab_set_map_op(&map->kmap_ops[i], address, flags,
 				map->grants[i].ref,
 				map->grants[i].domid);
 			gnttab_set_unmap_op(&map->kunmap_ops[i], address,
-				map->flags | GNTMAP_host_map, -1);
+				flags, -1);
 		}
 	}
 
@@ -336,17 +343,12 @@ int gntdev_map_grant_pages(struct gntdev
 			continue;
 		}
 
+		if (map->flags & GNTMAP_device_map)
+			map->unmap_ops[i].dev_bus_addr = map->map_ops[i].dev_bus_addr;
+
 		map->unmap_ops[i].handle = map->map_ops[i].handle;
 		if (use_ptemod)
 			map->kunmap_ops[i].handle = map->kmap_ops[i].handle;
-#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
-		else if (map->dma_vaddr) {
-			unsigned long bfn;
-
-			bfn = pfn_to_bfn(page_to_pfn(map->pages[i]));
-			map->unmap_ops[i].dev_bus_addr = __pfn_to_phys(bfn);
-		}
-#endif
 	}
 	return err;
 }

[-- Attachment #5: xsa361-linux-4.patch --]
[-- Type: application/octet-stream, Size: 2252 bytes --]

From: Jan Beulich <jbeulich@suse.com>
Subject: Xen/gntdev: correct error checking in gntdev_map_grant_pages()

Failure of the kernel part of the mapping operation should also be
indicated as an error to the caller, or else it may assume the
respective kernel VA is okay to access.

Furthermore gnttab_map_refs() failing still requires recording
successfully mapped handles, so they can be unmapped subsequently. This
in turn requires there to be a way to tell full hypercall failure from
partial success - preset map_op status fields such that they won't
"happen" to look as if the operation succeeded.

Also again use GNTST_okay instead of implying its value (zero).

This is part of XSA-361.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Cc: stable@vger.kernel.org
Reviewed-by: Juergen Gross <jgross@suse.com>
---
v4: Split out the v3 changes and re-base over the resulting earlier
    patch.
v3: Also fix GNTMAP_device_map / ->dev_bus_addr handling.
v2: Drop an inapplicable part of the description. Use 1 instead of
    __LINE__.

--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -334,21 +334,22 @@ int gntdev_map_grant_pages(struct gntdev
 	pr_debug("map %d+%d\n", map->index, map->count);
 	err = gnttab_map_refs(map->map_ops, use_ptemod ? map->kmap_ops : NULL,
 			map->pages, map->count);
-	if (err)
-		return err;
 
 	for (i = 0; i < map->count; i++) {
-		if (map->map_ops[i].status) {
+		if (map->map_ops[i].status == GNTST_okay)
+			map->unmap_ops[i].handle = map->map_ops[i].handle;
+		else if (!err)
 			err = -EINVAL;
-			continue;
-		}
 
 		if (map->flags & GNTMAP_device_map)
 			map->unmap_ops[i].dev_bus_addr = map->map_ops[i].dev_bus_addr;
 
-		map->unmap_ops[i].handle = map->map_ops[i].handle;
-		if (use_ptemod)
-			map->kunmap_ops[i].handle = map->kmap_ops[i].handle;
+		if (use_ptemod) {
+			if (map->kmap_ops[i].status == GNTST_okay)
+				map->kunmap_ops[i].handle = map->kmap_ops[i].handle;
+			else if (!err)
+				err = -EINVAL;
+		}
 	}
 	return err;
 }
--- a/include/xen/grant_table.h
+++ b/include/xen/grant_table.h
@@ -157,6 +157,7 @@ gnttab_set_map_op(struct gnttab_map_gran
 	map->flags = flags;
 	map->ref = ref;
 	map->dom = domid;
+	map->status = 1; /* arbitrary positive value */
 }
 
 static inline void

[-- Attachment #6: xsa361-linux-5.patch --]
[-- Type: application/octet-stream, Size: 1024 bytes --]

From: Stefano Stabellini <stefano.stabellini@xilinx.com>
Subject: xen/arm: don't ignore return errors from set_phys_to_machine

set_phys_to_machine can fail due to lack of memory, see the kzalloc call
in arch/arm/xen/p2m.c:__set_phys_to_machine_multi.

Don't ignore the potential return error in set_foreign_p2m_mapping,
returning it to the caller instead.

This is part of XSA-361.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Cc: stable@vger.kernel.org
Reviewed-by: Julien Grall <jgrall@amazon.com>

--- a/arch/arm/xen/p2m.c
+++ b/arch/arm/xen/p2m.c
@@ -95,8 +95,10 @@ int set_foreign_p2m_mapping(struct gnttab_map_grant_ref *map_ops,
 	for (i = 0; i < count; i++) {
 		if (map_ops[i].status)
 			continue;
-		set_phys_to_machine(map_ops[i].host_addr >> XEN_PAGE_SHIFT,
-				    map_ops[i].dev_bus_addr >> XEN_PAGE_SHIFT);
+		if (unlikely(!set_phys_to_machine(map_ops[i].host_addr >> XEN_PAGE_SHIFT,
+				    map_ops[i].dev_bus_addr >> XEN_PAGE_SHIFT))) {
+			return -ENOMEM;
+		}
 	}
 
 	return 0;