Re: [PATCH v8 03/16] virt: sev-guest: Add SNP guest request structure

["Nikunj A. Dadhania" <nikunj@amd.com>]

On 2/28/2024 3:50 AM, Tom Lendacky wrote:
> On 2/15/24 05:31, Nikunj A Dadhania wrote:
>> Add a snp_guest_req structure to simplify the function arguments. The
>> structure will be used to call the SNP Guest message request API
>> instead of passing a long list of parameters.
>>
>> Update snp_issue_guest_request() prototype to include the new guest request
>> structure and move the prototype to sev.h.
>>
>> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
>> ---
>>   arch/x86/include/asm/sev.h              |  75 ++++++++-
>>   arch/x86/kernel/sev.c                   |  15 +-
>>   drivers/virt/coco/sev-guest/sev-guest.c | 195 +++++++++++++-----------
>>   drivers/virt/coco/sev-guest/sev-guest.h |  66 --------
>>   4 files changed, 187 insertions(+), 164 deletions(-)
>>   delete mode 100644 drivers/virt/coco/sev-guest/sev-guest.h
>>
>> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
>> index bed95e1f4d52..0c0b11af9f89 100644
>> --- a/arch/x86/include/asm/sev.h
>> +++ b/arch/x86/include/asm/sev.h
>> @@ -111,8 +111,6 @@ struct rmp_state {
>>   struct snp_req_data {
>>       unsigned long req_gpa;
>>       unsigned long resp_gpa;
>> -    unsigned long data_gpa;
>> -    unsigned int data_npages;
>>   };
>>     struct sev_guest_platform_data {
>> @@ -154,6 +152,73 @@ struct snp_secrets_page_layout {
>>       u8 rsvd3[3840];
>>   } __packed;
>>   +#define MAX_AUTHTAG_LEN        32
>> +#define AUTHTAG_LEN        16
>> +#define AAD_LEN            48
>> +#define MSG_HDR_VER        1
>> +
>> +/* See SNP spec SNP_GUEST_REQUEST section for the structure */
>> +enum msg_type {
>> +    SNP_MSG_TYPE_INVALID = 0,
>> +    SNP_MSG_CPUID_REQ,
>> +    SNP_MSG_CPUID_RSP,
>> +    SNP_MSG_KEY_REQ,
>> +    SNP_MSG_KEY_RSP,
>> +    SNP_MSG_REPORT_REQ,
>> +    SNP_MSG_REPORT_RSP,
>> +    SNP_MSG_EXPORT_REQ,
>> +    SNP_MSG_EXPORT_RSP,
>> +    SNP_MSG_IMPORT_REQ,
>> +    SNP_MSG_IMPORT_RSP,
>> +    SNP_MSG_ABSORB_REQ,
>> +    SNP_MSG_ABSORB_RSP,
>> +    SNP_MSG_VMRK_REQ,
>> +    SNP_MSG_VMRK_RSP,
>> +
>> +    SNP_MSG_TYPE_MAX
>> +};
>> +
>> +enum aead_algo {
>> +    SNP_AEAD_INVALID,
>> +    SNP_AEAD_AES_256_GCM,
>> +};
>> +
>> +struct snp_guest_msg_hdr {
>> +    u8 authtag[MAX_AUTHTAG_LEN];
>> +    u64 msg_seqno;
>> +    u8 rsvd1[8];
>> +    u8 algo;
>> +    u8 hdr_version;
>> +    u16 hdr_sz;
>> +    u8 msg_type;
>> +    u8 msg_version;
>> +    u16 msg_sz;
>> +    u32 rsvd2;
>> +    u8 msg_vmpck;
>> +    u8 rsvd3[35];
>> +} __packed;
>> +
>> +struct snp_guest_msg {
>> +    struct snp_guest_msg_hdr hdr;
>> +    u8 payload[4000];
> 
> If the idea is to ensure that payload never goes beyond a page boundary (assuming page allocation/backing), it would be better to have:
> 
>     u8 payload[PAGE_SIZE - sizeof(struct snp_guest_msg_hdr)];
> 
> instead of hard-coding 4000 (I realize this is existing code). Although, since you probably want to ensure that you don't exceed the page allocation by testing against the size or page offset, you can just make this a variable length array:
> 
>     u8 payload[];
> 
> and ensure that you don't overrun.

Sure, below is the delta to make payload a variable length array. I will squash it with current patch.

diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 0c0b11af9f89..85cf160f6203 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -200,9 +200,12 @@ struct snp_guest_msg_hdr {
 
 struct snp_guest_msg {
 	struct snp_guest_msg_hdr hdr;
-	u8 payload[4000];
+	u8 payload[];
 } __packed;
 
+#define SNP_GUEST_MSG_SIZE 4096
+#define SNP_GUEST_MSG_PAYLOAD_SIZE (SNP_GUEST_MSG_SIZE - sizeof(struct snp_guest_msg))
+
 struct snp_guest_req {
 	void *req_buf;
 	size_t req_sz;
diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index 596cec03f9eb..da9a616c76cf 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -46,7 +46,7 @@ struct snp_guest_dev {
 	 * 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 snp_guest_msg *secret_request, *secret_response;
 
 	struct snp_secrets_page_layout *layout;
 	struct snp_req_data input;
@@ -169,8 +169,8 @@ static struct aesgcm_ctx *snp_init_crypto(u8 *key, size_t keylen)
 
 static int verify_and_dec_payload(struct snp_guest_dev *snp_dev, struct snp_guest_req *req)
 {
-	struct snp_guest_msg *resp_msg = &snp_dev->secret_response;
-	struct snp_guest_msg *req_msg = &snp_dev->secret_request;
+	struct snp_guest_msg *resp_msg = snp_dev->secret_response;
+	struct snp_guest_msg *req_msg = snp_dev->secret_request;
 	struct snp_guest_msg_hdr *req_msg_hdr = &req_msg->hdr;
 	struct snp_guest_msg_hdr *resp_msg_hdr = &resp_msg->hdr;
 	struct aesgcm_ctx *ctx = snp_dev->ctx;
@@ -181,7 +181,7 @@ static int verify_and_dec_payload(struct snp_guest_dev *snp_dev, struct snp_gues
 		 resp_msg_hdr->msg_sz);
 
 	/* Copy response from shared memory to encrypted memory. */
-	memcpy(resp_msg, snp_dev->response, sizeof(*resp_msg));
+	memcpy(resp_msg, snp_dev->response, SNP_GUEST_MSG_SIZE);
 
 	/* Verify that the sequence counter is incremented by 1 */
 	if (unlikely(resp_msg_hdr->msg_seqno != (req_msg_hdr->msg_seqno + 1)))
@@ -210,7 +210,7 @@ static int verify_and_dec_payload(struct snp_guest_dev *snp_dev, struct snp_gues
 
 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 *msg = snp_dev->secret_request;
 	struct snp_guest_msg_hdr *hdr = &msg->hdr;
 	struct aesgcm_ctx *ctx = snp_dev->ctx;
 	u8 iv[GCM_AES_IV_SIZE] = {};
@@ -233,7 +233,7 @@ static int enc_payload(struct snp_guest_dev *snp_dev, u64 seqno, struct snp_gues
 	pr_debug("request [seqno %lld type %d version %d sz %d]\n",
 		 hdr->msg_seqno, hdr->msg_type, hdr->msg_version, hdr->msg_sz);
 
-	if (WARN_ON((req->req_sz + ctx->authsize) > sizeof(msg->payload)))
+	if (WARN_ON((req->req_sz + ctx->authsize) > SNP_GUEST_MSG_PAYLOAD_SIZE))
 		return -EBADMSG;
 
 	memcpy(iv, &hdr->msg_seqno, min(sizeof(iv), sizeof(hdr->msg_seqno)));
@@ -341,7 +341,7 @@ static int snp_send_guest_request(struct snp_guest_dev *snp_dev, struct snp_gues
 		return -EIO;
 
 	/* Clear shared memory's response for the host to populate. */
-	memset(snp_dev->response, 0, sizeof(struct snp_guest_msg));
+	memset(snp_dev->response, 0, SNP_GUEST_MSG_SIZE);
 
 	/* Encrypt the userspace provided payload in snp_dev->secret_request. */
 	rc = enc_payload(snp_dev, seqno, req);
@@ -352,8 +352,7 @@ static int snp_send_guest_request(struct snp_guest_dev *snp_dev, struct snp_gues
 	 * Write the fully encrypted request to the shared unencrypted
 	 * request page.
 	 */
-	memcpy(snp_dev->request, &snp_dev->secret_request,
-	       sizeof(snp_dev->secret_request));
+	memcpy(snp_dev->request, snp_dev->secret_request, SNP_GUEST_MSG_SIZE);
 
 	rc = __handle_guest_request(snp_dev, req, rio);
 	if (rc) {
@@ -864,12 +863,21 @@ static int __init sev_guest_probe(struct platform_device *pdev)
 	snp_dev->dev = dev;
 	snp_dev->layout = layout;
 
+	/* Allocate secret request and response message for double buffering */
+	snp_dev->secret_request = kzalloc(SNP_GUEST_MSG_SIZE, GFP_KERNEL);
+	if (!snp_dev->secret_request)
+		goto e_unmap;
+
+	snp_dev->secret_response = kzalloc(SNP_GUEST_MSG_SIZE, GFP_KERNEL);
+	if (!snp_dev->secret_response)
+		goto e_free_secret_req;
+
 	/* Allocate the shared page used for the request and response message. */
-	snp_dev->request = alloc_shared_pages(dev, sizeof(struct snp_guest_msg));
+	snp_dev->request = alloc_shared_pages(dev, SNP_GUEST_MSG_SIZE);
 	if (!snp_dev->request)
-		goto e_unmap;
+		goto e_free_secret_resp;
 
-	snp_dev->response = alloc_shared_pages(dev, sizeof(struct snp_guest_msg));
+	snp_dev->response = alloc_shared_pages(dev, SNP_GUEST_MSG_SIZE);
 	if (!snp_dev->response)
 		goto e_free_request;
 
@@ -911,9 +919,13 @@ static int __init sev_guest_probe(struct platform_device *pdev)
 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));
+	free_shared_pages(snp_dev->response, SNP_GUEST_MSG_SIZE);
 e_free_request:
-	free_shared_pages(snp_dev->request, sizeof(struct snp_guest_msg));
+	free_shared_pages(snp_dev->request, SNP_GUEST_MSG_SIZE);
+e_free_secret_resp:
+	kfree(snp_dev->secret_response);
+e_free_secret_req:
+	kfree(snp_dev->secret_request);
 e_unmap:
 	iounmap(mapping);
 	return ret;
@@ -924,9 +936,11 @@ static void __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);
-	free_shared_pages(snp_dev->response, sizeof(struct snp_guest_msg));
-	free_shared_pages(snp_dev->request, sizeof(struct snp_guest_msg));
+	free_shared_pages(snp_dev->response, SNP_GUEST_MSG_SIZE);
+	free_shared_pages(snp_dev->request, SNP_GUEST_MSG_SIZE);
 	kfree(snp_dev->ctx);
+	kfree(snp_dev->secret_response);
+	kfree(snp_dev->secret_request);
 	misc_deregister(&snp_dev->misc);
 }