| 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
| ||
|
Date: Wed, 12 Oct 2022 19:42:11 -0500
From: "Kalra, Ashish" <ashish.kalra@....com>
To: Dionna Glaze <dionnaglaze@...gle.com>,
linux-kernel@...r.kernel.org, x86@...nel.org
Cc: Tom Lendacky <Thomas.Lendacky@....com>,
Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH 1/2] x86/sev: Add KVM commands for instance certs
On 10/12/2022 6:39 PM, Kalra, Ashish wrote:
> Hello Dionna,
>
> On 9/1/2022 7:04 PM, Dionna Glaze wrote:
>> 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, we need a different way 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>
>> ---
>> 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 a35cd9f33f16..f1d846081213 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -1636,6 +1636,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;
>> @@ -1940,6 +1941,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;
>> + struct kvm_sev_snp_set_certs params;
>> + void *new_certs = NULL, *to_certs = sev->snp_certs_data;
>> + unsigned long length = SEV_FW_BLOB_MAX_SIZE;
>> +
>> + 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;
>
> Probably can use PAGE_ALIGN() here.
>
>> +
>> + if (copy_from_user(to_certs,
>> + (void __user *)(uintptr_t)params.certs_uaddr,
>> + params.certs_len)) {
>> + return -EFAULT;
>> + }
>> +
>> + sev->snp_certs_len = length;
>> +
>> + return 0;
>> +}
>> +
>> int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
>> {
>> struct kvm_sev_cmd sev_cmd;
>> @@ -2038,6 +2119,12 @@ int svm_mem_enc_op(struct kvm *kvm, void __user
>> *argp)
>> case KVM_SEV_SNP_LAUNCH_FINISH:
>> r = snp_launch_finish(kvm, &sev_cmd);
>> break;
>> + case KVM_SEV_SNP_GET_CERTS:
>> + r = snp_get_instance_certs(kvm, &sev_cmd);
>> + break;
>> + case KVM_SEV_SNP_SET_CERTS:
>> + r = snp_set_instance_certs(kvm, &sev_cmd);
>> + break;
>> default:
>> r = -EINVAL;
>> goto out;
>> @@ -3361,8 +3448,28 @@ static void snp_handle_ext_guest_request(struct
>> vcpu_svm *svm, gpa_t req_gpa, gp
>> if (rc)
>> goto unlock;
>> - rc = snp_guest_ext_guest_request(&req, (unsigned
>> long)sev->snp_certs_data,
>> - &data_npages, &err);
>> + /*
>> + * If the VMM has overridden the certs, then we change the error
>> message
>> + * if the size is inappropriate for the override. Otherwise we use a
>> + * regular guest request and copy back the instance certs.
>> + */
>> + if (sev->snp_certs_len) {
>> + if ((data_npages << PAGE_SHIFT) < sev->snp_certs_len) {
>> + rc = -EINVAL;
>> + err = SNP_GUEST_REQ_INVALID_LEN;
>> + goto datalen;
>> + }
>> + rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &req,
>> + (int *)&err);
Though, one thing i don't understand is that why do we need to issue
the SNP_GUEST_REQUEST to FW if we are going to return the VMM
overriden certs back to the guest ?
Thanks,
Ashish
>> + } else {
>> + rc = snp_guest_ext_guest_request(
>> + &req, (unsigned long)sev->snp_certs_data, &data_npages,
>> + &err);
>> + }
>> +datalen:
>> + if (sev->snp_certs_len)
>> + data_npages = sev->snp_certs_len >> PAGE_SHIFT;
>> +
>> if (rc) {
>> /*
>> * If buffer length is small then return the expected
>> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
>> index e68b3aab57d6..9030a295cdf5 100644
>> --- a/arch/x86/kvm/svm/svm.h
>> +++ b/arch/x86/kvm/svm/svm.h
>> @@ -93,6 +93,7 @@ struct kvm_sev_info {
>> void *snp_context; /* SNP guest context page */
>> struct srcu_struct psc_srcu;
>> void *snp_certs_data;
>> + unsigned int snp_certs_len; /* Size of instance override for
>> certs */
>> struct mutex guest_req_lock;
>> u64 sev_features; /* Features set at VMSA creation */
>> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
>> index 859bbbcd5fa3..3d1811ffd9af 100644
>> --- a/include/linux/psp-sev.h
>> +++ b/include/linux/psp-sev.h
>> @@ -24,7 +24,7 @@
>> #define __psp_pa(x) __pa(x)
>> #endif
>> -#define SEV_FW_BLOB_MAX_SIZE 0x4000 /* 16KB */
>> +#define SEV_FW_BLOB_MAX_SIZE 0x5000 /* 20KB */
>> /**
>> * SEV platform state
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index db9eb36da3ec..d47b36dc681d 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -1717,6 +1717,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,
>> };
>> @@ -1864,6 +1866,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)
>>
>
> Otherwise, patch looks good to me.
>
> Reviewed-by: Ashish Kalra <ashish.kalra@....com>
>
> Thanks,
> Ashish
Powered by blists - more mailing lists