Re: [PATCH v2 1/2] xen/gntdev: Prevent leaking grants

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 3806 bytes --]

On 03.10.22 00:20, 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. 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>

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 --]