linux-kernel - Re: [PATCH RFC v7 62/64] x86/sev: Add KVM commands for instance certs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <54ff7326-e3a4-945f-1f60-e73dd8865527@amd.com>
Date:   Mon, 9 Jan 2023 16:27:12 -0600
From:   Tom Lendacky <thomas.lendacky@....com>
To:     Dionna Amalie Glaze <dionnaglaze@...gle.com>,
        Dov Murik <dovmurik@...ux.ibm.com>
Cc:     Michael Roth <michael.roth@....com>, kvm@...r.kernel.org,
        linux-coco@...ts.linux.dev, linux-mm@...ck.org,
        linux-crypto@...r.kernel.org, x86@...nel.org,
        linux-kernel@...r.kernel.org, tglx@...utronix.de, mingo@...hat.com,
        jroedel@...e.de, hpa@...or.com, ardb@...nel.org,
        pbonzini@...hat.com, seanjc@...gle.com, vkuznets@...hat.com,
        wanpengli@...cent.com, jmattson@...gle.com, luto@...nel.org,
        dave.hansen@...ux.intel.com, slp@...hat.com, pgonda@...gle.com,
        peterz@...radead.org, srinivas.pandruvada@...ux.intel.com,
        rientjes@...gle.com, tobin@....com, bp@...en8.de, vbabka@...e.cz,
        kirill@...temov.name, ak@...ux.intel.com, tony.luck@...el.com,
        marcorr@...gle.com, sathyanarayanan.kuppuswamy@...ux.intel.com,
        alpergun@...gle.com, dgilbert@...hat.com, jarkko@...nel.org,
        ashish.kalra@....com, harald@...fian.com
Subject: Re: [PATCH RFC v7 62/64] x86/sev: Add KVM commands for instance certs

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