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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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