Message-ID: <478c4bb3-1727-d80f-dc07-c0f6350b2da5@amd.com>
Date:   Thu, 2 Apr 2020 13:38:42 -0500
From:   Brijesh Singh <brijesh.singh@....com>
To:     Krish Sadhukhan <krish.sadhukhan@...cle.com>,
        Ashish Kalra <Ashish.Kalra@....com>, pbonzini@...hat.com
Cc:     brijesh.singh@....com, tglx@...utronix.de, mingo@...hat.com,
        hpa@...or.com, joro@...tes.org, bp@...e.de,
        thomas.lendacky@....com, x86@...nel.org, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, rientjes@...gle.com,
        srutherford@...gle.com, luto@...nel.org
Subject: Re: [PATCH v6 01/14] KVM: SVM: Add KVM_SEV SEND_START command


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@....com>
>>
>> The command is used to create an outgoing SEV guest encryption context.
>>
>> Cc: Thomas Gleixner <tglx@...utronix.de>
>> Cc: Ingo Molnar <mingo@...hat.com>
>> Cc: "H. Peter Anvin" <hpa@...or.com>
>> Cc: Paolo Bonzini <pbonzini@...hat.com>
>> Cc: "Radim Krčmář" <rkrcmar@...hat.com>
>> Cc: Joerg Roedel <joro@...tes.org>
>> Cc: Borislav Petkov <bp@...e.de>
>> Cc: Tom Lendacky <thomas.lendacky@....com>
>> Cc: x86@...nel.org
>> Cc: kvm@...r.kernel.org
>> Cc: linux-kernel@...r.kernel.org
>> Reviewed-by: Steve Rutherford <srutherford@...gle.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh@....com>
>> Signed-off-by: Ashish Kalra <ashish.kalra@....com>
>> ---
>>   .../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)

Powered by blists - more mailing lists