--- /dev/null
+From f28347cc66395e96712f5c2db0a302ee75bafce6 Mon Sep 17 00:00:00 2001
+From: Jan Beulich <jbeulich@suse.com>
+Date: Fri, 17 Sep 2021 08:13:08 +0200
+Subject: Xen/gntdev: don't ignore kernel unmapping error
+
+From: Jan Beulich <jbeulich@suse.com>
+
+commit f28347cc66395e96712f5c2db0a302ee75bafce6 upstream.
+
+While working on XSA-361 and its follow-ups, I failed to spot another
+place where the kernel mapping part of an operation was not treated the
+same as the user space part. Detect and propagate errors and add a 2nd
+pr_debug().
+
+Signed-off-by: Jan Beulich <jbeulich@suse.com>
+Reviewed-by: Juergen Gross <jgross@suse.com>
+Link: https://lore.kernel.org/r/c2513395-74dc-aea3-9192-fd265aa44e35@suse.com
+Signed-off-by: Juergen Gross <jgross@suse.com>
+Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
+Co-authored-by: Demi Marie Obenour <demi@invisiblethingslab.com>
+Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
+---
+ drivers/xen/gntdev.c | 8 ++++++++
+ 1 file changed, 8 insertions(+)
+
+--- a/drivers/xen/gntdev.c
++++ b/drivers/xen/gntdev.c
+@@ -399,6 +399,14 @@ static void __unmap_grant_pages_done(int
+ map->unmap_ops[offset+i].handle,
+ map->unmap_ops[offset+i].status);
+ map->unmap_ops[offset+i].handle = -1;
++ if (use_ptemod) {
++ WARN_ON(map->kunmap_ops[offset+i].status &&
++ map->kunmap_ops[offset+i].handle != -1);
++ pr_debug("kunmap handle=%u st=%d\n",
++ map->kunmap_ops[offset+i].handle,
++ map->kunmap_ops[offset+i].status);
++ map->kunmap_ops[offset+i].handle = -1;
++ }
+ }
+ /*
+ * Decrease the live-grant counter. This must happen after the loop to
--- /dev/null
+From 0991028cd49567d7016d1b224fe0117c35059f86 Mon Sep 17 00:00:00 2001
+From: "M. Vefa Bicakci" <m.v.b@runbox.com>
+Date: Sun, 2 Oct 2022 18:20:05 -0400
+Subject: xen/gntdev: Prevent leaking grants
+
+From: M. Vefa Bicakci <m.v.b@runbox.com>
+
+commit 0991028cd49567d7016d1b224fe0117c35059f86 upstream.
+
+Prior to this commit, if a grant mapping operation failed partially,
+some of the entries in the map_ops array would be invalid, whereas all
+of the entries in the kmap_ops array would be valid. This in turn would
+cause the following logic in gntdev_map_grant_pages to become invalid:
+
+ for (i = 0; i < map->count; i++) {
+ if (map->map_ops[i].status == GNTST_okay) {
+ map->unmap_ops[i].handle = map->map_ops[i].handle;
+ if (!use_ptemod)
+ alloced++;
+ }
+ if (use_ptemod) {
+ if (map->kmap_ops[i].status == GNTST_okay) {
+ if (map->map_ops[i].status == GNTST_okay)
+ alloced++;
+ map->kunmap_ops[i].handle = map->kmap_ops[i].handle;
+ }
+ }
+ }
+ ...
+ atomic_add(alloced, &map->live_grants);
+
+Assume that use_ptemod is true (i.e., the domain mapping the granted
+pages is a paravirtualized domain). In the code excerpt above, note that
+the "alloced" variable is only incremented when both kmap_ops[i].status
+and map_ops[i].status are set to GNTST_okay (i.e., both mapping
+operations are successful). However, as also noted above, there are
+cases where a grant mapping operation fails partially, breaking the
+assumption of the code excerpt above.
+
+The aforementioned causes map->live_grants to be incorrectly set. In
+some cases, all of the map_ops mappings fail, but all of the kmap_ops
+mappings succeed, meaning that live_grants may remain zero. This in turn
+makes it impossible to unmap the successfully grant-mapped pages pointed
+to by kmap_ops, because unmap_grant_pages has the following snippet of
+code at its beginning:
+
+ if (atomic_read(&map->live_grants) == 0)
+ return; /* Nothing to do */
+
+In other cases where only some of the map_ops mappings fail but all
+kmap_ops mappings succeed, live_grants is made positive, but when the
+user requests unmapping the grant-mapped pages, __unmap_grant_pages_done
+will then make map->live_grants negative, because the latter function
+does not check if all of the pages that were requested to be unmapped
+were actually unmapped, and the same function unconditionally subtracts
+"data->count" (i.e., a value that can be greater than map->live_grants)
+from map->live_grants. The side effects of a negative live_grants value
+have not been studied.
+
+The net effect of all of this is that grant references are leaked in one
+of the above conditions. In Qubes OS v4.1 (which uses Xen's grant
+mechanism extensively for X11 GUI isolation), this issue manifests
+itself with warning messages like the following to be printed out by the
+Linux kernel in the VM that had granted pages (that contain X11 GUI
+window data) to dom0: "g.e. 0x1234 still pending", especially after the
+user rapidly resizes GUI VM windows (causing some grant-mapping
+operations to partially or completely fail, due to the fact that the VM
+unshares some of the pages as part of the window resizing, making the
+pages impossible to grant-map from dom0).
+
+The fix for this issue involves counting all successful map_ops and
+kmap_ops mappings separately, and then adding the sum to live_grants.
+During unmapping, only the number of successfully unmapped grants is
+subtracted from live_grants. The code is also modified to check for
+negative live_grants values after the subtraction and warn the user.
+
+Link: https://github.com/QubesOS/qubes-issues/issues/7631
+Fixes: dbe97cff7dd9 ("xen/gntdev: Avoid blocking in unmap_grant_pages()")
+Cc: stable@vger.kernel.org
+Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com>
+Acked-by: Demi Marie Obenour <demi@invisiblethingslab.com>
+Reviewed-by: Juergen Gross <jgross@suse.com>
+Link: https://lore.kernel.org/r/20221002222006.2077-2-m.v.b@runbox.com
+Signed-off-by: Juergen Gross <jgross@suse.com>
+Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
+Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
+---
+ drivers/xen/gntdev.c | 22 +++++++++++++++++-----
+ 1 file changed, 17 insertions(+), 5 deletions(-)
+
+--- a/drivers/xen/gntdev.c
++++ b/drivers/xen/gntdev.c
+@@ -364,8 +364,7 @@ static int map_grant_pages(struct grant_
+ for (i = 0; i < map->count; i++) {
+ if (map->map_ops[i].status == GNTST_okay) {
+ map->unmap_ops[i].handle = map->map_ops[i].handle;
+- if (!use_ptemod)
+- alloced++;
++ alloced++;
+ } else if (!err)
+ err = -EINVAL;
+
+@@ -374,8 +373,7 @@ static int map_grant_pages(struct grant_
+
+ if (use_ptemod) {
+ if (map->kmap_ops[i].status == GNTST_okay) {
+- if (map->map_ops[i].status == GNTST_okay)
+- alloced++;
++ alloced++;
+ map->kunmap_ops[i].handle = map->kmap_ops[i].handle;
+ } else if (!err)
+ err = -EINVAL;
+@@ -391,8 +389,14 @@ static void __unmap_grant_pages_done(int
+ unsigned int i;
+ struct grant_map *map = data->data;
+ unsigned int offset = data->unmap_ops - map->unmap_ops;
++ int successful_unmaps = 0;
++ int live_grants;
+
+ for (i = 0; i < data->count; i++) {
++ if (map->unmap_ops[offset + i].status == GNTST_okay &&
++ map->unmap_ops[offset + i].handle != -1)
++ successful_unmaps++;
++
+ WARN_ON(map->unmap_ops[offset+i].status &&
+ map->unmap_ops[offset+i].handle != -1);
+ pr_debug("unmap handle=%d st=%d\n",
+@@ -400,6 +404,10 @@ static void __unmap_grant_pages_done(int
+ map->unmap_ops[offset+i].status);
+ map->unmap_ops[offset+i].handle = -1;
+ if (use_ptemod) {
++ if (map->kunmap_ops[offset + i].status == GNTST_okay &&
++ map->kunmap_ops[offset + i].handle != -1)
++ successful_unmaps++;
++
+ WARN_ON(map->kunmap_ops[offset+i].status &&
+ map->kunmap_ops[offset+i].handle != -1);
+ pr_debug("kunmap handle=%u st=%d\n",
+@@ -408,11 +416,15 @@ static void __unmap_grant_pages_done(int
+ map->kunmap_ops[offset+i].handle = -1;
+ }
+ }
++
+ /*
+ * Decrease the live-grant counter. This must happen after the loop to
+ * prevent premature reuse of the grants by gnttab_mmap().
+ */
+- atomic_sub(data->count, &map->live_grants);
++ live_grants = atomic_sub_return(successful_unmaps, &map->live_grants);
++ if (WARN_ON(live_grants < 0))
++ pr_err("%s: live_grants became negative (%d) after unmapping %d pages!\n",
++ __func__, live_grants, successful_unmaps);
+
+ /* Release reference taken by unmap_grant_pages */
+ gntdev_put_map(NULL, map);
projects / thirdparty / kernel / stable-queue.git / commitdiff
