* [PATCH 0/2] xen/gntdev: Fixes for leaks and VMA splitting @ 2022-09-12 4:00 M. Vefa Bicakci 2022-09-12 4:00 ` [PATCH 1/2] xen/gntdev: Prevent leaking grants M. Vefa Bicakci 2022-09-12 4:00 ` [PATCH 2/2] xen/gntdev: Accommodate VMA splitting M. Vefa Bicakci 0 siblings, 2 replies; 6+ messages in thread From: M. Vefa Bicakci @ 2022-09-12 4:00 UTC (permalink / raw) To: xen-devel, linux-kernel Cc: m.v.b, Juergen Gross, Stefano Stabellini, Oleksandr Tyshchenko, Demi Marie Obenour, Gerd Hoffmann Hi all, The changes in this patch series intend to fix the Xen grant device driver, so that grant mapping leaks caused by partially failed grant mapping operations are avoided with the first patch, and so that the splitting of VMAs does not result in incorrectly unmapped grant pages with the second patch. The second patch also prevents a similar issue in a double-mapping scenario, where mmap() is used with MAP_FIXED to map grants over an existing mapping created with the same grants, and where grant pages are unmapped incorrectly as well. These commits were tested on top of Linux v5.15.67, but I have also verified that they compile fine on top of the base commit mentioned at the bottom of this cover letter. The base commit in question is tagged as "next-20220909". Thank you, Vefa M. Vefa Bicakci (2): xen/gntdev: Prevent leaking grants xen/gntdev: Accommodate VMA splitting drivers/xen/gntdev-common.h | 3 +- drivers/xen/gntdev.c | 90 +++++++++++++++++++++---------------- 2 files changed, 54 insertions(+), 39 deletions(-) base-commit: 9a82ccda91ed2b40619cb3c10d446ae1f97bab6e -- 2.37.3 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] xen/gntdev: Prevent leaking grants 2022-09-12 4:00 [PATCH 0/2] xen/gntdev: Fixes for leaks and VMA splitting M. Vefa Bicakci @ 2022-09-12 4:00 ` M. Vefa Bicakci 2022-09-19 9:52 ` Juergen Gross 2022-09-12 4:00 ` [PATCH 2/2] xen/gntdev: Accommodate VMA splitting M. Vefa Bicakci 1 sibling, 1 reply; 6+ messages in thread From: M. Vefa Bicakci @ 2022-09-12 4:00 UTC (permalink / raw) To: xen-devel, linux-kernel Cc: m.v.b, Juergen Gross, Stefano Stabellini, Oleksandr Tyshchenko, Demi Marie Obenour, Gerd Hoffmann 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. To determine which grants were successfully unmapped, their status fields are set to an arbitrary positive number (1), as was done in commit ebee0eab0859 ("Xen/gntdev: correct error checking in gntdev_map_grant_pages()"). 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> --- drivers/xen/gntdev.c | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c index 84b143eef395..485fa9c630aa 100644 --- a/drivers/xen/gntdev.c +++ b/drivers/xen/gntdev.c @@ -367,8 +367,7 @@ int gntdev_map_grant_pages(struct gntdev_grant_map *map) 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; @@ -377,8 +376,7 @@ int gntdev_map_grant_pages(struct gntdev_grant_map *map) 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; @@ -394,8 +392,13 @@ static void __unmap_grant_pages_done(int result, unsigned int i; struct gntdev_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) + successful_unmaps++; + WARN_ON(map->unmap_ops[offset + i].status != GNTST_okay && map->unmap_ops[offset + i].handle != INVALID_GRANT_HANDLE); pr_debug("unmap handle=%d st=%d\n", @@ -403,6 +406,9 @@ static void __unmap_grant_pages_done(int result, map->unmap_ops[offset+i].status); map->unmap_ops[offset+i].handle = INVALID_GRANT_HANDLE; if (use_ptemod) { + if (map->kunmap_ops[offset + i].status == GNTST_okay) + successful_unmaps++; + WARN_ON(map->kunmap_ops[offset + i].status != GNTST_okay && map->kunmap_ops[offset + i].handle != INVALID_GRANT_HANDLE); pr_debug("kunmap handle=%u st=%d\n", @@ -411,11 +417,15 @@ static void __unmap_grant_pages_done(int result, map->kunmap_ops[offset+i].handle = INVALID_GRANT_HANDLE; } } + /* * 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); @@ -424,6 +434,8 @@ static void __unmap_grant_pages_done(int result, static void __unmap_grant_pages(struct gntdev_grant_map *map, int offset, int pages) { + int idx; + if (map->notify.flags & UNMAP_NOTIFY_CLEAR_BYTE) { int pgno = (map->notify.addr >> PAGE_SHIFT); @@ -436,6 +448,16 @@ static void __unmap_grant_pages(struct gntdev_grant_map *map, int offset, } } + /* Set all unmap/kunmap status fields to an arbitrary positive value, + * so that it is possible to determine which grants were successfully + * unmapped by inspecting the status fields. + */ + for (idx = offset; idx < offset + pages; idx++) { + map->unmap_ops[idx].status = 1; + if (use_ptemod) + map->kunmap_ops[idx].status = 1; + } + map->unmap_data.unmap_ops = map->unmap_ops + offset; map->unmap_data.kunmap_ops = use_ptemod ? map->kunmap_ops + offset : NULL; map->unmap_data.pages = map->pages + offset; -- 2.37.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] xen/gntdev: Prevent leaking grants 2022-09-12 4:00 ` [PATCH 1/2] xen/gntdev: Prevent leaking grants M. Vefa Bicakci @ 2022-09-19 9:52 ` Juergen Gross 2022-09-23 1:48 ` M. Vefa Bicakci 0 siblings, 1 reply; 6+ messages in thread From: Juergen Gross @ 2022-09-19 9:52 UTC (permalink / raw) To: M. Vefa Bicakci, xen-devel, linux-kernel Cc: Stefano Stabellini, Oleksandr Tyshchenko, Demi Marie Obenour, Gerd Hoffmann [-- Attachment #1.1.1: Type: text/plain, Size: 7902 bytes --] On 12.09.22 06:00, M. Vefa Bicakci wrote: > 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. To determine which grants were successfully > unmapped, their status fields are set to an arbitrary positive number > (1), as was done in commit ebee0eab0859 ("Xen/gntdev: correct error > checking in gntdev_map_grant_pages()"). 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> > --- > drivers/xen/gntdev.c | 32 +++++++++++++++++++++++++++----- > 1 file changed, 27 insertions(+), 5 deletions(-) > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > index 84b143eef395..485fa9c630aa 100644 > --- a/drivers/xen/gntdev.c > +++ b/drivers/xen/gntdev.c > @@ -367,8 +367,7 @@ int gntdev_map_grant_pages(struct gntdev_grant_map *map) > 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; > > @@ -377,8 +376,7 @@ int gntdev_map_grant_pages(struct gntdev_grant_map *map) > > 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; > @@ -394,8 +392,13 @@ static void __unmap_grant_pages_done(int result, > unsigned int i; > struct gntdev_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) > + successful_unmaps++; Shouldn't this test include "&& handle != INVALID_GRANT_HANDLE" ? This should enable you to drop setting status to 1 below. > + > WARN_ON(map->unmap_ops[offset + i].status != GNTST_okay && > map->unmap_ops[offset + i].handle != INVALID_GRANT_HANDLE); > pr_debug("unmap handle=%d st=%d\n", > @@ -403,6 +406,9 @@ static void __unmap_grant_pages_done(int result, > map->unmap_ops[offset+i].status); > map->unmap_ops[offset+i].handle = INVALID_GRANT_HANDLE; > if (use_ptemod) { > + if (map->kunmap_ops[offset + i].status == GNTST_okay) > + successful_unmaps++; > + > WARN_ON(map->kunmap_ops[offset + i].status != GNTST_okay && > map->kunmap_ops[offset + i].handle != INVALID_GRANT_HANDLE); > pr_debug("kunmap handle=%u st=%d\n", > @@ -411,11 +417,15 @@ static void __unmap_grant_pages_done(int result, > map->kunmap_ops[offset+i].handle = INVALID_GRANT_HANDLE; > } > } > + > /* > * 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); > @@ -424,6 +434,8 @@ static void __unmap_grant_pages_done(int result, > static void __unmap_grant_pages(struct gntdev_grant_map *map, int offset, > int pages) > { > + int idx; > + > if (map->notify.flags & UNMAP_NOTIFY_CLEAR_BYTE) { > int pgno = (map->notify.addr >> PAGE_SHIFT); > > @@ -436,6 +448,16 @@ static void __unmap_grant_pages(struct gntdev_grant_map *map, int offset, > } > } > > + /* Set all unmap/kunmap status fields to an arbitrary positive value, > + * so that it is possible to determine which grants were successfully > + * unmapped by inspecting the status fields. > + */ > + for (idx = offset; idx < offset + pages; idx++) { > + map->unmap_ops[idx].status = 1; > + if (use_ptemod) > + map->kunmap_ops[idx].status = 1; > + } > + > map->unmap_data.unmap_ops = map->unmap_ops + offset; > map->unmap_data.kunmap_ops = use_ptemod ? map->kunmap_ops + offset : NULL; > map->unmap_data.pages = map->pages + offset; Juergen [-- Attachment #1.1.2: OpenPGP public key --] [-- Type: application/pgp-keys, Size: 3149 bytes --] [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 495 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] xen/gntdev: Prevent leaking grants 2022-09-19 9:52 ` Juergen Gross @ 2022-09-23 1:48 ` M. Vefa Bicakci 0 siblings, 0 replies; 6+ messages in thread From: M. Vefa Bicakci @ 2022-09-23 1:48 UTC (permalink / raw) To: Juergen Gross, xen-devel, linux-kernel Cc: Stefano Stabellini, Oleksandr Tyshchenko, Demi Marie Obenour, Gerd Hoffmann On 2022-09-19 05:52, Juergen Gross wrote: > On 12.09.22 06:00, M. Vefa Bicakci wrote: >> 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. To determine which grants were successfully >> unmapped, their status fields are set to an arbitrary positive number >> (1), as was done in commit ebee0eab0859 ("Xen/gntdev: correct error >> checking in gntdev_map_grant_pages()"). 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> >> --- >> drivers/xen/gntdev.c | 32 +++++++++++++++++++++++++++----- >> 1 file changed, 27 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c >> index 84b143eef395..485fa9c630aa 100644 >> --- a/drivers/xen/gntdev.c >> +++ b/drivers/xen/gntdev.c >> @@ -367,8 +367,7 @@ int gntdev_map_grant_pages(struct gntdev_grant_map *map) >> 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; >> @@ -377,8 +376,7 @@ int gntdev_map_grant_pages(struct gntdev_grant_map *map) >> 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; >> @@ -394,8 +392,13 @@ static void __unmap_grant_pages_done(int result, >> unsigned int i; >> struct gntdev_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) >> + successful_unmaps++; > Hi, Sorry for the delay, and thank you for reviewing my patches! > Shouldn't this test include "&& handle != INVALID_GRANT_HANDLE" ? > > This should enable you to drop setting status to 1 below. I had not thought of the approach you suggested. Just now, I applied your suggestion to my local kernel tree, and I am building a new kernel for testing now. I hope to publish a newer version of this patch set over the weekend. Thanks again, Vefa > >> + >> WARN_ON(map->unmap_ops[offset + i].status != GNTST_okay && >> map->unmap_ops[offset + i].handle != INVALID_GRANT_HANDLE); >> pr_debug("unmap handle=%d st=%d\n", >> @@ -403,6 +406,9 @@ static void __unmap_grant_pages_done(int result, >> map->unmap_ops[offset+i].status); >> map->unmap_ops[offset+i].handle = INVALID_GRANT_HANDLE; >> if (use_ptemod) { >> + if (map->kunmap_ops[offset + i].status == GNTST_okay) >> + successful_unmaps++; >> + >> WARN_ON(map->kunmap_ops[offset + i].status != GNTST_okay && >> map->kunmap_ops[offset + i].handle != INVALID_GRANT_HANDLE); >> pr_debug("kunmap handle=%u st=%d\n", >> @@ -411,11 +417,15 @@ static void __unmap_grant_pages_done(int result, >> map->kunmap_ops[offset+i].handle = INVALID_GRANT_HANDLE; >> } >> } >> + >> /* >> * 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); >> @@ -424,6 +434,8 @@ static void __unmap_grant_pages_done(int result, >> static void __unmap_grant_pages(struct gntdev_grant_map *map, int offset, >> int pages) >> { >> + int idx; >> + >> if (map->notify.flags & UNMAP_NOTIFY_CLEAR_BYTE) { >> int pgno = (map->notify.addr >> PAGE_SHIFT); >> @@ -436,6 +448,16 @@ static void __unmap_grant_pages(struct gntdev_grant_map *map, int offset, >> } >> } >> + /* Set all unmap/kunmap status fields to an arbitrary positive value, >> + * so that it is possible to determine which grants were successfully >> + * unmapped by inspecting the status fields. >> + */ >> + for (idx = offset; idx < offset + pages; idx++) { >> + map->unmap_ops[idx].status = 1; >> + if (use_ptemod) >> + map->kunmap_ops[idx].status = 1; >> + } >> + >> map->unmap_data.unmap_ops = map->unmap_ops + offset; >> map->unmap_data.kunmap_ops = use_ptemod ? map->kunmap_ops + offset : NULL; >> map->unmap_data.pages = map->pages + offset; > > Juergen ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] xen/gntdev: Accommodate VMA splitting 2022-09-12 4:00 [PATCH 0/2] xen/gntdev: Fixes for leaks and VMA splitting M. Vefa Bicakci 2022-09-12 4:00 ` [PATCH 1/2] xen/gntdev: Prevent leaking grants M. Vefa Bicakci @ 2022-09-12 4:00 ` M. Vefa Bicakci 2022-09-19 10:32 ` Juergen Gross 1 sibling, 1 reply; 6+ messages in thread From: M. Vefa Bicakci @ 2022-09-12 4:00 UTC (permalink / raw) To: xen-devel, linux-kernel Cc: m.v.b, Juergen Gross, Stefano Stabellini, Oleksandr Tyshchenko, Demi Marie Obenour, Gerd Hoffmann Prior to this commit, the gntdev driver code did not handle the following scenario correctly: * User process sets up a gntdev mapping composed of two grant mappings (i.e., two pages shared by another Xen domain). * User process munmap()s one of the pages. * User process munmap()s the remaining page. * User process exits. In the scenario above, the user process would cause the kernel to log the following messages in dmesg for the first munmap(), and the second munmap() call would result in similar log messages: BUG: Bad page map in process doublemap.test pte:... pmd:... page:0000000057c97bff refcount:1 mapcount:-1 \ mapping:0000000000000000 index:0x0 pfn:... ... page dumped because: bad pte ... file:gntdev fault:0x0 mmap:gntdev_mmap [xen_gntdev] readpage:0x0 ... Call Trace: <TASK> dump_stack_lvl+0x46/0x5e print_bad_pte.cold+0x66/0xb6 unmap_page_range+0x7e5/0xdc0 unmap_vmas+0x78/0xf0 unmap_region+0xa8/0x110 __do_munmap+0x1ea/0x4e0 __vm_munmap+0x75/0x120 __x64_sys_munmap+0x28/0x40 do_syscall_64+0x38/0x90 entry_SYSCALL_64_after_hwframe+0x61/0xcb ... For each munmap() call, the Xen hypervisor (if built with CONFIG_DEBUG) would print out the following and trigger a general protection fault in dom0: (XEN) d0v... Attempt to implicitly unmap d0's grant PTE ... (XEN) d0v... Attempt to implicitly unmap d0's grant PTE ... As of this writing, gntdev_grant_map structure's vma field (referred to as map->vma below) is mainly used for checking the start and end addresses of mappings. However, with split VMAs, these may change, and there could be more than one VMA associated with a gntdev mapping. Hence, remove the use of map->vma and rely on map->pages_vm_start for the original start address and on (map->count << PAGE_SHIFT) for the original mapping size. Let the invalidate() and find_special_page() hooks use these. Also, given that there can be multiple VMAs associated with a gntdev mapping, move the "mmu_interval_notifier_remove(&map->notifier)" call to the end of gntdev_put_map, so that the MMU notifier is only removed after the closing of the last remaining VMA. Finally, use an atomic to prevent inadvertent gntdev mapping re-use, instead of using the map->live_grants atomic counter and/or the map->vma pointer (the latter of which is now removed). This prevents the userspace from mmap()'ing (with MAP_FIXED) a gntdev mapping over the same address range as a previously set up gntdev mapping. This scenario can be summarized with the following call-trace, which was valid prior to this commit: mmap gntdev_mmap mmap (repeat mmap with MAP_FIXED over the same address range) gntdev_invalidate unmap_grant_pages (sets 'being_removed' entries to true) gnttab_unmap_refs_async unmap_single_vma gntdev_mmap (maps the shared pages again) munmap gntdev_invalidate unmap_grant_pages (no-op because 'being_removed' entries are true) unmap_single_vma (Xen reports that a granted page is being unmapped and triggers a general protection fault in dom0 if Xen was built with CONFIG_DEBUG) The fix for this last scenario could be worth its own commit, but we opted for a single commit, because removing the gntdev_grant_map structure's vma field requires guarding the entry to gntdev_mmap(), and the live_grants atomic counter is not sufficient on its own to prevent the mmap() over a pre-existing mapping. Link: https://github.com/QubesOS/qubes-issues/issues/7631 Fixes: ab31523c2fca ("xen/gntdev: allow usermode to map granted pages") Cc: stable@vger.kernel.org Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com> --- Note for reviewers: I am not 100% sure if the "Fixes" tag is correct. Based on a quick look at the history of the modified file, I am under the impression that VMA splits could be broken for the Xen gntdev driver since day 1 (i.e., v2.6.38), but I did not yet attempt to verify this by testing older kernels where the gntdev driver's code is sufficiently similar. Also, resetting the being_removed flags to false after the completion of unmap operation could be another potential solution (that I have not yet tested in the context of this change) to the mmap and MAP_FIXED issue discussed at the end of the patch description. --- drivers/xen/gntdev-common.h | 3 +- drivers/xen/gntdev.c | 58 ++++++++++++++++--------------------- 2 files changed, 27 insertions(+), 34 deletions(-) diff --git a/drivers/xen/gntdev-common.h b/drivers/xen/gntdev-common.h index 40ef379c28ab..9c286b2a1900 100644 --- a/drivers/xen/gntdev-common.h +++ b/drivers/xen/gntdev-common.h @@ -44,9 +44,10 @@ struct gntdev_unmap_notify { }; struct gntdev_grant_map { + atomic_t in_use; struct mmu_interval_notifier notifier; + bool notifier_init; struct list_head next; - struct vm_area_struct *vma; int index; int count; int flags; diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c index 485fa9c630aa..a3a0813dada3 100644 --- a/drivers/xen/gntdev.c +++ b/drivers/xen/gntdev.c @@ -286,6 +286,9 @@ void gntdev_put_map(struct gntdev_priv *priv, struct gntdev_grant_map *map) */ } + if (use_ptemod && map->notifier_init) + mmu_interval_notifier_remove(&map->notifier); + if (map->notify.flags & UNMAP_NOTIFY_SEND_EVENT) { notify_remote_via_evtchn(map->notify.event); evtchn_put(map->notify.event); @@ -298,7 +301,7 @@ void gntdev_put_map(struct gntdev_priv *priv, struct gntdev_grant_map *map) static int find_grant_ptes(pte_t *pte, unsigned long addr, void *data) { struct gntdev_grant_map *map = data; - unsigned int pgnr = (addr - map->vma->vm_start) >> PAGE_SHIFT; + unsigned int pgnr = (addr - map->pages_vm_start) >> PAGE_SHIFT; int flags = map->flags | GNTMAP_application_map | GNTMAP_contains_pte | (1 << _GNTMAP_guest_avail0); u64 pte_maddr; @@ -518,11 +521,7 @@ static void gntdev_vma_close(struct vm_area_struct *vma) struct gntdev_priv *priv = file->private_data; pr_debug("gntdev_vma_close %p\n", vma); - if (use_ptemod) { - WARN_ON(map->vma != vma); - mmu_interval_notifier_remove(&map->notifier); - map->vma = NULL; - } + vma->vm_private_data = NULL; gntdev_put_map(priv, map); } @@ -550,29 +549,30 @@ static bool gntdev_invalidate(struct mmu_interval_notifier *mn, struct gntdev_grant_map *map = container_of(mn, struct gntdev_grant_map, notifier); unsigned long mstart, mend; + unsigned long map_start, map_end; if (!mmu_notifier_range_blockable(range)) return false; + map_start = map->pages_vm_start; + map_end = map->pages_vm_start + (map->count << PAGE_SHIFT); + /* * If the VMA is split or otherwise changed the notifier is not * updated, but we don't want to process VA's outside the modified * VMA. FIXME: It would be much more understandable to just prevent * modifying the VMA in the first place. */ - if (map->vma->vm_start >= range->end || - map->vma->vm_end <= range->start) + if (map_start >= range->end || map_end <= range->start) return true; - mstart = max(range->start, map->vma->vm_start); - mend = min(range->end, map->vma->vm_end); + mstart = max(range->start, map_start); + mend = min(range->end, map_end); pr_debug("map %d+%d (%lx %lx), range %lx %lx, mrange %lx %lx\n", - map->index, map->count, - map->vma->vm_start, map->vma->vm_end, - range->start, range->end, mstart, mend); - unmap_grant_pages(map, - (mstart - map->vma->vm_start) >> PAGE_SHIFT, - (mend - mstart) >> PAGE_SHIFT); + map->index, map->count, map_start, map_end, + range->start, range->end, mstart, mend); + unmap_grant_pages(map, (mstart - map_start) >> PAGE_SHIFT, + (mend - mstart) >> PAGE_SHIFT); return true; } @@ -1052,18 +1052,15 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) return -EINVAL; pr_debug("map %d+%d at %lx (pgoff %lx)\n", - index, count, vma->vm_start, vma->vm_pgoff); + index, count, vma->vm_start, vma->vm_pgoff); mutex_lock(&priv->lock); map = gntdev_find_map_index(priv, index, count); if (!map) goto unlock_out; - if (use_ptemod && map->vma) - goto unlock_out; - if (atomic_read(&map->live_grants)) { - err = -EAGAIN; + if (!atomic_add_unless(&map->in_use, 1, 1)) goto unlock_out; - } + refcount_inc(&map->users); vma->vm_ops = &gntdev_vmops; @@ -1084,15 +1081,16 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) map->flags |= GNTMAP_readonly; } + map->pages_vm_start = vma->vm_start; + if (use_ptemod) { - map->vma = vma; err = mmu_interval_notifier_insert_locked( &map->notifier, vma->vm_mm, vma->vm_start, vma->vm_end - vma->vm_start, &gntdev_mmu_ops); - if (err) { - map->vma = NULL; + if (err) goto out_unlock_put; - } + + map->notifier_init = true; } mutex_unlock(&priv->lock); @@ -1109,7 +1107,6 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) */ mmu_interval_read_begin(&map->notifier); - map->pages_vm_start = vma->vm_start; err = apply_to_page_range(vma->vm_mm, vma->vm_start, vma->vm_end - vma->vm_start, find_grant_ptes, map); @@ -1138,13 +1135,8 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) out_unlock_put: mutex_unlock(&priv->lock); out_put_map: - if (use_ptemod) { + if (use_ptemod) unmap_grant_pages(map, 0, map->count); - if (map->vma) { - mmu_interval_notifier_remove(&map->notifier); - map->vma = NULL; - } - } gntdev_put_map(priv, map); return err; } -- 2.37.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] xen/gntdev: Accommodate VMA splitting 2022-09-12 4:00 ` [PATCH 2/2] xen/gntdev: Accommodate VMA splitting M. Vefa Bicakci @ 2022-09-19 10:32 ` Juergen Gross 0 siblings, 0 replies; 6+ messages in thread From: Juergen Gross @ 2022-09-19 10:32 UTC (permalink / raw) To: M. Vefa Bicakci, xen-devel, linux-kernel Cc: Stefano Stabellini, Oleksandr Tyshchenko, Demi Marie Obenour, Gerd Hoffmann [-- Attachment #1.1.1: Type: text/plain, Size: 4084 bytes --] On 12.09.22 06:00, M. Vefa Bicakci wrote: > Prior to this commit, the gntdev driver code did not handle the > following scenario correctly: > > * User process sets up a gntdev mapping composed of two grant mappings > (i.e., two pages shared by another Xen domain). > * User process munmap()s one of the pages. > * User process munmap()s the remaining page. > * User process exits. > > In the scenario above, the user process would cause the kernel to log > the following messages in dmesg for the first munmap(), and the second > munmap() call would result in similar log messages: > > BUG: Bad page map in process doublemap.test pte:... pmd:... > page:0000000057c97bff refcount:1 mapcount:-1 \ > mapping:0000000000000000 index:0x0 pfn:... > ... > page dumped because: bad pte > ... > file:gntdev fault:0x0 mmap:gntdev_mmap [xen_gntdev] readpage:0x0 > ... > Call Trace: > <TASK> > dump_stack_lvl+0x46/0x5e > print_bad_pte.cold+0x66/0xb6 > unmap_page_range+0x7e5/0xdc0 > unmap_vmas+0x78/0xf0 > unmap_region+0xa8/0x110 > __do_munmap+0x1ea/0x4e0 > __vm_munmap+0x75/0x120 > __x64_sys_munmap+0x28/0x40 > do_syscall_64+0x38/0x90 > entry_SYSCALL_64_after_hwframe+0x61/0xcb > ... > > For each munmap() call, the Xen hypervisor (if built with CONFIG_DEBUG) > would print out the following and trigger a general protection fault in > dom0: > > (XEN) d0v... Attempt to implicitly unmap d0's grant PTE ... > (XEN) d0v... Attempt to implicitly unmap d0's grant PTE ... > > As of this writing, gntdev_grant_map structure's vma field (referred to > as map->vma below) is mainly used for checking the start and end > addresses of mappings. However, with split VMAs, these may change, and > there could be more than one VMA associated with a gntdev mapping. > Hence, remove the use of map->vma and rely on map->pages_vm_start for > the original start address and on (map->count << PAGE_SHIFT) for the > original mapping size. Let the invalidate() and find_special_page() > hooks use these. > > Also, given that there can be multiple VMAs associated with a gntdev > mapping, move the "mmu_interval_notifier_remove(&map->notifier)" call to > the end of gntdev_put_map, so that the MMU notifier is only removed > after the closing of the last remaining VMA. > > Finally, use an atomic to prevent inadvertent gntdev mapping re-use, > instead of using the map->live_grants atomic counter and/or the map->vma > pointer (the latter of which is now removed). This prevents the > userspace from mmap()'ing (with MAP_FIXED) a gntdev mapping over the > same address range as a previously set up gntdev mapping. This scenario > can be summarized with the following call-trace, which was valid prior > to this commit: > > mmap > gntdev_mmap > mmap (repeat mmap with MAP_FIXED over the same address range) > gntdev_invalidate > unmap_grant_pages (sets 'being_removed' entries to true) > gnttab_unmap_refs_async > unmap_single_vma > gntdev_mmap (maps the shared pages again) > munmap > gntdev_invalidate > unmap_grant_pages > (no-op because 'being_removed' entries are true) > unmap_single_vma (Xen reports that a granted page is being > unmapped and triggers a general protection fault in dom0 > if Xen was built with CONFIG_DEBUG) > > The fix for this last scenario could be worth its own commit, but we > opted for a single commit, because removing the gntdev_grant_map > structure's vma field requires guarding the entry to gntdev_mmap(), and > the live_grants atomic counter is not sufficient on its own to prevent > the mmap() over a pre-existing mapping. > > Link: https://github.com/QubesOS/qubes-issues/issues/7631 > Fixes: ab31523c2fca ("xen/gntdev: allow usermode to map granted pages") > Cc: stable@vger.kernel.org > Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com> Reviewed-by: Juergen Gross <jgross@suse.com> Juergen [-- Attachment #1.1.2: OpenPGP public key --] [-- Type: application/pgp-keys, Size: 3149 bytes --] [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 495 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-23 1:49 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-12 4:00 [PATCH 0/2] xen/gntdev: Fixes for leaks and VMA splitting M. Vefa Bicakci 2022-09-12 4:00 ` [PATCH 1/2] xen/gntdev: Prevent leaking grants M. Vefa Bicakci 2022-09-19 9:52 ` Juergen Gross 2022-09-23 1:48 ` M. Vefa Bicakci 2022-09-12 4:00 ` [PATCH 2/2] xen/gntdev: Accommodate VMA splitting M. Vefa Bicakci 2022-09-19 10:32 ` Juergen Gross
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).