lkml.org 
[lkml]   [2022]   [Oct]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 5.15 059/530] xen/gntdev: Prevent leaking grants
    Date
    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: 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
    @@ -367,8 +367,7 @@ int gntdev_map_grant_pages(struct gntdev
    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

    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,14 @@ static void __unmap_grant_pages_done(int
    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 &&
    + map->unmap_ops[offset + i].handle != INVALID_GRANT_HANDLE)
    + 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 +407,10 @@ static void __unmap_grant_pages_done(int
    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 &&
    + map->kunmap_ops[offset + i].handle != INVALID_GRANT_HANDLE)
    + 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 +419,15 @@ static void __unmap_grant_pages_done(int
    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);

    \
     
     \ /
      Last update: 2022-10-24 15:56    [W:7.299 / U:0.044 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site