Messages in this thread Patch in this message | | | Date | Thu, 12 Oct 2023 10:10:43 +0530 | Subject | Re: [PATCH v4 01/14] virt: sev-guest: Use AES GCM crypto library | From | "Nikunj A. Dadhania" <> |
| |
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
| |