From: Greg Kroah-Hartman Date: Mon, 31 Oct 2022 07:25:31 +0000 (+0100) Subject: 4.14-stable patches X-Git-Tag: v4.19.263~35 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=f9d9014eaf5db4ebc5a1d607f351311ad257e7fb;p=thirdparty%2Fkernel%2Fstable-queue.git 4.14-stable patches added patches: xen-gntdev-don-t-ignore-kernel-unmapping-error.patch xen-gntdev-prevent-leaking-grants.patch --- diff --git a/queue-4.14/series b/queue-4.14/series index d742b1b7b2d..5499ed6c1b4 100644 --- a/queue-4.14/series +++ b/queue-4.14/series @@ -29,3 +29,5 @@ drm-msm-hdmi-fix-memory-corruption-with-too-many-bridges.patch mmc-core-fix-kernel-panic-when-remove-non-standard-sdio-card.patch kernfs-fix-use-after-free-in-__kernfs_remove.patch s390-futex-add-missing-ex_table-entry-to-__futex_atomic_op.patch +xen-gntdev-don-t-ignore-kernel-unmapping-error.patch +xen-gntdev-prevent-leaking-grants.patch diff --git a/queue-4.14/xen-gntdev-don-t-ignore-kernel-unmapping-error.patch b/queue-4.14/xen-gntdev-don-t-ignore-kernel-unmapping-error.patch new file mode 100644 index 00000000000..233acd38f2c --- /dev/null +++ b/queue-4.14/xen-gntdev-don-t-ignore-kernel-unmapping-error.patch @@ -0,0 +1,42 @@ +From f28347cc66395e96712f5c2db0a302ee75bafce6 Mon Sep 17 00:00:00 2001 +From: Jan Beulich +Date: Fri, 17 Sep 2021 08:13:08 +0200 +Subject: Xen/gntdev: don't ignore kernel unmapping error + +From: Jan Beulich + +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 +Reviewed-by: Juergen Gross +Link: https://lore.kernel.org/r/c2513395-74dc-aea3-9192-fd265aa44e35@suse.com +Signed-off-by: Juergen Gross +Signed-off-by: Demi Marie Obenour +Co-authored-by: Demi Marie Obenour +Signed-off-by: Greg Kroah-Hartman +--- + 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 diff --git a/queue-4.14/xen-gntdev-prevent-leaking-grants.patch b/queue-4.14/xen-gntdev-prevent-leaking-grants.patch new file mode 100644 index 00000000000..56d87215246 --- /dev/null +++ b/queue-4.14/xen-gntdev-prevent-leaking-grants.patch @@ -0,0 +1,155 @@ +From 0991028cd49567d7016d1b224fe0117c35059f86 Mon Sep 17 00:00:00 2001 +From: "M. Vefa Bicakci" +Date: Sun, 2 Oct 2022 18:20:05 -0400 +Subject: xen/gntdev: Prevent leaking grants + +From: M. Vefa Bicakci + +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 +Acked-by: Demi Marie Obenour +Reviewed-by: Juergen Gross +Link: https://lore.kernel.org/r/20221002222006.2077-2-m.v.b@runbox.com +Signed-off-by: Juergen Gross +Signed-off-by: Demi Marie Obenour +Signed-off-by: Greg Kroah-Hartman +--- + 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);