Re: [PATCH v5 07/14] x86/sev: Move and reorganize sev guest request api

[Tom Lendacky]

On 10/30/23 01:36, Nikunj A Dadhania wrote:
> For enabling Secure TSC, SEV-SNP guests need to communicate with the
> AMD Security Processor early during boot. Many of the required
> functions are implemented in the sev-guest driver and therefore not
> available at early boot. Move the required functions and provide an
> API to the driver to assign key and send guest request.
>
> Signed-off-by: Nikunj A Dadhania <nikunj@xxxxxxx>
> ---
>   arch/x86/Kconfig                        |   1 +
>   arch/x86/include/asm/sev-guest.h        |  84 +++-
>   arch/x86/include/asm/sev.h              |  10 -
>   arch/x86/kernel/sev.c                   | 466 ++++++++++++++++++++++-
>   drivers/virt/coco/sev-guest/Kconfig     |   1 -
>   drivers/virt/coco/sev-guest/sev-guest.c | 486 +-----------------------
>   6 files changed, 555 insertions(+), 493 deletions(-)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 66bfabae8814..245a18f6910a 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1509,6 +1509,7 @@ config AMD_MEM_ENCRYPT
>   	select ARCH_HAS_CC_PLATFORM
>   	select X86_MEM_ENCRYPT
>   	select UNACCEPTED_MEMORY
> +	select CRYPTO_LIB_AESGCM
>   	help
>   	  Say yes to enable support for the encryption of system memory.
>   	  This requires an AMD processor that supports Secure Memory
> diff --git a/arch/x86/include/asm/sev-guest.h b/arch/x86/include/asm/sev-guest.h
> index 22ef97b55069..e6f94208173d 100644
> --- a/arch/x86/include/asm/sev-guest.h
> +++ b/arch/x86/include/asm/sev-guest.h
> @@ -11,6 +11,11 @@
>   #define __VIRT_SEVGUEST_H__
>   
>   #include <linux/types.h>
> +#include <linux/miscdevice.h>
> +#include <asm/sev.h>
> +
> +#define SNP_REQ_MAX_RETRY_DURATION    (60*HZ)
> +#define SNP_REQ_RETRY_DELAY           (2*HZ)
>   
>   #define MAX_AUTHTAG_LEN		32
>   #define AUTHTAG_LEN		16
> @@ -58,11 +63,45 @@ struct snp_guest_msg_hdr {
>   	u8 rsvd3[35];
>   } __packed;
>   
> +/* SNP Guest message request */
> +struct snp_req_data {
> +	unsigned long req_gpa;
> +	unsigned long resp_gpa;
> +};
> +
>   struct snp_guest_msg {
>   	struct snp_guest_msg_hdr hdr;
>   	u8 payload[4000];
>   } __packed;
>   
> +struct sev_guest_platform_data {
> +	/* request and response are in unencrypted memory */
> +	struct snp_guest_msg *request, *response;
> +
> +	struct snp_secrets_page_layout *layout;
> +	struct snp_req_data input;
> +};
> +
> +struct snp_guest_dev {
> +	struct device *dev;
> +	struct miscdevice misc;
> +
> +	/* Mutex to serialize the shared buffer access and command handling. */
> +	struct mutex cmd_mutex;
> +
> +	void *certs_data;
> +	struct aesgcm_ctx *ctx;
> +
> +	/*
> +	 * Avoid information leakage by double-buffering shared messages
> +	 * in fields that are in regular encrypted memory
> +	 */
> +	struct snp_guest_msg secret_request, secret_response;
> +
> +	struct sev_guest_platform_data *pdata;
> +	unsigned int vmpck_id;
> +};
> +
>   struct snp_guest_req {
>   	void *req_buf, *resp_buf, *data;
>   	size_t req_sz, resp_sz, *data_npages;
> @@ -72,6 +111,47 @@ struct snp_guest_req {
>   	u8 msg_type;
>   };
>   
> -int snp_issue_guest_request(struct snp_guest_req *req, struct snp_req_data *input,
> -			    struct snp_guest_request_ioctl *rio);
> +int snp_setup_psp_messaging(struct snp_guest_dev *snp_dev);
> +int snp_send_guest_request(struct snp_guest_dev *dev, struct snp_guest_req *req,
> +			   struct snp_guest_request_ioctl *rio);
> +bool snp_assign_vmpck(struct snp_guest_dev *dev, unsigned int vmpck_id);
> +bool snp_is_vmpck_empty(unsigned int vmpck_id);
> +
> +static void free_shared_pages(void *buf, size_t sz)

These should probably be marked __inline if you're going to define them in 
a header file.

> +{
> +	unsigned int npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
> +	int ret;
> +
> +	if (!buf)
> +		return;
> +
> +	ret = set_memory_encrypted((unsigned long)buf, npages);
> +	if (ret) {
> +		WARN_ONCE(ret, "failed to restore encryption mask (leak it)\n");
> +		return;
> +	}
> +
> +	__free_pages(virt_to_page(buf), get_order(sz));
> +}
> +
> +static void *alloc_shared_pages(size_t sz)
> +{
> +	unsigned int npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
> +	struct page *page;
> +	int ret;
> +
> +	page = alloc_pages(GFP_KERNEL_ACCOUNT, get_order(sz));
> +	if (!page)
> +		return NULL;
> +
> +	ret = set_memory_decrypted((unsigned long)page_address(page), npages);
> +	if (ret) {
> +		pr_err("%s: failed to mark page shared, ret=%d\n", __func__, ret);
> +		__free_pages(page, get_order(sz));
> +		return NULL;
> +	}
> +
> +	return page_address(page);
> +}
> +
>   #endif /* __VIRT_SEVGUEST_H__ */
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index 78465a8c7dc6..783150458864 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -93,16 +93,6 @@ extern bool handle_vc_boot_ghcb(struct pt_regs *regs);
>   
>   #define RMPADJUST_VMSA_PAGE_BIT		BIT(16)
>   
> -/* SNP Guest message request */
> -struct snp_req_data {
> -	unsigned long req_gpa;
> -	unsigned long resp_gpa;
> -};
> -
> -struct sev_guest_platform_data {
> -	u64 secrets_gpa;
> -};
> -
>   /*
>    * The secrets page contains 96-bytes of reserved field that can be used by
>    * the guest OS. The guest OS uses the area to save the message sequence
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index fd3b822fa9e7..fb3b1feb1b84 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -24,6 +24,7 @@
>   #include <linux/io.h>
>   #include <linux/psp-sev.h>
>   #include <uapi/linux/sev-guest.h>
> +#include <crypto/gcm.h>
>   
>   #include <asm/cpu_entry_area.h>
>   #include <asm/stacktrace.h>
> @@ -941,6 +942,457 @@ static void snp_cleanup_vmsa(struct sev_es_save_area *vmsa)
>   		free_page((unsigned long)vmsa);
>   }
>   
> +static struct sev_guest_platform_data *platform_data;
> +
> +static inline u8 *snp_get_vmpck(unsigned int vmpck_id)
> +{
> +	if (!platform_data)
> +		return NULL;
> +
> +	return platform_data->layout->vmpck0 + vmpck_id * VMPCK_KEY_LEN;
> +}
> +
> +static inline u32 *snp_get_os_area_msg_seqno(unsigned int vmpck_id)
> +{
> +	if (!platform_data)
> +		return NULL;
> +
> +	return &platform_data->layout->os_area.msg_seqno_0 + vmpck_id;
> +}
> +
> +bool snp_is_vmpck_empty(unsigned int vmpck_id)
> +{
> +	char zero_key[VMPCK_KEY_LEN] = {0};
> +	u8 *key = snp_get_vmpck(vmpck_id);
> +
> +	if (key)
> +		return !memcmp(key, zero_key, VMPCK_KEY_LEN);
> +
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(snp_is_vmpck_empty);
> +
> +/*
> + * If an error is received from the host or AMD Secure Processor (ASP) there
> + * are two options. Either retry the exact same encrypted request or discontinue
> + * using the VMPCK.
> + *
> + * This is because in the current encryption scheme GHCB v2 uses AES-GCM to
> + * encrypt the requests. The IV for this scheme is the sequence number. GCM
> + * cannot tolerate IV reuse.
> + *
> + * The ASP FW v1.51 only increments the sequence numbers on a successful
> + * guest<->ASP back and forth and only accepts messages at its exact sequence
> + * number.
> + *
> + * So if the sequence number were to be reused the encryption scheme is
> + * vulnerable. If the sequence number were incremented for a fresh IV the ASP
> + * will reject the request.
> + */
> +static void snp_disable_vmpck(struct snp_guest_dev *snp_dev)
> +{
> +	u8 *key = snp_get_vmpck(snp_dev->vmpck_id);
> +
> +	pr_alert("Disabling vmpck_id %d to prevent IV reuse.\n", snp_dev->vmpck_id);
> +	memzero_explicit(key, VMPCK_KEY_LEN);
> +}
> +
> +static inline u64 __snp_get_msg_seqno(struct snp_guest_dev *snp_dev)
> +{
> +	u32 *os_area_msg_seqno = snp_get_os_area_msg_seqno(snp_dev->vmpck_id);
> +	u64 count;
> +
> +	if (!os_area_msg_seqno) {
> +		pr_err("SNP unable to get message sequence counter\n");
> +		return 0;
> +	}
> +
> +	lockdep_assert_held(&snp_dev->cmd_mutex);
> +
> +	/* Read the current message sequence counter from secrets pages */
> +	count = *os_area_msg_seqno;
> +
> +	return count + 1;
> +}
> +
> +/* Return a non-zero on success */
> +static u64 snp_get_msg_seqno(struct snp_guest_dev *snp_dev)
> +{
> +	u64 count = __snp_get_msg_seqno(snp_dev);
> +
> +	/*
> +	 * The message sequence counter for the SNP guest request is a  64-bit
> +	 * value but the version 2 of GHCB specification defines a 32-bit storage
> +	 * for it. If the counter exceeds the 32-bit value then return zero.
> +	 * The caller should check the return value, but if the caller happens to
> +	 * not check the value and use it, then the firmware treats zero as an
> +	 * invalid number and will fail the  message request.
> +	 */
> +	if (count >= UINT_MAX) {
> +		pr_err("SNP request message sequence counter overflow\n");
> +		return 0;
> +	}
> +
> +	return count;
> +}
> +
> +static void snp_inc_msg_seqno(struct snp_guest_dev *snp_dev)
> +{
> +	u32 *os_area_msg_seqno = snp_get_os_area_msg_seqno(snp_dev->vmpck_id);
> +
> +	if (!os_area_msg_seqno) {
> +		pr_err("SNP unable to get message sequence counter\n");
> +		return;
> +	}

I probably missed this in the other patch or even when the driver was 
first created, but shouldn't we have a lockdep_assert_held() here, too, 
before updating the count?

> +
> +	/*
> +	 * The counter is also incremented by the PSP, so increment it by 2
> +	 * and save in secrets page.
> +	 */
> +	*os_area_msg_seqno += 2;
> +}
> +
> +static struct aesgcm_ctx *snp_init_crypto(unsigned int vmpck_id)
> +{
> +	struct aesgcm_ctx *ctx;
> +	u8 *key;
> +
> +	if (snp_is_vmpck_empty(vmpck_id)) {
> +		pr_err("SNP: vmpck id %d is null\n", vmpck_id);
> +		return NULL;
> +	}
> +
> +	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL_ACCOUNT);
> +	if (!ctx)
> +		return NULL;
> +
> +	key = snp_get_vmpck(vmpck_id);
> +	if (aesgcm_expandkey(ctx, key, VMPCK_KEY_LEN, AUTHTAG_LEN)) {
> +		pr_err("SNP: crypto init failed\n");
> +		kfree(ctx);
> +		return NULL;
> +	}
> +
> +	return ctx;
> +}
> +
> +int snp_setup_psp_messaging(struct snp_guest_dev *snp_dev)
> +{
> +	struct sev_guest_platform_data *pdata;
> +	int ret;
> +
> +	if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP)) {
> +		pr_err("SNP not supported\n");
> +		return 0;
> +	}
> +
> +	if (platform_data) {
> +		pr_debug("SNP platform data already initialized.\n");
> +		goto create_ctx;
> +	}
> +
> +	if (!secrets_pa) {
> +		pr_err("SNP no secrets page\n");

Maybe "SNP secrets page not found\n" ?

> +		return -ENODEV;
> +	}
> +
> +	pdata = kzalloc(sizeof(struct sev_guest_platform_data), GFP_KERNEL);
> +	if (!pdata) {
> +		pr_err("SNP alloc failed\n");

Maybe "Allocation of SNP guest platform data failed\n" ?

> +		return -ENOMEM;
> +	}
> +
> +	pdata->layout = (__force void *)ioremap_encrypted(secrets_pa, PAGE_SIZE);
> +	if (!pdata->layout) {
> +		pr_err("Unable to locate AP jump table address: failed to map the SNP secrets page.\n");

Maybe "Failed to map SNP secrets page\n" ? Not sure where the AP jump 
table came in on this...

> +		goto e_free_pdata;
> +	}
> +
> +	ret = -ENOMEM;
> +	/* Allocate the shared page used for the request and response message. */
> +	pdata->request = alloc_shared_pages(sizeof(struct snp_guest_msg));
> +	if (!pdata->request)
> +		goto e_unmap;
> +
> +	pdata->response = alloc_shared_pages(sizeof(struct snp_guest_msg));
> +	if (!pdata->response)
> +		goto e_free_request;
> +
> +	/* initial the input address for guest request */
> +	pdata->input.req_gpa = __pa(pdata->request);
> +	pdata->input.resp_gpa = __pa(pdata->response);
> +	platform_data = pdata;
> +
> +create_ctx:
> +	ret = -EIO;
> +	snp_dev->ctx = snp_init_crypto(snp_dev->vmpck_id);
> +	if (!snp_dev->ctx) {
> +		pr_err("SNP init crypto failed\n");

Maybe "SNP crypto context initialization failed\n" ?

> +		platform_data = NULL;
> +		goto e_free_response;
> +	}
> +
> +	snp_dev->pdata = platform_data;

Add a blank line here.

> +	return 0;
> +
> +e_free_response:
> +	free_shared_pages(pdata->response, sizeof(struct snp_guest_msg));
> +e_free_request:
> +	free_shared_pages(pdata->request, sizeof(struct snp_guest_msg));
> +e_unmap:
> +	iounmap(pdata->layout);
> +e_free_pdata:
> +	kfree(pdata);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(snp_setup_psp_messaging);
> +
> +static int __enc_payload(struct aesgcm_ctx *ctx, struct snp_guest_msg *msg,
> +			 void *plaintext, size_t len)
> +{
> +	struct snp_guest_msg_hdr *hdr = &msg->hdr;
> +	u8 iv[GCM_AES_IV_SIZE] = {};
> +
> +	if (WARN_ON((hdr->msg_sz + ctx->authsize) > sizeof(msg->payload)))
> +		return -EBADMSG;
> +
> +	memcpy(iv, &hdr->msg_seqno, sizeof(hdr->msg_seqno));
> +	aesgcm_encrypt(ctx, msg->payload, plaintext, len, &hdr->algo, AAD_LEN,
> +		       iv, hdr->authtag);
> +	return 0;
> +}
> +
> +static int dec_payload(struct aesgcm_ctx *ctx, struct snp_guest_msg *msg,
> +		       void *plaintext, size_t len)
> +{
> +	struct snp_guest_msg_hdr *hdr = &msg->hdr;
> +	u8 iv[GCM_AES_IV_SIZE] = {};
> +
> +	memcpy(iv, &hdr->msg_seqno, sizeof(hdr->msg_seqno));
> +	if (aesgcm_decrypt(ctx, plaintext, msg->payload, len, &hdr->algo,
> +			   AAD_LEN, iv, hdr->authtag))
> +		return 0;
> +	else
> +		return -EBADMSG;
> +}
> +
> +static int verify_and_dec_payload(struct snp_guest_dev *snp_dev, struct snp_guest_req *guest_req,
> +				  struct sev_guest_platform_data *pdata)
> +{
> +	struct snp_guest_msg *resp = &snp_dev->secret_response;
> +	struct snp_guest_msg *req = &snp_dev->secret_request;
> +	struct snp_guest_msg_hdr *req_hdr = &req->hdr;
> +	struct snp_guest_msg_hdr *resp_hdr = &resp->hdr;
> +	struct aesgcm_ctx *ctx = snp_dev->ctx;
> +
> +	pr_debug("response [seqno %lld type %d version %d sz %d]\n",
> +		 resp_hdr->msg_seqno, resp_hdr->msg_type, resp_hdr->msg_version,
> +		 resp_hdr->msg_sz);
> +
> +	/* Copy response from shared memory to encrypted memory. */
> +	memcpy(resp, pdata->response, sizeof(*resp));
> +
> +	/* Verify that the sequence counter is incremented by 1 */
> +	if (unlikely(resp_hdr->msg_seqno != (req_hdr->msg_seqno + 1)))
> +		return -EBADMSG;
> +
> +	/* Verify response message type and version number. */
> +	if (resp_hdr->msg_type != (req_hdr->msg_type + 1) ||
> +	    resp_hdr->msg_version != req_hdr->msg_version)
> +		return -EBADMSG;
> +
> +	/*
> +	 * If the message size is greater than our buffer length then return
> +	 * an error.
> +	 */
> +	if (unlikely((resp_hdr->msg_sz + ctx->authsize) > guest_req->resp_sz))
> +		return -EBADMSG;
> +
> +	return dec_payload(ctx, resp, guest_req->resp_buf, resp_hdr->msg_sz);
> +}
> +
> +static int enc_payload(struct snp_guest_dev *snp_dev, u64 seqno, struct snp_guest_req *req)
> +{
> +	struct snp_guest_msg *msg = &snp_dev->secret_request;
> +	struct snp_guest_msg_hdr *hdr = &msg->hdr;
> +
> +	memset(msg, 0, sizeof(*msg));
> +
> +	hdr->algo = SNP_AEAD_AES_256_GCM;
> +	hdr->hdr_version = MSG_HDR_VER;
> +	hdr->hdr_sz = sizeof(*hdr);
> +	hdr->msg_type = req->msg_type;
> +	hdr->msg_version = req->msg_version;
> +	hdr->msg_seqno = seqno;
> +	hdr->msg_vmpck = req->vmpck_id;
> +	hdr->msg_sz = req->req_sz;
> +
> +	/* Verify the sequence number is non-zero */
> +	if (!hdr->msg_seqno)
> +		return -ENOSR;
> +
> +	pr_debug("request [seqno %lld type %d version %d sz %d]\n",
> +		 hdr->msg_seqno, hdr->msg_type, hdr->msg_version, hdr->msg_sz);
> +
> +	return __enc_payload(snp_dev->ctx, msg, req->req_buf, req->req_sz);
> +}
> +
> +static int snp_issue_guest_request(struct snp_guest_req *req, struct snp_req_data *input,
> +				   struct snp_guest_request_ioctl *rio);

Could all of these routines been moved down closer to the bottom of the 
file to avoid this forward declaration?

> +
> +static int __handle_guest_request(struct snp_guest_dev *snp_dev, struct snp_guest_req *req,
> +				  struct snp_guest_request_ioctl *rio,
> +				  struct sev_guest_platform_data *pdata)
> +{
> +	unsigned long req_start = jiffies;
> +	unsigned int override_npages = 0;
> +	u64 override_err = 0;
> +	int rc;
> +

...

>   
> -e_free_ctx:
> -	kfree(snp_dev->ctx);
>   e_free_cert_data:
>   	free_shared_pages(snp_dev->certs_data, SEV_FW_BLOB_MAX_SIZE);
> -e_free_response:
> -	free_shared_pages(snp_dev->response, sizeof(struct snp_guest_msg));
> -e_free_request:
> -	free_shared_pages(snp_dev->request, sizeof(struct snp_guest_msg));
> -e_unmap:
> -	iounmap(mapping);
> + e_free_ctx:
> +	kfree(snp_dev->ctx);
> +e_free_snpdev:
> +	kfree(snp_dev);
>   	return ret;
>   }
>   
> @@ -780,11 +332,9 @@ static int __exit sev_guest_remove(struct platform_device *pdev)
>   {
>   	struct snp_guest_dev *snp_dev = platform_get_drvdata(pdev);
>   
> -	free_shared_pages(snp_dev->certs_data, SEV_FW_BLOB_MAX_SIZE);

Looks like this one should still be here, right?

Thanks,
Tom

> -	free_shared_pages(snp_dev->response, sizeof(struct snp_guest_msg));
> -	free_shared_pages(snp_dev->request, sizeof(struct snp_guest_msg));
> -	kfree(snp_dev->ctx);
>   	misc_deregister(&snp_dev->misc);
> +	kfree(snp_dev->ctx);
> +	kfree(snp_dev);
>   
>   	return 0;
>   }