Received-SPF: pass (google.com: domain of linux-kernel+bounces-88988-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: Michael Kelley <mhklinux@outlook.com>
To: Rick Edgecombe <rick.p.edgecombe@intel.com>, "kys@microsoft.com"
	<kys@microsoft.com>, "haiyangz@microsoft.com" <haiyangz@microsoft.com>,
	"wei.liu@kernel.org" <wei.liu@kernel.org>, "decui@microsoft.com"
	<decui@microsoft.com>, "linux-hyperv@vger.kernel.org"
	<linux-hyperv@vger.kernel.org>, "gregkh@linuxfoundation.org"
	<gregkh@linuxfoundation.org>, "davem@davemloft.net" <davem@davemloft.net>,
	"edumazet@google.com" <edumazet@google.com>, "kuba@kernel.org"
	<kuba@kernel.org>, "pabeni@redhat.com" <pabeni@redhat.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"kirill.shutemov@linux.intel.com" <kirill.shutemov@linux.intel.com>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
	"linux-coco@lists.linux.dev" <linux-coco@lists.linux.dev>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
CC: "sathyanarayanan.kuppuswamy@linux.intel.com"
	<sathyanarayanan.kuppuswamy@linux.intel.com>, "elena.reshetova@intel.com"
	<elena.reshetova@intel.com>
Subject: RE: [RFC RFT PATCH 1/4] hv: Leak pages if set_memory_encrypted()
 fails
Thread-Topic: [RFC RFT PATCH 1/4] hv: Leak pages if set_memory_encrypted()
 fails
Thread-Index: AQHaZTRYDcNuyb66Gkm4wm9TLGb/jbEjO5pg
Date: Fri, 1 Mar 2024 19:00:38 +0000
Message-ID:
 <SN6PR02MB4157B2E6EC690B11AB334522D45E2@SN6PR02MB4157.namprd02.prod.outlook.com>
References: <20240222021006.2279329-1-rick.p.edgecombe@intel.com>
 <20240222021006.2279329-2-rick.p.edgecombe@intel.com>
In-Reply-To: <20240222021006.2279329-2-rick.p.edgecombe@intel.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0:
 =?us-ascii?Q?B8vFaNELlKMwP/JYczfrlaV/jjfIvdfpzZfsWFCQE+HmtgTQ/YBAdXid+yax?=
 =?us-ascii?Q?spf1HTa15/LW3i8+E5QnSAEK+FHah6U/kevqBn4wBXbuG8hyTgc7oKDF87hF?=
 =?us-ascii?Q?awsJicLMwRDqsmMvyzWSW2IOkTDvkHjXW6zmWVBy7oua2Ck2VX3XWiBRzBKA?=
 =?us-ascii?Q?MguSQTqvAWv1zIc7RziShNw3ATBb/zdYy/WzHiEa/nm4xrwX46LcEGZhWBDf?=
 =?us-ascii?Q?dmzLE433rny+QY9zgpQMG8ZcIJgf7oR0Uw2lA+a5LpJO4kybN2NDZJwYeuCO?=
 =?us-ascii?Q?P6x5LJEIaGXl7eSXN4IdfwGIOQJwZqrlZAA4q/gKfm1Ox+WCZ7RjSKb9a0XW?=
 =?us-ascii?Q?EZLOZBOzotk2AEEYukWSI3Oz8i/ER4DOLPgOKji+V1JppNkjrgo7ZIWn6lhl?=
 =?us-ascii?Q?qbrnY2wHScvi/yuOLRkqwu2/bAOS7rG8GyyZCKi7Rof9Z19fmCl3jfTeZ686?=
 =?us-ascii?Q?Xq6G6qW0tSZBwVT/TFr+FNm/hqTPqVtOAma3s/OayQ1/rxEX2rLpL6HrOYYQ?=
 =?us-ascii?Q?xNnCPHznN3/dH3vKGVaMoYvM6fseaaYKrfzLBhQwX25X8fh0Wo62q/mRPGdD?=
 =?us-ascii?Q?UmSCgHCtzPuD+8IZXeLu9SagGdBShzmebHGAtb6T5f+Cj3YgSpzPWugrOaPU?=
 =?us-ascii?Q?W0HCFicsb0lvYgKetrXiDfd0VxZpvLtOQMELGGXV/SZB2+BSbs3HLRXn1Vvn?=
 =?us-ascii?Q?PlP853Bzx4Pi63igVRK6YfrNILYJ2q6mlHi2dO4t2pU0eqeoMRRg1Mxq7fEw?=
 =?us-ascii?Q?KugWKi9DPKf9xtoOXEd2d6ubjU3CXYyiuQq/A00uJCnXBdBg0l+kNBv9NOTL?=
 =?us-ascii?Q?teQJFQgANqB/ltSxONZyVlaQdvfjmBCq7HD7be1pTgfyPEJWL+NNtgaQ/T36?=
 =?us-ascii?Q?kvoYDvBkjvTiGJAmLHhiKdF6XU869AncLEj2mJz8Lys8bvhw8Xv+iPmIKfRA?=
 =?us-ascii?Q?I2QKV6Zjp8Ik/bH6IEebmxNyRND7GqLMowIvZH1TLWXgKBYHJty/s2AhCAqg?=
 =?us-ascii?Q?MFrxpDuP34c4LCp7j/tPegkAKcyvXvyPiK8qipOxl/rYHooa15lD8wkE6NQE?=
 =?us-ascii?Q?hkFkMz9pTfNS1abzIiE7PZX3NLWeh2Id+jtDpvRRNbSbcYrLy68IMrY+AVAf?=
 =?us-ascii?Q?ozkJ/7YUvU6dWBBvBwYwdLHHlCI0rDxF1UJ33Z2vQqLMpB/Rh0RsP/LKOAQ7?=
 =?us-ascii?Q?un9KcFYDWxZ5SvMXSn1496jEcLfTh5KglmYwHw=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SN6PR02MB4157.namprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000
X-MS-Exchange-CrossTenant-Network-Message-Id: b60ac30e-baa6-40cb-0ece-08dc3a21e22e
X-MS-Exchange-CrossTenant-rms-persistedconsumerorg: 00000000-0000-0000-0000-000000000000
X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Mar 2024 19:00:38.2352
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR02MB8941

From: Rick Edgecombe <rick.p.edgecombe@intel.com> Sent: Wednesday, February=
 21, 2024 6:10 PM
>=20

Historically, the preferred Subject prefix for changes to connection.c has
been "Drivers: hv: vmbus:", not just "hv:".  Sometimes that preference
isn't followed, but most of the time it is.

> On TDX it is possible for the untrusted host to cause

I'd argue that this is for CoCo VMs in general, not just TDX.  I don't know
all the failure modes for SEV-SNP, but the code paths you are changing
are run in both TDX and SEV-SNP CoCo VMs.

> set_memory_encrypted() or set_memory_decrypted() to fail such that an
> error is returned and the resulting memory is shared. Callers need to tak=
e
> care to handle these errors to avoid returning decrypted (shared) memory =
to
> the page allocator, which could lead to functional or security issues.
>=20
> Hyperv could free decrypted/shared pages if set_memory_encrypted() fails.

It's not Hyper-V doing the freeing.  Maybe say "VMBus code could
free ...."

> Leak the pages if this happens.
>=20
> Only compile tested.
>=20
> Cc: "K. Y. Srinivasan" <kys@microsoft.com>
> Cc: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: Wei Liu <wei.liu@kernel.org>
> Cc: Dexuan Cui <decui@microsoft.com>
> Cc: linux-hyperv@vger.kernel.org
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
>  drivers/hv/connection.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>=20
> diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
> index 3cabeeabb1ca..e39493421bbb 100644
> --- a/drivers/hv/connection.c
> +++ b/drivers/hv/connection.c
> @@ -315,6 +315,7 @@ int vmbus_connect(void)
>=20
>  void vmbus_disconnect(void)
>  {
> +	int ret;
>  	/*
>  	 * First send the unload request to the host.
>  	 */
> @@ -337,11 +338,13 @@ void vmbus_disconnect(void)
>  		vmbus_connection.int_page =3D NULL;
>  	}
>=20
> -	set_memory_encrypted((unsigned long)vmbus_connection.monitor_pages[0], =
1);
> -	set_memory_encrypted((unsigned long)vmbus_connection.monitor_pages[1], =
1);
> +	ret =3D set_memory_encrypted((unsigned long)vmbus_connection.monitor_pa=
ges[0], 1);
> +	ret |=3D set_memory_encrypted((unsigned long)vmbus_connection.monitor_p=
ages[1], 1);
>=20
> -	hv_free_hyperv_page(vmbus_connection.monitor_pages[0]);
> -	hv_free_hyperv_page(vmbus_connection.monitor_pages[1]);
> +	if (!ret) {
> +		hv_free_hyperv_page(vmbus_connection.monitor_pages[0]);
> +		hv_free_hyperv_page(vmbus_connection.monitor_pages[1]);
> +	}

Of course, this will leak the memory for both pages if only one of the
set_memory_encrypted() calls fails, but I'm OK with that.  It doesn't
seem worth the additional complexity to treat each page separately.

>  	vmbus_connection.monitor_pages[0] =3D NULL;
>  	vmbus_connection.monitor_pages[1] =3D NULL;
>  }
> --
> 2.34.1