[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1cdb3a7f-a30a-4874-ab3a-8fd0f8b5351a@amd.com>
Date: Thu, 2 Nov 2023 09:03:38 +0530
From: "Nikunj A. Dadhania" <nikunj@....com>
To: Tom Lendacky <thomas.lendacky@....com>,
linux-kernel@...r.kernel.org, x86@...nel.org, kvm@...r.kernel.org
Cc: bp@...en8.de, mingo@...hat.com, tglx@...utronix.de,
dave.hansen@...ux.intel.com, dionnaglaze@...gle.com,
pgonda@...gle.com, seanjc@...gle.com, pbonzini@...hat.com
Subject: Re: [PATCH v5 01/14] virt: sev-guest: Use AES GCM crypto library
On 10/30/2023 11:21 PM, Tom Lendacky wrote:
> On 10/30/23 01:36, Nikunj A Dadhania wrote:
>> The sev-guest driver encryption code uses Crypto API for SNP guest
>> messaging to interact with AMD Security processor. For enabling SecureTSC,
>> SEV-SNP guests need to send a TSC_INFO request guest message before the
>> smpboot phase starts. Details from the TSC_INFO response will be used to
>> program the VMSA before the secondary CPUs are brought up. The Crypto API
>> is not available this early in the boot phase.
>>
>> In preparation of moving the encryption code out of sev-guest driver to
>> support SecureTSC and make reviewing the diff easier, start using AES GCM
>> library implementation instead of Crypto API.
>>
>> CC: Ard Biesheuvel <ardb@...nel.org>
>> Signed-off-by: Nikunj A Dadhania <nikunj@....com>
>> Reviewed-by: Tom Lendacky <thomas.lendacky@....com>
>
> I just a few nit comments that might be nice to cover if you have to do a v6...
Sure, I will address them in v6.
>> -static int __enc_payload(struct snp_guest_dev *snp_dev, struct snp_guest_msg *msg,
>> +static int __enc_payload(struct aesgcm_ctx *ctx, struct snp_guest_msg *msg,
>> void *plaintext, size_t len)
>> {
>> - struct snp_guest_crypto *crypto = snp_dev->crypto;
>> struct snp_guest_msg_hdr *hdr = &msg->hdr;
>> + u8 iv[GCM_AES_IV_SIZE] = {};
>> - memset(crypto->iv, 0, crypto->iv_len);
>> - memcpy(crypto->iv, &hdr->msg_seqno, sizeof(hdr->msg_seqno));
>> + if (WARN_ON((hdr->msg_sz + ctx->authsize) > sizeof(msg->payload)))
>> + return -EBADMSG;
>> - return enc_dec_message(crypto, msg, plaintext, msg->payload, len, true);
>> + 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;
>> }
>
> __enc_payload() is pretty small now and can probably just be part of the only function that calls it, enc_payload().
>
>> -static int dec_payload(struct snp_guest_dev *snp_dev, struct snp_guest_msg *msg,
>> +static int dec_payload(struct aesgcm_ctx *ctx, struct snp_guest_msg *msg,
>> void *plaintext, size_t len)
>> {
>> - struct snp_guest_crypto *crypto = snp_dev->crypto;
>> struct snp_guest_msg_hdr *hdr = &msg->hdr;
>> + u8 iv[GCM_AES_IV_SIZE] = {};
>> - /* Build IV with response buffer sequence number */
>> - memset(crypto->iv, 0, crypto->iv_len);
>> - memcpy(crypto->iv, &hdr->msg_seqno, sizeof(hdr->msg_seqno));
>> -
>> - return enc_dec_message(crypto, msg, msg->payload, plaintext, len, false);
>> + 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;
>
> This would look cleaner / read easier to me to have as:
>
> if (!aesgcm_decrypt(...))
> return -EBADMSG;
>
> return 0;
>
> But just my opinion.
>
> And ditto here on the size now, can probably just be part of verify_and_dec_payload() now.
>
> Thanks,
> Tom
Regards
Nikunj
Powered by blists - more mailing lists