Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Message-ID: <1cdb3a7f-a30a-4874-ab3a-8fd0f8b5351a@amd.com>
Date:   Thu, 2 Nov 2023 09:03:38 +0530
User-Agent: Mozilla Thunderbird
Reply-To: nikunj@amd.com
Subject: Re: [PATCH v5 01/14] virt: sev-guest: Use AES GCM crypto library
To:     Tom Lendacky <thomas.lendacky@amd.com>,
        linux-kernel@vger.kernel.org, x86@kernel.org, kvm@vger.kernel.org
Cc:     bp@alien8.de, mingo@redhat.com, tglx@linutronix.de,
        dave.hansen@linux.intel.com, dionnaglaze@google.com,
        pgonda@google.com, seanjc@google.com, pbonzini@redhat.com
References: <20231030063652.68675-1-nikunj@amd.com>
 <20231030063652.68675-2-nikunj@amd.com>
 <2a4bbe7b-31e5-7527-8e63-10fb318e0f46@amd.com>
Content-Language: en-US
From:   "Nikunj A. Dadhania" <nikunj@amd.com>
In-Reply-To: <2a4bbe7b-31e5-7527-8e63-10fb318e0f46@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bVRQWEZ1by9oK3JsSU5iRFQ1VlhoKzA0T1h5VExGeWNtVUpCUFk4Z0hsZmNR?=
 =?utf-8?B?c3Q3VUYzRE9aYTgyTGloN29MRGpWM1hlQ1VZQWd5QmlXZ1NGSmlCdHdpZGZu?=
 =?utf-8?B?U3p0bXc2M1Z6S09SLysrNEhWZWtYK2gxUXU3RHhkSlFKTlZXZVhNTGdnUHFV?=
 =?utf-8?B?R2MzcjhlRnZoYVlzUG8zelBXS1VrTm9LejEyZHlkVEF5OFo2emlPaUd5eFor?=
 =?utf-8?B?T2lTTWRvQ3N1cERzSGQxQzVleDFnQVhuQTlVTGxTdHZaWlJROGN6R0pGMXg2?=
 =?utf-8?B?TDE3L3NReGNMdUJjSmJva2JDRitoWGZ2TFdRVVpsTDloeUF5YkRzcHUwMDJw?=
 =?utf-8?B?SElOTmdVcGpEZGlHdmdnc3Fsejg1UllFd1ltenEvSXZja25rU09kTTd6QUQx?=
 =?utf-8?B?VXI4V1U5bjhEam4wQUN3NTU3QlhQZ0FhUUhhU0Q2TVFOVlZId3N2cWNCMzgv?=
 =?utf-8?B?aEwzckJweTg0UDkwbUpxRklWeEtZQjJyZmIzbUZYajlReGhkVGRTRk5NWGlx?=
 =?utf-8?B?KzhkbkZWdit6WFFWS2hnVVdMdzl6bHJFbUZ4OUpvTVBUb0crcFQ3Ujc2S2lU?=
 =?utf-8?B?Rnl3QytlUWt6cmx5YVJwS0U0d1dtY0dvd3p4RlB2bXF0bU1va3Fwd1RUM3Uv?=
 =?utf-8?B?b1JZditCMjVHV1hrTXkxQnBERHIvUzgwWjlwdkVDT24rTGZyd0kwMmJaWjhI?=
 =?utf-8?B?cmhkL0FOZms5NFpwUVRqcHBYVmVHbnRUL2JJTTVkRGlra2Y1NXBCODZZTWY4?=
 =?utf-8?B?UEpmWkRudjZVWE10TDR5azlTV2ZpeHgzOUNtcnNhL0VwRUtPSmJlZmZscFpC?=
 =?utf-8?B?bkMycHNkeS9vQndhRm5QcEhza0VDbTZJT1IrRURWNnR3dG9PZW9BSHpXa2FB?=
 =?utf-8?B?MGR1OHYzUkcrMkRQaW96c2gwOGdEYWFPam14LzFuNXJYRFRZTlo1emRwejBi?=
 =?utf-8?B?SVlaS2xYbmVDS1FYWmJ0SU5mVDA3ZlJIc2hSL09CaFE2Z2tsNVowdXJDcjdp?=
 =?utf-8?B?OTVKeFV2L3lEd2hYT2IrN0xPL1hmYlpkQXhxZkYzSUhqZ0RNYjZrTmZVYVFK?=
 =?utf-8?B?ZVVpMlphUjRKM1M1ZXAwcEhUUlZvVW14NWhDTkxsL1JzMU84VmJNMmNzOC84?=
 =?utf-8?B?eVJRNjBUNjlwQ2xrbTgyM3NnZFNad3FjNTBPT1dLaFhSRFpFOXU5NW1IRE1w?=
 =?utf-8?B?QXl1VXl3b0Q3TFVHUzlTdG1VZzA4empkZUgvamNaU0RwUGNoSytOdm9LajVI?=
 =?utf-8?B?am54L3c5VGRzOWZNc01wRU9Fem1nVENQTk5oZ0FuVDYyeXI5a1B2bkt5OGh2?=
 =?utf-8?B?UFhDa29BNWpEZGR4WngwNk9BUmlHcVd3bzMrdFVGeHZ4UG1tbmVpaWxTOWFI?=
 =?utf-8?B?VVdtMDZHbjdRWHEyS3R1ejdPVHlkcTRycm9mUDJpZjE3VElySGdzRThSQU02?=
 =?utf-8?B?T1RydEJwQ3RpWjJoYWdWZXMyK3lJbGk1L2xBK2hsdWtlaXlpb2NjbWZGRkxL?=
 =?utf-8?B?a2tvVldUMHJSZGtqSG5PRWc2dVcyVFRRWTBkNXNqM1BwUVkwd0lIalNLcDlV?=
 =?utf-8?B?MFE1SEdlOFNOSW13VGZGUkh4RTNESkU4RVd3R0NLVC9McjMvY3BjaDVQVlpI?=
 =?utf-8?B?R2I5VEtlVUxhSnBpbzZ0eFhBZjdaTEZ6WmNUTmJva3NBVjlMZkszcE5nb1Vs?=
 =?utf-8?B?d3I4RFhMWTk1aVFpZVhCVVFsaFhZeVl1Z2tvaHBSODJjd1dya0ZNRjNJa1BW?=
 =?utf-8?B?dmo3aGM3NHRvdlhSRy9XYVNqaENtMy9BL1ExMGtUQkZGTWFkNzI0VU5TR3VC?=
 =?utf-8?B?TVNFN3FPRFF4OXlhMXRqeGovSktRalVISXZEM2NFa0FzRDB5WWNseDlYai9v?=
 =?utf-8?B?aDlOUCttUFo4WlVjRkI2UlpLU0Z5by93bVFpSWZmVHhXM3dISldvVVJaOTZV?=
 =?utf-8?B?ckpxaThOaDd0TXlNekU2cFFXdVljUkNCbUdoNjFiRUJyVy9aL2wvcFhEK0pG?=
 =?utf-8?B?ZlZTSjRkaG5IbVZza0RtU3pFcloyMndYM3RGNzZKM3ZtTXN1ZFEzNW1ISWRk?=
 =?utf-8?B?a3hST0wxcWpZcUhrMVRmZCthQWZjY2w3a2FLS1ExcE1mekZFTVpMT1h2bC83?=
 =?utf-8?Q?fPtLE0jB/mL2jbCFm/oZbVLrD?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0ba855b2-c2a7-46cb-9e48-08dbdb5485dc
X-MS-Exchange-CrossTenant-AuthSource: DS7PR12MB6309.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Nov 2023 03:33:47.6432
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: KVCUAaSDlwk1N1C8ykJA1zGulTt95xeaA62OLVv9fPtbK2eB1Shr4+W7DIs3sklP8EPDDaDrhUBfnQcQ5HJFBg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4388
X-Spam-Status: No, score=-1.3 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE,
	URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Wed, 01 Nov 2023 20:34:21 -0700 (PDT)

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@kernel.org>
>> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
>> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.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