Re: [PATCH v6 01/14] KVM: SVM: Add KVM_SEV SEND_START command

[Brijesh Singh]

On 4/2/20 12:51 PM, Krish Sadhukhan wrote:
>
> On 3/29/20 11:19 PM, Ashish Kalra wrote:
>> From: Brijesh Singh <Brijesh.Singh@xxxxxxx>
>>
>> The command is used to create an outgoing SEV guest encryption context.
>>
>> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>> Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
>> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
>> Cc: "Radim KrÄmÃÅ" <rkrcmar@xxxxxxxxxx>
>> Cc: Joerg Roedel <joro@xxxxxxxxxx>
>> Cc: Borislav Petkov <bp@xxxxxxx>
>> Cc: Tom Lendacky <thomas.lendacky@xxxxxxx>
>> Cc: x86@xxxxxxxxxx
>> Cc: kvm@xxxxxxxxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> Reviewed-by: Steve Rutherford <srutherford@xxxxxxxxxx>
>> Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
>> Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
>> ---
>> Â .../virt/kvm/amd-memory-encryption.rstÂÂÂÂÂÂÂ |Â 27 ++++
>> Â arch/x86/kvm/svm.cÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ | 128 ++++++++++++++++++
>> Â include/linux/psp-sev.hÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |ÂÂ 8 +-
>> Â include/uapi/linux/kvm.hÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |Â 12 ++
>> Â 4 files changed, 171 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/virt/kvm/amd-memory-encryption.rst
>> b/Documentation/virt/kvm/amd-memory-encryption.rst
>> index c3129b9ba5cb..4fd34fc5c7a7 100644
>> --- a/Documentation/virt/kvm/amd-memory-encryption.rst
>> +++ b/Documentation/virt/kvm/amd-memory-encryption.rst
>> @@ -263,6 +263,33 @@ Returns: 0 on success, -negative on error
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ __u32 trans_len;
>> ÂÂÂÂÂÂÂÂÂ };
>> Â +10. KVM_SEV_SEND_START
>> +----------------------
>> +
>> +The KVM_SEV_SEND_START command can be used by the hypervisor to
>> create an
>> +outgoing guest encryption context.
> Shouldn't we mention that this command is also used to save the guest
> to the disk ?


No, because this command is not used for saving to the disk.


>> +
>> +Parameters (in): struct kvm_sev_send_start
>> +
>> +Returns: 0 on success, -negative on error
>> +
>> +::
>> +ÂÂÂÂÂÂÂ struct kvm_sev_send_start {
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ __u32 policy;ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ /* guest policy */
>> +
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ __u64 pdh_cert_uaddr;ÂÂÂÂÂÂÂÂ /* platform
>> Diffie-Hellman certificate */
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ __u32 pdh_cert_len;
>> +
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ __u64 plat_certs_uadr;ÂÂÂÂÂÂÂ /* platform
>> certificate chain */
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ __u32 plat_certs_len;
>> +
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ __u64 amd_certs_uaddr;ÂÂÂÂÂÂÂ /* AMD certificate */
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ __u32 amd_cert_len;
>> +
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ __u64 session_uaddr;ÂÂÂÂÂÂÂÂÂ /* Guest session
>> information */
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ __u32 session_len;
>> +ÂÂÂÂÂÂÂ };
>> +
>> Â References
>> Â ==========
>> Â diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 50d1ebafe0b3..63d172e974ad 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -7149,6 +7149,131 @@ static int sev_launch_secret(struct kvm *kvm,
>> struct kvm_sev_cmd *argp)
>> ÂÂÂÂÂ return ret;
>> Â }
>> Â +/* Userspace wants to query session length. */
>> +static int
>> +__sev_send_start_query_session_length(struct kvm *kvm, struct
>> kvm_sev_cmd *argp,
>
>
> __sev_query_send_start_session_length a better name perhaps ?
>
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct kvm_sev_send_start *params)
>> +{
>> +ÂÂÂ struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>> +ÂÂÂ struct sev_data_send_start *data;
>> +ÂÂÂ int ret;
>> +
>> +ÂÂÂ data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
>> +ÂÂÂ if (data == NULL)
>> +ÂÂÂÂÂÂÂ return -ENOMEM;
>> +
>> +ÂÂÂ data->handle = sev->handle;
>> +ÂÂÂ ret = sev_issue_cmd(kvm, SEV_CMD_SEND_START, data, &argp->error);
>
>
> We are not checking ret here as we are assuming that the command will
> always be successful. Is there any chance that sev->handle can be junk
> and should we have an ASSERT for it ?


Both failure and success of this command need to be propagated to the
userspace. In the case of failure the FW may provide additional
information which also need to be propagated to the userspace hence on
failure we should not be asserting.


>
>> +
>> +ÂÂÂ params->session_len = data->session_len;
>> +ÂÂÂ if (copy_to_user((void __user *)(uintptr_t)argp->data, params,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ sizeof(struct kvm_sev_send_start)))
>> +ÂÂÂÂÂÂÂ ret = -EFAULT;
>> +
>> +ÂÂÂ kfree(data);
>> +ÂÂÂ return ret;
>> +}
>> +
>> +static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>
>
> For readability and ease of cscope searches, isn't it better to append
> "svm" to all these functions ?
>
> It seems svm_sev_enabled() is an example of an appropriate naming style.


I don't have strong opinion to prepend or append "svm" to all these
functions, it can be done outside this series. I personally don't see
any strong reason to append svm at this time. The SEV is applicable to
the SVM file only. There is a pending patch which moves all the SEV
stuff in svm/sev.c.


>
>> +{
>> +ÂÂÂ struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>> +ÂÂÂ struct sev_data_send_start *data;
>> +ÂÂÂ struct kvm_sev_send_start params;
>> +ÂÂÂ void *amd_certs, *session_data;
>> +ÂÂÂ void *pdh_cert, *plat_certs;
>> +ÂÂÂ int ret;
>> +
>> +ÂÂÂ if (!sev_guest(kvm))
>> +ÂÂÂÂÂÂÂ return -ENOTTY;
>> +
>> +ÂÂÂ if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ sizeof(struct kvm_sev_send_start)))
>> +ÂÂÂÂÂÂÂ return -EFAULT;
>> +
>> +ÂÂÂ /* if session_len is zero, userspace wants to query the session
>> length */
>> +ÂÂÂ if (!params.session_len)
>> +ÂÂÂÂÂÂÂ return __sev_send_start_query_session_length(kvm, argp,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &params);
>> +
>> +ÂÂÂ /* some sanity checks */
>> +ÂÂÂ if (!params.pdh_cert_uaddr || !params.pdh_cert_len ||
>> +ÂÂÂÂÂÂÂ !params.session_uaddr || params.session_len >
>> SEV_FW_BLOB_MAX_SIZE)
>> +ÂÂÂÂÂÂÂ return -EINVAL;
>
>
> What if params.plat_certs_uaddr or params.amd_certs_uaddr is NULL ?
>
>> +
>> +ÂÂÂ /* allocate the memory to hold the session data blob */
>> +ÂÂÂ session_data = kmalloc(params.session_len, GFP_KERNEL_ACCOUNT);
>> +ÂÂÂ if (!session_data)
>> +ÂÂÂÂÂÂÂ return -ENOMEM;
>> +
>> +ÂÂÂ /* copy the certificate blobs from userspace */
>
>
> You haven't added comments for plat_cert and amd_cert. Also, it's much
> more readable if you add block comments like,
>
> ÂÂÂ ÂÂÂ /*
>
> ÂÂÂÂÂÂÂÂ *Â PDH cert
>
> ÂÂÂ ÂÂÂÂ */
>
>> +ÂÂÂ pdh_cert = psp_copy_user_blob(params.pdh_cert_uaddr,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ params.pdh_cert_len);
>> +ÂÂÂ if (IS_ERR(pdh_cert)) {
>> +ÂÂÂÂÂÂÂ ret = PTR_ERR(pdh_cert);
>> +ÂÂÂÂÂÂÂ goto e_free_session;
>> +ÂÂÂ }
>> +
>> +ÂÂÂ plat_certs = psp_copy_user_blob(params.plat_certs_uaddr,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ params.plat_certs_len);
>> +ÂÂÂ if (IS_ERR(plat_certs)) {
>> +ÂÂÂÂÂÂÂ ret = PTR_ERR(plat_certs);
>> +ÂÂÂÂÂÂÂ goto e_free_pdh;
>> +ÂÂÂ }
>> +
>> +ÂÂÂ amd_certs = psp_copy_user_blob(params.amd_certs_uaddr,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ params.amd_certs_len);
>> +ÂÂÂ if (IS_ERR(amd_certs)) {
>> +ÂÂÂÂÂÂÂ ret = PTR_ERR(amd_certs);
>> +ÂÂÂÂÂÂÂ goto e_free_plat_cert;
>> +ÂÂÂ }
>> +
>> +ÂÂÂ data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
>> +ÂÂÂ if (data == NULL) {
>> +ÂÂÂÂÂÂÂ ret = -ENOMEM;
>> +ÂÂÂÂÂÂÂ goto e_free_amd_cert;
>> +ÂÂÂ }
>> +
>> +ÂÂÂ /* populate the FW SEND_START field with system physical address */
>> +ÂÂÂ data->pdh_cert_address = __psp_pa(pdh_cert);
>> +ÂÂÂ data->pdh_cert_len = params.pdh_cert_len;
>> +ÂÂÂ data->plat_certs_address = __psp_pa(plat_certs);
>> +ÂÂÂ data->plat_certs_len = params.plat_certs_len;
>> +ÂÂÂ data->amd_certs_address = __psp_pa(amd_certs);
>> +ÂÂÂ data->amd_certs_len = params.amd_certs_len;
>> +ÂÂÂ data->session_address = __psp_pa(session_data);
>> +ÂÂÂ data->session_len = params.session_len;
>> +ÂÂÂ data->handle = sev->handle;
>> +
>> +ÂÂÂ ret = sev_issue_cmd(kvm, SEV_CMD_SEND_START, data, &argp->error);
>> +
>> +ÂÂÂ if (ret)
>> +ÂÂÂÂÂÂÂ goto e_free;
>> +
>> +ÂÂÂ if (copy_to_user((void __user *)(uintptr_t) params.session_uaddr,
>> +ÂÂÂÂÂÂÂÂÂÂÂ session_data, params.session_len)) {
>> +ÂÂÂÂÂÂÂ ret = -EFAULT;
>> +ÂÂÂÂÂÂÂ goto e_free;
>> +ÂÂÂ }
>> +
>> +ÂÂÂ params.policy = data->policy;
>> +ÂÂÂ params.session_len = data->session_len;
>> +ÂÂÂ if (copy_to_user((void __user *)(uintptr_t)argp->data, &params,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ sizeof(struct kvm_sev_send_start)))
>> +ÂÂÂÂÂÂÂ ret = -EFAULT;
>> +
>> +e_free:
>> +ÂÂÂ kfree(data);
>> +e_free_amd_cert:
>> +ÂÂÂ kfree(amd_certs);
>> +e_free_plat_cert:
>> +ÂÂÂ kfree(plat_certs);
>> +e_free_pdh:
>> +ÂÂÂ kfree(pdh_cert);
>> +e_free_session:
>> +ÂÂÂ kfree(session_data);
>> +ÂÂÂ return ret;
>> +}
>> +
>> Â static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
>> Â {
>> ÂÂÂÂÂ struct kvm_sev_cmd sev_cmd;
>> @@ -7193,6 +7318,9 @@ static int svm_mem_enc_op(struct kvm *kvm, void
>> __user *argp)
>> ÂÂÂÂÂ case KVM_SEV_LAUNCH_SECRET:
>> ÂÂÂÂÂÂÂÂÂ r = sev_launch_secret(kvm, &sev_cmd);
>> ÂÂÂÂÂÂÂÂÂ break;
>> +ÂÂÂ case KVM_SEV_SEND_START:
>> +ÂÂÂÂÂÂÂ r = sev_send_start(kvm, &sev_cmd);
>> +ÂÂÂÂÂÂÂ break;
>> ÂÂÂÂÂ default:
>> ÂÂÂÂÂÂÂÂÂ r = -EINVAL;
>> ÂÂÂÂÂÂÂÂÂ goto out;
>> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
>> index 5167bf2bfc75..9f63b9d48b63 100644
>> --- a/include/linux/psp-sev.h
>> +++ b/include/linux/psp-sev.h
>> @@ -323,11 +323,11 @@ struct sev_data_send_start {
>> ÂÂÂÂÂ u64 pdh_cert_address;ÂÂÂÂÂÂÂÂÂÂÂ /* In */
>> ÂÂÂÂÂ u32 pdh_cert_len;ÂÂÂÂÂÂÂÂÂÂÂ /* In */
>> ÂÂÂÂÂ u32 reserved1;
>> -ÂÂÂ u64 plat_cert_address;ÂÂÂÂÂÂÂÂÂÂÂ /* In */
>> -ÂÂÂ u32 plat_cert_len;ÂÂÂÂÂÂÂÂÂÂÂ /* In */
>> +ÂÂÂ u64 plat_certs_address;ÂÂÂÂÂÂÂÂÂÂÂ /* In */
>> +ÂÂÂ u32 plat_certs_len;ÂÂÂÂÂÂÂÂÂÂÂ /* In */
>
>
> It seems that the 'platform certificate' and the 'amd_certificate' are
> single entities, meaning only copy is there for the particular
> platform and particular the AMD product. Why are these plural then ?
>
>
>> ÂÂÂÂÂ u32 reserved2;
>> -ÂÂÂ u64 amd_cert_address;ÂÂÂÂÂÂÂÂÂÂÂ /* In */
>> -ÂÂÂ u32 amd_cert_len;ÂÂÂÂÂÂÂÂÂÂÂ /* In */
>> +ÂÂÂ u64 amd_certs_address;ÂÂÂÂÂÂÂÂÂÂÂ /* In */
>> +ÂÂÂ u32 amd_certs_len;ÂÂÂÂÂÂÂÂÂÂÂ /* In */
>> ÂÂÂÂÂ u32 reserved3;
>> ÂÂÂÂÂ u64 session_address;ÂÂÂÂÂÂÂÂÂÂÂ /* In */
>> ÂÂÂÂÂ u32 session_len;ÂÂÂÂÂÂÂÂÂÂÂ /* In/Out */
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index 4b95f9a31a2f..17bef4c245e1 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -1558,6 +1558,18 @@ struct kvm_sev_dbg {
>> ÂÂÂÂÂ __u32 len;
>> Â };
>> Â +struct kvm_sev_send_start {
>> +ÂÂÂ __u32 policy;
>> +ÂÂÂ __u64 pdh_cert_uaddr;
>> +ÂÂÂ __u32 pdh_cert_len;
>> +ÂÂÂ __u64 plat_certs_uaddr;
>> +ÂÂÂ __u32 plat_certs_len;
>> +ÂÂÂ __u64 amd_certs_uaddr;
>> +ÂÂÂ __u32 amd_certs_len;
>> +ÂÂÂ __u64 session_uaddr;
>> +ÂÂÂ __u32 session_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)