Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: iac594737:Fastmail
Date:   Sun, 2 Oct 2022 20:29:59 -0400
From:   Demi Marie Obenour <demi@invisiblethingslab.com>
To:     "M. Vefa Bicakci" <m.v.b@runbox.com>,
        xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org
Cc:     Juergen Gross <jgross@suse.com>,
        Stefano Stabellini <sstabellini@kernel.org>,
        Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
        Gerd Hoffmann <kraxel@redhat.com>
Subject: Re: [PATCH v2 1/2] xen/gntdev: Prevent leaking grants
Message-ID: <YzotDube4gTn7r9b@itl-email>
References: <20221002222006.2077-1-m.v.b@runbox.com>
 <20221002222006.2077-2-m.v.b@runbox.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="g3VIftLCTfH9xeQV"
Content-Disposition: inline
In-Reply-To: <20221002222006.2077-2-m.v.b@runbox.com>
Precedence: bulk


--g3VIftLCTfH9xeQV
Content-Type: text/plain; protected-headers=v1; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
Date: Sun, 2 Oct 2022 20:29:59 -0400
From: Demi Marie Obenour <demi@invisiblethingslab.com>
To: "M. Vefa Bicakci" <m.v.b@runbox.com>, xen-devel@lists.xenproject.org,
	linux-kernel@vger.kernel.org
Cc: Juergen Gross <jgross@suse.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
	Gerd Hoffmann <kraxel@redhat.com>
Subject: Re: [PATCH v2 1/2] xen/gntdev: Prevent leaking grants

On Sun, Oct 02, 2022 at 06:20:05PM -0400, 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:
>=20
>   for (i =3D 0; i < map->count; i++) {
>     if (map->map_ops[i].status =3D=3D GNTST_okay) {
>       map->unmap_ops[i].handle =3D map->map_ops[i].handle;
>       if (!use_ptemod)
>         alloced++;
>     }
>     if (use_ptemod) {
>       if (map->kmap_ops[i].status =3D=3D GNTST_okay) {
>         if (map->map_ops[i].status =3D=3D GNTST_okay)
>           alloced++;
>         map->kunmap_ops[i].handle =3D map->kmap_ops[i].handle;
>       }
>     }
>   }
>   ...
>   atomic_add(alloced, &map->live_grants);
>=20
> 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.
>=20
> 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:
>=20
>   if (atomic_read(&map->live_grants) =3D=3D 0)
>     return; /* Nothing to do */
>=20
> 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.
>=20
> 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).
>=20
> 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.
>=20
> Link: https://github.com/QubesOS/qubes-issues/issues/7631
> Fixes: dbe97cff7dd9 ("xen/gntdev: Avoid blocking in unmap_grant_pages()")

Looks like this patch has been pretty buggy, sorry.  This is the second
time there has been a problem with it.  Thanks for the fix.

> Cc: stable@vger.kernel.org
> Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com>
> ---
>=20
> Changes since v1:
> - To determine which unmap operations were successful, the previous
>   version of this patch set the "unmap_ops[i].status" and
>   "kunmap_ops[i].status" fields to the value "1" prior to passing these
>   data structures to the hypervisor. Instead of doing that, the code now
>   checks whether the "handle" fields in the same data structures were
>   *not* set to "INVALID_GRANT_HANDLE". (Suggested by Juergen Gross.)
> ---
>  drivers/xen/gntdev.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
>=20
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index 84b143eef395..eb0586b9767d 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -367,8 +367,7 @@ int gntdev_map_grant_pages(struct gntdev_grant_map *m=
ap)
>  	for (i =3D 0; i < map->count; i++) {
>  		if (map->map_ops[i].status =3D=3D GNTST_okay) {
>  			map->unmap_ops[i].handle =3D map->map_ops[i].handle;
> -			if (!use_ptemod)
> -				alloced++;
> +			alloced++;
>  		} else if (!err)
>  			err =3D -EINVAL;
> =20
> @@ -377,8 +376,7 @@ int gntdev_map_grant_pages(struct gntdev_grant_map *m=
ap)
> =20
>  		if (use_ptemod) {
>  			if (map->kmap_ops[i].status =3D=3D GNTST_okay) {
> -				if (map->map_ops[i].status =3D=3D GNTST_okay)
> -					alloced++;
> +				alloced++;
>  				map->kunmap_ops[i].handle =3D map->kmap_ops[i].handle;
>  			} else if (!err)
>  				err =3D -EINVAL;
> @@ -394,8 +392,14 @@ static void __unmap_grant_pages_done(int result,
>  	unsigned int i;
>  	struct gntdev_grant_map *map =3D data->data;
>  	unsigned int offset =3D data->unmap_ops - map->unmap_ops;
> +	int successful_unmaps =3D 0;
> +	int live_grants;
> =20
>  	for (i =3D 0; i < data->count; i++) {
> +		if (map->unmap_ops[offset + i].status =3D=3D GNTST_okay &&
> +		    map->unmap_ops[offset + i].handle !=3D INVALID_GRANT_HANDLE)
> +			successful_unmaps++;
> +
>  		WARN_ON(map->unmap_ops[offset + i].status !=3D GNTST_okay &&
>  			map->unmap_ops[offset + i].handle !=3D INVALID_GRANT_HANDLE);
>  		pr_debug("unmap handle=3D%d st=3D%d\n",
> @@ -403,6 +407,10 @@ static void __unmap_grant_pages_done(int result,
>  			map->unmap_ops[offset+i].status);
>  		map->unmap_ops[offset+i].handle =3D INVALID_GRANT_HANDLE;
>  		if (use_ptemod) {
> +			if (map->kunmap_ops[offset + i].status =3D=3D GNTST_okay &&
> +			    map->kunmap_ops[offset + i].handle !=3D INVALID_GRANT_HANDLE)
> +				successful_unmaps++;
> +
>  			WARN_ON(map->kunmap_ops[offset + i].status !=3D GNTST_okay &&
>  				map->kunmap_ops[offset + i].handle !=3D INVALID_GRANT_HANDLE);
>  			pr_debug("kunmap handle=3D%u st=3D%d\n",
> @@ -411,11 +419,15 @@ static void __unmap_grant_pages_done(int result,
>  			map->kunmap_ops[offset+i].handle =3D 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 =3D 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);
> =20
>  	/* Release reference taken by __unmap_grant_pages */
>  	gntdev_put_map(NULL, map);
> --=20
> 2.37.3

Is there a possibility that live_grants could overflow, as it is now
set to a value twice as large as what it had been previously?

If not, you can add:

Acked-by: Demi Marie Obenour <demi@invisiblethingslab.com>
--=20
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

--g3VIftLCTfH9xeQV
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEdodNnxM2uiJZBxxxsoi1X/+cIsEFAmM6LQ8ACgkQsoi1X/+c
IsE/mxAAjAWZ3FBXk78lfnRfKqcGSGmPfM6Mztzu0e1i3Q1ckY25IIvfM8kKRc/y
LpUo2Ei39YXyJNitexDxjqd2+HfOYOI1Ln2mmiI8IpvjBSgTwOO5XAA8YoSstc8g
IQUXK+wukBYaO+AAmb0JPMhpfKGrDV0tvfyz4G2k7u11pcY8F218pAiSQHtoSrCg
3MCO0g7rZMCwM6CcWiHup5dujAzgUDgr0Zen1JWAicRjeXewHoSCDcPwdTHWExft
ETT/FcI67wjSL9QTiCliZDLBcuJ4/HMs6fNDv8SFB0xM29ebxN08aJFyHzqps/8A
ZOA5n5g6Ltnjda9f7k9Zfx+nLkn2aKaw4qc5YLMqt0XzdVPMZEGiOo8mZvnlMfHK
V/QczwqHXcX4bPw/gjftXFIdXQN7jWOfr2svI5Lp07jD5QVhHQD3/2Asgjh6U9/F
R1j1c6i34SuVothF9kHKtBRGW/L7wY5WrLBOx75uXSYB/aKVgewNA1aVo+098sRN
6/TtuYnwmSAJl3O0iIMYaXME6+CRU63b2XMj/1t1KE9N1nfcNPqqegABhIQbgn0q
kBz9a+DGhjb7xSwN56g4JUpQ2A48ij7/x7aQGkQqbkfUBQsyLZNGS5ALhEmEUaIZ
MOKGzzC8G5IgwyvXa/+mdloMIZkyWUYqQ/lECYAjv3BhTPxTIWQ=
=6a+L
-----END PGP SIGNATURE-----

--g3VIftLCTfH9xeQV--