| 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
| ||
|
Message-ID: <1c02cc0d-9f0c-cf4a-b012-9932f551dd83@linux.ibm.com>
Date: Thu, 22 Dec 2022 16:57:30 +0200
From: Dov Murik <dovmurik@...ux.ibm.com>
To: Michael Roth <michael.roth@....com>, kvm@...r.kernel.org
Cc: 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, thomas.lendacky@....com, 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, Dionna Glaze <dionnaglaze@...gle.com>,
Dov Murik <dovmurik@...ux.ibm.com>
Subject: Re: [PATCH RFC v7 62/64] x86/sev: Add KVM commands for instance certs
Hi Dionna, Mike,
On 14/12/2022 21:40, Michael Roth wrote:
> From: Dionna Glaze <dionnaglaze@...gle.com>
>
> The /dev/sev device has the ability to store host-wide certificates for
> the key used by the AMD-SP for SEV-SNP attestation report signing,
> but for hosts that want to specify additional certificates that are
> specific to the image launched in a VM, a different way is needed to
> communicate those certificates.
>
> This patch adds two new KVM ioctl commands: KVM_SEV_SNP_{GET,SET}_CERTS
>
> The certificates that are set with this command are expected to follow
> the same format as the host certificates, but that format is opaque
> to the kernel.
>
> The new behavior for custom certificates is that the extended guest
> request command will now return the overridden certificates if they
> were installed for the instance. The error condition for a too small
> data buffer is changed to return the overridden certificate data size
> if there is an overridden certificate set installed.
>
> Setting a 0 length certificate returns the system state to only return
> the host certificates on an extended guest request.
>
> We also increase the SEV_FW_BLOB_MAX_SIZE another 4K page to allow
> space for an extra certificate.
>
> Cc: Tom Lendacky <Thomas.Lendacky@....com>
> Cc: Paolo Bonzini <pbonzini@...hat.com>
>
> Signed-off-by: Dionna Glaze <dionnaglaze@...gle.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@....com>
> Signed-off-by: Michael Roth <michael.roth@....com>
> ---
> arch/x86/kvm/svm/sev.c | 111 ++++++++++++++++++++++++++++++++++++++-
> arch/x86/kvm/svm/svm.h | 1 +
> include/linux/psp-sev.h | 2 +-
> include/uapi/linux/kvm.h | 12 +++++
> 4 files changed, 123 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 4de952d1d446..d0e58cffd1ed 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2081,6 +2081,7 @@ static void *snp_context_create(struct kvm *kvm, struct kvm_sev_cmd *argp)
> goto e_free;
>
> sev->snp_certs_data = certs_data;
> + sev->snp_certs_len = 0;
>
> return context;
>
> @@ -2364,6 +2365,86 @@ static int snp_launch_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)
> return ret;
> }
>
> +static int snp_get_instance_certs(struct kvm *kvm, struct kvm_sev_cmd *argp)
> +{
> + struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> + struct kvm_sev_snp_get_certs params;
> +
> + if (!sev_snp_guest(kvm))
> + return -ENOTTY;
> +
> + if (!sev->snp_context)
> + return -EINVAL;
> +
> + if (copy_from_user(¶ms, (void __user *)(uintptr_t)argp->data,
> + sizeof(params)))
> + return -EFAULT;
> +
> + /* No instance certs set. */
> + if (!sev->snp_certs_len)
> + return -ENOENT;
> +
> + if (params.certs_len < sev->snp_certs_len) {
> + /* Output buffer too small. Return the required size. */
> + params.certs_len = sev->snp_certs_len;
> +
> + if (copy_to_user((void __user *)(uintptr_t)argp->data, ¶ms,
> + sizeof(params)))
> + return -EFAULT;
> +
> + return -EINVAL;
> + }
> +
> + if (copy_to_user((void __user *)(uintptr_t)params.certs_uaddr,
> + sev->snp_certs_data, sev->snp_certs_len))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +static int snp_set_instance_certs(struct kvm *kvm, struct kvm_sev_cmd *argp)
> +{
> + struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> + unsigned long length = SEV_FW_BLOB_MAX_SIZE;
> + void *to_certs = sev->snp_certs_data;
> + struct kvm_sev_snp_set_certs params;
> +
> + if (!sev_snp_guest(kvm))
> + return -ENOTTY;
> +
> + if (!sev->snp_context)
> + return -EINVAL;
> +
> + if (copy_from_user(¶ms, (void __user *)(uintptr_t)argp->data,
> + sizeof(params)))
> + return -EFAULT;
> +
> + if (params.certs_len > SEV_FW_BLOB_MAX_SIZE)
> + return -EINVAL;
> +
> + /*
> + * Setting a length of 0 is the same as "uninstalling" instance-
> + * specific certificates.
> + */
> + if (params.certs_len == 0) {
> + sev->snp_certs_len = 0;
> + return 0;
> + }
> +
> + /* Page-align the length */
> + length = (params.certs_len + PAGE_SIZE - 1) & PAGE_MASK;
> +
> + if (copy_from_user(to_certs,
> + (void __user *)(uintptr_t)params.certs_uaddr,
> + params.certs_len)) {
> + return -EFAULT;
> + }
> +
> + sev->snp_certs_len = length;
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?
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.
> +
> + return 0;
> +}
> +
> int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp)
> {
> struct kvm_sev_cmd sev_cmd;
[...]
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index a1e6624540f3..970a9de0ed20 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -22,7 +22,7 @@
> #define __psp_pa(x) __pa(x)
> #endif
>
> -#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?
-Dov
> /**
> * SEV platform state
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 61b1e26ced01..48bcc59cf86b 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1949,6 +1949,8 @@ enum sev_cmd_id {
> KVM_SEV_SNP_LAUNCH_START,
> KVM_SEV_SNP_LAUNCH_UPDATE,
> KVM_SEV_SNP_LAUNCH_FINISH,
> + KVM_SEV_SNP_GET_CERTS,
> + KVM_SEV_SNP_SET_CERTS,
>
> KVM_SEV_NR_MAX,
> };
> @@ -2096,6 +2098,16 @@ struct kvm_sev_snp_launch_finish {
> __u8 pad[6];
> };
>
> +struct kvm_sev_snp_get_certs {
> + __u64 certs_uaddr;
> + __u64 certs_len;
> +};
> +
> +struct kvm_sev_snp_set_certs {
> + __u64 certs_uaddr;
> + __u64 certs_len;
> +};
> +
> #define KVM_DEV_ASSIGN_ENABLE_IOMMU (1 << 0)
> #define KVM_DEV_ASSIGN_PCI_2_3 (1 << 1)
> #define KVM_DEV_ASSIGN_MASK_INTX (1 << 2)
Powered by blists - more mailing lists