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

[Borislav Petkov]

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@xxxxxxxxxx

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...

> +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?

> +	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.

> +		kfree(ctx);
> +		return NULL;
>  	}

...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette