Message-ID: <740d3ee7-e981-0812-f21e-296a7f350388@linux.ibm.com>
Date:   Thu, 2 Mar 2023 23:11:13 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH RFC v8 52/56] ccp: Add support to decrypt the page
Content-Language: en-US
To:     Tom Lendacky <thomas.lendacky@amd.com>,
        Zhi Wang <zhi.wang.linux@gmail.com>,
        Michael Roth <michael.roth@amd.com>
Cc:     kvm@vger.kernel.org, linux-coco@lists.linux.dev,
        linux-mm@kvack.org, linux-crypto@vger.kernel.org, x86@kernel.org,
        linux-kernel@vger.kernel.org, tglx@linutronix.de, mingo@redhat.com,
        jroedel@suse.de, hpa@zytor.com, ardb@kernel.org,
        pbonzini@redhat.com, seanjc@google.com, vkuznets@redhat.com,
        jmattson@google.com, luto@kernel.org, dave.hansen@linux.intel.com,
        slp@redhat.com, pgonda@google.com, peterz@infradead.org,
        srinivas.pandruvada@linux.intel.com, rientjes@google.com,
        tobin@ibm.com, bp@alien8.de, vbabka@suse.cz, kirill@shutemov.name,
        ak@linux.intel.com, tony.luck@intel.com, marcorr@google.com,
        sathyanarayanan.kuppuswamy@linux.intel.com, alpergun@google.com,
        dgilbert@redhat.com, jarkko@kernel.org, ashish.kalra@amd.com,
        nikunj.dadhania@amd.com, Brijesh Singh <brijesh.singh@amd.com>,
        Dov Murik <dovmurik@linux.ibm.com>
References: <20230220183847.59159-1-michael.roth@amd.com>
 <20230220183847.59159-53-michael.roth@amd.com>
 <20230301232045.0000502e@intel.com>
 <e63ba525-644d-1a8c-afe7-2ced4a8fbb93@linux.ibm.com>
 <36734887-6474-b43e-51ae-34f37e6670a5@amd.com>
From:   Dov Murik <dovmurik@linux.ibm.com>
In-Reply-To: <36734887-6474-b43e-51ae-34f37e6670a5@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


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

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

-Dov