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

[Dov Murik]

On 02/03/2023 16:33, Tom Lendacky wrote:
> 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@xxxxxxx> wrote:
>>>
>>>> From: Brijesh Singh <brijesh.singh@xxxxxxx>
>>>>
>>>> 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@xxxxxxx>
>>>> Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
>>>> [mdr: minor commit fixups]
>>>> Signed-off-by: Michael Roth <michael.roth@xxxxxxx>
>>>> ---
>>>>   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.
> 

Ah, OK, that's clear now. Thanks Tom.

-Dov