Re: [PATCH v5 01/14] virt: sev-guest: Use AES GCM crypto library

[Nikunj A. Dadhania]

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