linux-kernel - Re: [PATCH v4 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]

Message-ID: <3b484a1a-e6dc-3b73-ba1d-72fd6ba38ebe@amd.com>
Date:   Thu, 12 Oct 2023 10:10:43 +0530
From:   "Nikunj A. Dadhania" <nikunj@....com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org,
        thomas.lendacky@....com, dionnaglaze@...gle.com, pgonda@...gle.com,
        seanjc@...gle.com, pbonzini@...hat.com
Subject: Re: [PATCH v4 01/14] virt: sev-guest: Use AES GCM crypto library

Hi Boris,

On 10/12/2023 12:26 AM, Borislav Petkov wrote:
> On Mon, Aug 14, 2023 at 11:22:09AM +0530, 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.
>>
>> Link: https://lore.kernel.org/all/20221103192259.2229-1-ardb@kernel.org
> 
> Why is that Link pointing to Ard's lib?
> 
> Link tags are used to point to relevant threads regarding *this* code
> - not the lib you're using...

Ard had added the library specifically for this use case, this was the first 
discussion thread: https://lore.kernel.org/all/c6fb9b25-a4b6-2e4a-2dd1-63adda055a49@amd.com/

Should I add the above link instead ?

> 
>> +static inline unsigned int get_ctx_authsize(struct snp_guest_dev *snp_dev)
>> +{
>> +	if (snp_dev && snp_dev->ctx)
>> +		return snp_dev->ctx->authsize;
>> +
>> +	WARN_ONCE(1, "Unable to get crypto authsize\n");
> 
> What's the point of this?
> 
> You either fail the whole process or you succeed. What's the point of
> warning and still returning 0?
> 
> What do you do when no one is looking at dmesg?

Thinking more about this snp_dev and snp_dev->ctx will not be null, 
because the snp_init_crypto() would have failed resulting in sev-guest driver 
load failure. The below should suffice or I can drop the helper. 

static inline unsigned int get_ctx_authsize(struct snp_guest_dev *snp_dev)
{
	return snp_dev->ctx->authsize;
}

> 
>> +	return 0;
>> +}
>> +
>>  static bool is_vmpck_empty(struct snp_guest_dev *snp_dev)
>>  {
>>  	char zero_key[VMPCK_KEY_LEN] = {0};
>> @@ -152,132 +152,59 @@ static inline struct snp_guest_dev *to_snp_dev(struct file *file)
>>  	return container_of(dev, struct snp_guest_dev, misc);
>>  }
>>  
>> -static struct snp_guest_crypto *init_crypto(struct snp_guest_dev *snp_dev, u8 *key, size_t keylen)
>> +static struct aesgcm_ctx *snp_init_crypto(u8 *key, size_t keylen)
>>  {
>> -	struct snp_guest_crypto *crypto;
>> +	struct aesgcm_ctx *ctx;
>>  
>> -	crypto = kzalloc(sizeof(*crypto), GFP_KERNEL_ACCOUNT);
>> -	if (!crypto)
>> +	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL_ACCOUNT);
>> +	if (!ctx)
>>  		return NULL;
>>  
>> -	crypto->tfm = crypto_alloc_aead("gcm(aes)", 0, 0);
>> -	if (IS_ERR(crypto->tfm))
>> -		goto e_free;
>> -
>> -	if (crypto_aead_setkey(crypto->tfm, key, keylen))
>> -		goto e_free_crypto;
>> -
>> -	crypto->iv_len = crypto_aead_ivsize(crypto->tfm);
>> -	crypto->iv = kmalloc(crypto->iv_len, GFP_KERNEL_ACCOUNT);
>> -	if (!crypto->iv)
>> -		goto e_free_crypto;
>> -
>> -	if (crypto_aead_authsize(crypto->tfm) > MAX_AUTHTAG_LEN) {
>> -		if (crypto_aead_setauthsize(crypto->tfm, MAX_AUTHTAG_LEN)) {
>> -			dev_err(snp_dev->dev, "failed to set authsize to %d\n", MAX_AUTHTAG_LEN);
>> -			goto e_free_iv;
>> -		}
>> +	if (aesgcm_expandkey(ctx, key, keylen, AUTHTAG_LEN)) {
>> +		pr_err("SNP: crypto init failed\n");
> 
> This driver should already be printing with "sev-guest:" prefix - no
> need for "SNP:" too.

Sure, I will change this.

Regards
Nikunj