Messages in this thread | | | Date | Mon, 9 Jan 2023 16:27:12 -0600 | Subject | Re: [PATCH RFC v7 62/64] x86/sev: Add KVM commands for instance certs | From | Tom Lendacky <> |
| |
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. > >
| |