Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <54ff7326-e3a4-945f-1f60-e73dd8865527@amd.com>
Date:   Mon, 9 Jan 2023 16:27:12 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH RFC v7 62/64] x86/sev: Add KVM commands for instance certs
Content-Language: en-US
To:     Dionna Amalie Glaze <dionnaglaze@google.com>,
        Dov Murik <dovmurik@linux.ibm.com>
Cc:     Michael Roth <michael.roth@amd.com>, 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,
        wanpengli@tencent.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, harald@profian.com
References: <20221214194056.161492-1-michael.roth@amd.com>
 <20221214194056.161492-63-michael.roth@amd.com>
 <1c02cc0d-9f0c-cf4a-b012-9932f551dd83@linux.ibm.com>
 <CAAH4kHbhFezeY3D_qoMQBLuFzWNDQF2YLQ-FW_dp5itHShKUWw@mail.gmail.com>
From:   Tom Lendacky <thomas.lendacky@amd.com>
In-Reply-To: <CAAH4kHbhFezeY3D_qoMQBLuFzWNDQF2YLQ-FW_dp5itHShKUWw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NGhDcXgxWU5oRHNpSzdEZ1JzOHorWGI0UktNQzJYWE84b1g5YzFWNTg2S25E?=
 =?utf-8?B?TDR6ZnBRY2VHaERPOWQ0SVBhZFFSQ1pqQ3BaSm9CT0hwaDdicXhCU1VoT2Jx?=
 =?utf-8?B?U0o5QVFnbWU2bExsVWN3Rmt2S2dlV1RFb0tSdm9ueGZmQmtIaU4vQVphc0U4?=
 =?utf-8?B?eVdNM1R0cVBYNDRDYlpBUWgyNERLempxaEJSNndPUHVDalUwanZBSWc4TGw0?=
 =?utf-8?B?cE9nY1l5cGQwZDBYN2prNlhZOWlLTTZPVnh6d3k1MlF5bGRUQWlwZCtadWk4?=
 =?utf-8?B?N3hOaEszem5nejNiSnBFbEZ3MDk4U2VBQ01ZVXNxZS9ldkxNbFhDY1lRcnQy?=
 =?utf-8?B?SUNITDN1RHYzNGx5YWJjeTlkOXc4YmtPQVh6a0Y5cTFEcGtyMTJ4bStiTXVZ?=
 =?utf-8?B?YTUxZzRlQW1aWlErY0VvbVVRRHNybVRJT1JsM1FGTGpveTROMzl2b2paYThv?=
 =?utf-8?B?M3g2RmRtbGRqdU10ZFhManMreCt6eFc3bGlmcmt4YnhLellOTDN0Ni9QOWlo?=
 =?utf-8?B?bzZScVhaYWR2QTQvYnNOOE5Zb1NWQkhQSVJIVXZhUWJoL1JxaFlTZU50Z2dh?=
 =?utf-8?B?enFXWE1tTHJpRlBsUmVJdG1DV3RONGZpZDVIK0dTd0kwNW1XMmhDNGkwQlpJ?=
 =?utf-8?B?WlQvSHRjVXZDb0owOE5iMmZHb1ZRVGxHSy9SUm82ZUQ5NkFMSVVQbXRmeCs4?=
 =?utf-8?B?SEo3OUxucWVyWEwxMmxWclExcHE3NUdHWERpTUplcmR6NmxZb1VvMjY3UGdQ?=
 =?utf-8?B?djZwd0RWMEluUzMvbW1pZStZUmoxS0Rkazh6VHE2djBkM2h1M3FMMUxMbnVu?=
 =?utf-8?B?VzIvM1U2Z01VTjJPRkh6VlFjWFFLU2UrNjNSSlVYMCt6cFBIanJDME9yZzJu?=
 =?utf-8?B?R256WTRVUGxXUTRGTVhWUU9YYTFBdGUxMVljUzJTNm1iblozSGd5b3p5WjNK?=
 =?utf-8?B?N3UvRTByb1drUXo4Nkl0eDZua3BkQkwzSlRQSGlKZjVGWG9hU3QxWFVITDFB?=
 =?utf-8?B?TElKcndET0FiYXFmV1FyNHpvZEh4VEFabnd5Ylkxa0JuN1lsUWNWNTd4dkk3?=
 =?utf-8?B?SVFzcnZOZVYxUFhoRnd2OVBLcmlIRmFXUFJnaFYyM1diazEvR0gxbSt1K3pK?=
 =?utf-8?B?OXZMRTZsYi9CV2xhdjNRdXRwU3hOL1Y4WTVudzV6Vys5QkNRR3RIN2NrV1lR?=
 =?utf-8?B?STJudUphN05Ib2kxbWplN1NhL05OaHhhK0Ztc2I3Wm96TnduNG5jMFlZTG1h?=
 =?utf-8?B?Z3VJb002RHI3RFFMelJlT1BUdGRxTGJac3d2SzV6THppb2F3Tlh3Q0pqVWNG?=
 =?utf-8?B?Y3oxTk9xTmZjcHhaOFd2ai92WjlXVEk3TFhLdVJ5bnhieHF5NGtKak1uRFdS?=
 =?utf-8?B?NW9lVnkva3BSVTByaDVaaDh6bmtoUkI2L3FLNGFrZVR1eXhsT2hza28wNXZW?=
 =?utf-8?B?M25wa1hRcXo3VTR3bnNLb3E1cE9ydVpkK2NBc1oyMk1ITUFhNCtVTE1GdW1Y?=
 =?utf-8?B?OHJwemJvZmVMMlBiendRY1hQZFVjMVZWUkozczk4YXNPUk0zdTcwTXJHU0ZI?=
 =?utf-8?B?NGhKZFlWRkJialFiUVg5aWZOeEh5OTJWQlpEU2tvdGFWQTV5QklmL0NGdGRY?=
 =?utf-8?B?ZC9OMWtHQzE4YnhhVlMvV2I0WHUvN1haSFFHQWJWSjhEVUF0Wk5sQnhyWkxR?=
 =?utf-8?B?eFRrYUN1SEJKRElxWXcxS1BjNnJzeVpSdmw0YVpIdWRzcE5wbFhqTEZsRG54?=
 =?utf-8?B?bExxQTU0NVllTTZIU2Rvdk01SGR5dStudk5WSktDOXNrK0h6REp6UFZXcUtT?=
 =?utf-8?B?WWpOYlBEYjZZZUJLZGhQeUFnYUw1dXkzWWxUMytIcDVxK1RadzMxdDVmRzZs?=
 =?utf-8?B?SlRZa1BlRGRtV3hIYWQ1NHBlNU0wQTJvNGplQ0JUckRLK0ZLS0JiT3l0WHpF?=
 =?utf-8?B?U0luTnhTUzNOYmhhNkZoSm8wYU9XdHF5bmd5V0E5TXNQK1ZaV1VQY0t3cjBY?=
 =?utf-8?B?MjhnbkVJUUJFTGFyNXdnMFM2OE5WNWtwRnFnS3czcVhOYTNmcjNzczZPOEJQ?=
 =?utf-8?B?WS93cjdVTG1DSklGSWFTNWtEdVozb3JMM3lLQ3dzcEtjQng2QWN2MXVtd3h6?=
 =?utf-8?Q?VWJfKiTj94/839KWlbcL0Oz2n?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: cfb0569a-af58-49af-32e5-08daf290aa40
X-MS-Exchange-CrossTenant-AuthSource: DM4PR12MB5229.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Jan 2023 22:27:17.7672
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Hs7jN8hZr6IOlmILuFFaM0Zkv146Kywfo9HwWG8vwbcTTBHCGCNK6cO2PbJibjT/B8Jl52qkPJoPuVBVqzTdog==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB8077
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org

On 1/9/23 10:55, Dionna Amalie Glaze wrote:
>>> +
>>> +static int snp_set_instance_certs(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>> +{
>> [...]
>>
>> Here we set the length to the page-aligned value, but we copy only
>> params.cert_len bytes.  If there are two subsequent
>> snp_set_instance_certs() calls where the second one has a shorter
>> length, we might "keep" some leftover bytes from the first call.
>>
>> Consider:
>> 1. snp_set_instance_certs(certs_addr point to "AAA...", certs_len=8192)
>> 2. snp_set_instance_certs(certs_addr point to "BBB...", certs_len=4097)
>>
>> If I understand correctly, on the second call we'll copy 4097 "BBB..."
>> bytes into the to_certs buffer, but length will be (4096 + PAGE_SIZE -
>> 1) & PAGE_MASK which will be 8192.
>>
>> Later when fetching the certs (for the extended report or in
>> snp_get_instance_certs()) the user will get a buffer of 8192 bytes
>> filled with 4097 BBBs and 4095 leftover AAAs.
>>
>> Maybe zero sev->snp_certs_data entirely before writing to it?
>>
> 
> Yes, I agree it should be zeroed, at least if the previous length is
> greater than the new length. Good catch.
> 
> 
>> Related question (not only for this patch) regarding snp_certs_data
>> (host or per-instance): why is its size page-aligned at all? why is it
>> limited by 16KB or 20KB? If I understand correctly, for SNP, this buffer
>> is never sent to the PSP.
>>
> 
> The buffer is meant to be copied into the guest driver following the
> GHCB extended guest request protocol. The data to copy back are
> expected to be in 4K page granularity.

I don't think the data has to be in 4K page granularity. Why do you think 
it does?

Thanks,
Tom

> 
>> [...]
>>>
>>> -#define SEV_FW_BLOB_MAX_SIZE 0x4000  /* 16KB */
>>> +#define SEV_FW_BLOB_MAX_SIZE 0x5000  /* 20KB */
>>>
>>
>> This has effects in drivers/crypto/ccp/sev-dev.c
>>                                                                 (for
>> example in alloc_snp_host_map).  Is that OK?
>>
> 
> No, this was a mistake of mine because I was using a bloated data
> encoding that needed 5 pages for the GUID table plus 4 small
> certificates. I've since fixed that in our user space code.
> We shouldn't change this size and instead wait for a better size
> negotiation protocol between the guest and host to avoid this awkward
> hard-coding.
> 
>