Re: [PATCH RFC v8 52/56] ccp: Add support to decrypt the page

[Tom Lendacky <thomas.lendacky@amd.com>]

On 3/1/23 23:59, Dov Murik wrote:
> Hi Mike, Zhi,
> 
> On 01/03/2023 23:20, Zhi Wang wrote:
>> On Mon, 20 Feb 2023 12:38:43 -0600
>> Michael Roth <michael.roth@amd.com> wrote:
>>
>>> From: Brijesh Singh <brijesh.singh@amd.com>
>>>
>>> Add support to decrypt guest encrypted memory. These API interfaces can
>>> be used for example to dump VMCBs on SNP guest exit.
>>>
>>
>> What kinds of check will be applied from firmware when VMM decrypts this
>> page? I suppose there has to be kinda mechanism to prevent VMM to decrypt
>> any page in the guest. It would be nice to have some introduction about
>> it in the comments.
>>
> 
> The SNP ABI spec says (section 8.27.2 SNP_DBG_DECRYPT):
> 
>    The firmware checks that the guest's policy allows debugging. If not,
>    the firmware returns POLICY_FAILURE.
> 
> and in the Guest Policy (section 4.3):
> 
>    Bit 19 - DEBUG
>    0: Debugging is disallowed.
>    1: Debugging is allowed.
> 
> In the kernel, that firmware error code is defined as
> SEV_RET_POLICY_FAILURE.
> 
> 
>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
>>> [mdr: minor commit fixups]
>>> Signed-off-by: Michael Roth <michael.roth@amd.com>
>>> ---
>>>   drivers/crypto/ccp/sev-dev.c | 32 ++++++++++++++++++++++++++++++++
>>>   include/linux/psp-sev.h      | 22 ++++++++++++++++++++--
>>>   2 files changed, 52 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>>> index e65563bc8298..bf5167b2acfc 100644
>>> --- a/drivers/crypto/ccp/sev-dev.c
>>> +++ b/drivers/crypto/ccp/sev-dev.c
>>> @@ -2017,6 +2017,38 @@ int sev_guest_df_flush(int *error)
>>>   }
>>>   EXPORT_SYMBOL_GPL(sev_guest_df_flush);
>>>   
>>> +int snp_guest_dbg_decrypt_page(u64 gctx_pfn, u64 src_pfn, u64 dst_pfn, int *error)
>>> +{
>>> +	struct sev_data_snp_dbg data = {0};
>>> +	struct sev_device *sev;
>>> +	int ret;
>>> +
>>> +	if (!psp_master || !psp_master->sev_data)
>>> +		return -ENODEV;
>>> +
>>> +	sev = psp_master->sev_data;
>>> +
>>> +	if (!sev->snp_initialized)
>>> +		return -EINVAL;
>>> +
>>> +	data.gctx_paddr = sme_me_mask | (gctx_pfn << PAGE_SHIFT);
>>> +	data.src_addr = sme_me_mask | (src_pfn << PAGE_SHIFT);
>>> +	data.dst_addr = sme_me_mask | (dst_pfn << PAGE_SHIFT);
> 
> I guess this works, but I wonder why we need to turn on sme_me_mask on
> teh dst_addr.  I thought that the firmware decrypts the guest page
> (src_addr) to a plaintext page.  Couldn't find this requirement in the
> SNP spec.

This sme_me_mask tells the firmware how to access the host memory (similar 
to how DMA uses sme_me_mask when supplying addresses to devices under 
SME). This needs to match the pagetable mapping being used by the host, 
otherwise the contents will appears as ciphertext to the host if they are 
not in sync. Since the default pagetable mapping is encrypted, the 
sme_me_mask bit must be provided on the destination address. So it is not 
a spec requirement, but an SME implementation requirement.

Thanks,
Tom

> 
> 
>>> +
>>> +	/* The destination page must be in the firmware state. */
>>> +	if (rmp_mark_pages_firmware(data.dst_addr, 1, false))
>>> +		return -EIO;
>>> +
>>> +	ret = sev_do_cmd(SEV_CMD_SNP_DBG_DECRYPT, &data, error);
>>> +
>>> +	/* Restore the page state */
>>> +	if (snp_reclaim_pages(data.dst_addr, 1, false))
>>> +		ret = -EIO;
>>> +
>>> +	return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(snp_guest_dbg_decrypt_page);
>>> +
>>>   int snp_guest_ext_guest_request(struct sev_data_snp_guest_request *data,
>>>   				unsigned long vaddr, unsigned long *npages, unsigned long *fw_err)
>>>   {
>>> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
>>> index 81bafc049eca..92116e2b74fd 100644
>>> --- a/include/linux/psp-sev.h
>>> +++ b/include/linux/psp-sev.h
>>> @@ -710,7 +710,6 @@ struct sev_data_snp_dbg {
>>>   	u64 gctx_paddr;				/* In */
>>>   	u64 src_addr;				/* In */
>>>   	u64 dst_addr;				/* In */
>>> -	u32 len;				/* In */
>>>   } __packed;
> 
> The comment above this ^^^ struct still lists the 'len' field, and also
> calls the first field 'handle' instead of 'gctx_paddr'.
> 
> Also - why is this change happening in this patch? Why was the incorrect
> 'len' field added in the first place in "[PATCH RFC v8 20/56]
> crypto:ccp: Define the SEV-SNP commands" ? (the comment fixes should
> probably go there too).
> 
> 
> 
>>>   
>>>   /**
>>> @@ -913,13 +912,27 @@ int sev_guest_decommission(struct sev_data_decommission *data, int *error);
>>>    * @error: SEV command return code
>>>    *
>>>    * Returns:
>>> + * 0 if the sev successfully processed the command
>>> + * -%ENODEV    if the sev device is not available
>>> + * -%ENOTSUPP  if the sev does not support SEV
>>> + * -%ETIMEDOUT if the sev command timed out
>>> + * -%EIO       if the sev returned a non-zero return code
>>> + */
> 
> I think that if the word 'sev' would be 'SEV' in this comment, the diff
> will be a bit less misleading (basically this patch should not introduce
> changes to sev_do_cmd).
> 
> -Dov
> 
>>> +int sev_do_cmd(int cmd, void *data, int *psp_ret);
>>> +
>>> +/**
>>> + * snp_guest_dbg_decrypt_page - perform SEV SNP_DBG_DECRYPT command
>>> + *
>>> + * @sev_ret: sev command return code
>>> + *
>>> + * Returns:
>>>    * 0 if the SEV successfully processed the command
>>>    * -%ENODEV    if the SEV device is not available
>>>    * -%ENOTSUPP  if the SEV does not support SEV
>>>    * -%ETIMEDOUT if the SEV command timed out
>>>    * -%EIO       if the SEV returned a non-zero return code
>>>    */
>>> -int sev_do_cmd(int cmd, void *data, int *psp_ret);
>>> +int snp_guest_dbg_decrypt_page(u64 gctx_pfn, u64 src_pfn, u64 dst_pfn, int *error);
>>>   
>>>   void *psp_copy_user_blob(u64 uaddr, u32 len);
>>>   void *snp_alloc_firmware_page(gfp_t mask);
>>> @@ -987,6 +1000,11 @@ static inline void *psp_copy_user_blob(u64 __user uaddr, u32 len) { return ERR_P
>>>   
>>>   void snp_mark_pages_offline(unsigned long pfn, unsigned int npages) {}
>>>   
>>> +static inline int snp_guest_dbg_decrypt_page(u64 gctx_pfn, u64 src_pfn, u64 dst_pfn, int *error)
>>> +{
>>> +	return -ENODEV;
>>> +}
>>> +
>>>   static inline void *snp_alloc_firmware_page(gfp_t mask)
>>>   {
>>>   	return NULL;
>>