Received-SPF: pass (google.com: domain of linux-kernel+bounces-99834-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <13581af9-e5f0-41ca-939f-33948b2133e7@linux.intel.com>
Date: Mon, 11 Mar 2024 22:02:48 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 2/5] Drivers: hv: vmbus: Track decrypted status in
 vmbus_gpadl
Content-Language: en-US
To: mhklinux@outlook.com, rick.p.edgecombe@intel.com, kys@microsoft.com,
 haiyangz@microsoft.com, wei.liu@kernel.org, decui@microsoft.com,
 gregkh@linuxfoundation.org, davem@davemloft.net, edumazet@google.com,
 kuba@kernel.org, pabeni@redhat.com, kirill.shutemov@linux.intel.com,
 dave.hansen@linux.intel.com, linux-kernel@vger.kernel.org,
 linux-hyperv@vger.kernel.org, netdev@vger.kernel.org,
 linux-coco@lists.linux.dev
Cc: elena.reshetova@intel.com
References: <20240311161558.1310-1-mhklinux@outlook.com>
 <20240311161558.1310-3-mhklinux@outlook.com>
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
In-Reply-To: <20240311161558.1310-3-mhklinux@outlook.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


On 3/11/24 9:15 AM, mhkelley58@gmail.com wrote:
> From: Rick Edgecombe <rick.p.edgecombe@intel.com>
>
> In CoCo VMs it is possible for the untrusted host to cause
> set_memory_encrypted() or set_memory_decrypted() to fail such that an
> error is returned and the resulting memory is shared. Callers need to
> take care to handle these errors to avoid returning decrypted (shared)
> memory to the page allocator, which could lead to functional or security
> issues.
>
> In order to make sure callers of vmbus_establish_gpadl() and
> vmbus_teardown_gpadl() don't return decrypted/shared pages to
> allocators, add a field in struct vmbus_gpadl to keep track of the
> decryption status of the buffers. This will allow the callers to
> know if they should free or leak the pages.
>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Signed-off-by: Michael Kelley <mhklinux@outlook.com>
> ---
>  drivers/hv/channel.c   | 25 +++++++++++++++++++++----
>  include/linux/hyperv.h |  1 +
>  2 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 56f7e06c673e..bb5abdcda18f 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -472,9 +472,18 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
>  		(atomic_inc_return(&vmbus_connection.next_gpadl_handle) - 1);
>  
>  	ret = create_gpadl_header(type, kbuffer, size, send_offset, &msginfo);
> -	if (ret)
> +	if (ret) {
> +		gpadl->decrypted = false;

Why not set it by default at the beginning of the function?

>  		return ret;
> +	}
>  
> +	/*
> +	 * Set the "decrypted" flag to true for the set_memory_decrypted()
> +	 * success case. In the failure case, the encryption state of the
> +	 * memory is unknown. Leave "decrypted" as true to ensure the
> +	 * memory will be leaked instead of going back on the free list.
> +	 */
> +	gpadl->decrypted = true;
>  	ret = set_memory_decrypted((unsigned long)kbuffer,
>  				   PFN_UP(size));
>  	if (ret) {
> @@ -563,9 +572,15 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
>  
>  	kfree(msginfo);
>  
> -	if (ret)
> -		set_memory_encrypted((unsigned long)kbuffer,
> -				     PFN_UP(size));
> +	if (ret) {
> +		/*
> +		 * If set_memory_encrypted() fails, the decrypted flag is
> +		 * left as true so the memory is leaked instead of being
> +		 * put back on the free list.
> +		 */
> +		if (!set_memory_encrypted((unsigned long)kbuffer, PFN_UP(size)))
> +			gpadl->decrypted = false;
> +	}
>  
>  	return ret;
>  }
> @@ -886,6 +901,8 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, struct vmbus_gpadl *gpad
>  	if (ret)
>  		pr_warn("Fail to set mem host visibility in GPADL teardown %d.\n", ret);

Will this be called only if vmbus_establish_gpad() is successful? If not, you might want to skip
set_memory_encrypted() call for decrypted = false case.

>  
> +	gpadl->decrypted = ret;
> +

IMO, you can set it to false by default. Any way with non zero return, user know about the
decryption failure.

>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(vmbus_teardown_gpadl);
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 2b00faf98017..5bac136c268c 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -812,6 +812,7 @@ struct vmbus_gpadl {
>  	u32 gpadl_handle;
>  	u32 size;
>  	void *buffer;
> +	bool decrypted;
>  };
>  
>  struct vmbus_channel {

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer