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@xxxxxxxxxx>
>> Signed-off-by: Nikunj A Dadhania <nikunj@xxxxxxx>
>> Reviewed-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
>
> 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
