[RFC,v7,52/64] KVM: SVM: Provide support for SNP_GUEST_REQUEST NAE event

Commit Message

Michael Roth Dec. 14, 2022, 7:40 p.m. UTC

From: Brijesh Singh <[e-mail]>

Version 2 of GHCB specification added the support for two SNP Guest
Request Message NAE events. The events allows for an SEV-SNP guest to
make request to the SEV-SNP firmware through hypervisor using the
SNP_GUEST_REQUEST API define in the SEV-SNP firmware specification.

The SNP_EXT_GUEST_REQUEST is similar to SNP_GUEST_REQUEST with the
difference of an additional certificate blob that can be passed through
the SNP_SET_CONFIG ioctl defined in the CCP driver. The CCP driver
provides snp_guest_ext_guest_request() that is used by the KVM to get
both the report and certificate data at once.

Signed-off-by: Brijesh Singh <[e-mail]>
Signed-off-by: Ashish Kalra <[e-mail]>
Signed-off-by: Michael Roth <[e-mail]>
---
 arch/x86/kvm/svm/sev.c | 185 +++++++++++++++++++++++++++++++++++++++--
 arch/x86/kvm/svm/svm.h |   2 +
 2 files changed, 181 insertions(+), 6 deletions(-)

Comments

Alexey Kardashevskiy Jan. 9, 2023, 3:33 a.m. UTC | #1

On 15/12/22 06:40, Michael Roth wrote:
> From: Brijesh Singh <[e-mail]>
> 
> Version 2 of GHCB specification added the support for two SNP Guest
> Request Message NAE events. The events allows for an SEV-SNP guest to
> make request to the SEV-SNP firmware through hypervisor using the
> SNP_GUEST_REQUEST API define in the SEV-SNP firmware specification.
> 
> The SNP_EXT_GUEST_REQUEST is similar to SNP_GUEST_REQUEST with the
> difference of an additional certificate blob that can be passed through
> the SNP_SET_CONFIG ioctl defined in the CCP driver. The CCP driver
> provides snp_guest_ext_guest_request() that is used by the KVM to get
> both the report and certificate data at once.
> 
> Signed-off-by: Brijesh Singh <[e-mail]>
> Signed-off-by: Ashish Kalra <[e-mail]>
> Signed-off-by: Michael Roth <[e-mail]>
> ---
>   arch/x86/kvm/svm/sev.c | 185 +++++++++++++++++++++++++++++++++++++++--
>   arch/x86/kvm/svm/svm.h |   2 +
>   2 files changed, 181 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 5f2b2092cdae..18efa70553c2 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -331,6 +331,7 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   		if (ret)
>   			goto e_free;
>   
> +		mutex_init(&sev->guest_req_lock);
>   		ret = sev_snp_init(&argp->error, false);
>   	} else {
>   		ret = sev_platform_init(&argp->error);
> @@ -2051,23 +2052,34 @@ int sev_vm_move_enc_context_from(struct kvm *kvm, unsigned int source_fd)
>    */
>   static void *snp_context_create(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   {
> +	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>   	struct sev_data_snp_addr data = {};
> -	void *context;
> +	void *context, *certs_data;
>   	int rc;
>   
> +	/* Allocate memory used for the certs data in SNP guest request */
> +	certs_data = kzalloc(SEV_FW_BLOB_MAX_SIZE, GFP_KERNEL_ACCOUNT);
> +	if (!certs_data)
> +		return NULL;
> +
>   	/* Allocate memory for context page */
>   	context = snp_alloc_firmware_page(GFP_KERNEL_ACCOUNT);
>   	if (!context)
> -		return NULL;
> +		goto e_free;
>   
>   	data.gctx_paddr = __psp_pa(context);
>   	rc = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_GCTX_CREATE, &data, &argp->error);
> -	if (rc) {
> -		snp_free_firmware_page(context);
> -		return NULL;
> -	}
> +	if (rc)
> +		goto e_free;
> +
> +	sev->snp_certs_data = certs_data;
>   
>   	return context;
> +
> +e_free:
> +	snp_free_firmware_page(context);
> +	kfree(certs_data);
> +	return NULL;
>   }
>   
>   static int snp_bind_asid(struct kvm *kvm, int *error)
> @@ -2653,6 +2665,8 @@ static int snp_decommission_context(struct kvm *kvm)
>   	snp_free_firmware_page(sev->snp_context);
>   	sev->snp_context = NULL;
>   
> +	kfree(sev->snp_certs_data);
> +
>   	return 0;
>   }
>   
> @@ -3174,6 +3188,8 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm, u64 *exit_code)
>   	case SVM_VMGEXIT_UNSUPPORTED_EVENT:
>   	case SVM_VMGEXIT_HV_FEATURES:
>   	case SVM_VMGEXIT_PSC:
> +	case SVM_VMGEXIT_GUEST_REQUEST:
> +	case SVM_VMGEXIT_EXT_GUEST_REQUEST:
>   		break;
>   	default:
>   		reason = GHCB_ERR_INVALID_EVENT;
> @@ -3396,6 +3412,149 @@ static int snp_complete_psc(struct kvm_vcpu *vcpu)
>   	return 1;
>   }
>   
> +static unsigned long snp_setup_guest_buf(struct vcpu_svm *svm,
> +					 struct sev_data_snp_guest_request *data,
> +					 gpa_t req_gpa, gpa_t resp_gpa)
> +{
> +	struct kvm_vcpu *vcpu = &svm->vcpu;
> +	struct kvm *kvm = vcpu->kvm;
> +	kvm_pfn_t req_pfn, resp_pfn;
> +	struct kvm_sev_info *sev;
> +
> +	sev = &to_kvm_svm(kvm)->sev_info;
> +
> +	if (!IS_ALIGNED(req_gpa, PAGE_SIZE) || !IS_ALIGNED(resp_gpa, PAGE_SIZE))
> +		return SEV_RET_INVALID_PARAM;
> +
> +	req_pfn = gfn_to_pfn(kvm, gpa_to_gfn(req_gpa));
> +	if (is_error_noslot_pfn(req_pfn))
> +		return SEV_RET_INVALID_ADDRESS;
> +
> +	resp_pfn = gfn_to_pfn(kvm, gpa_to_gfn(resp_gpa));
> +	if (is_error_noslot_pfn(resp_pfn))
> +		return SEV_RET_INVALID_ADDRESS;
> +
> +	if (rmp_make_private(resp_pfn, 0, PG_LEVEL_4K, 0, true))
> +		return SEV_RET_INVALID_ADDRESS;
> +
> +	data->gctx_paddr = __psp_pa(sev->snp_context);
> +	data->req_paddr = __sme_set(req_pfn << PAGE_SHIFT);
> +	data->res_paddr = __sme_set(resp_pfn << PAGE_SHIFT);
> +
> +	return 0;
> +}
> +
> +static void snp_cleanup_guest_buf(struct sev_data_snp_guest_request *data, unsigned long *rc)
> +{
> +	u64 pfn = __sme_clr(data->res_paddr) >> PAGE_SHIFT;
> +	int ret;
> +
> +	ret = snp_page_reclaim(pfn);
> +	if (ret)
> +		*rc = SEV_RET_INVALID_ADDRESS;
> +
> +	ret = rmp_make_shared(pfn, PG_LEVEL_4K);
> +	if (ret)
> +		*rc = SEV_RET_INVALID_ADDRESS;
> +}
> +
> +static void snp_handle_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp_gpa)
> +{
> +	struct sev_data_snp_guest_request data = {0};
> +	struct kvm_vcpu *vcpu = &svm->vcpu;
> +	struct kvm *kvm = vcpu->kvm;
> +	struct kvm_sev_info *sev;
> +	unsigned long rc;
> +	int err;
> +
> +	if (!sev_snp_guest(vcpu->kvm)) {
> +		rc = SEV_RET_INVALID_GUEST;
> +		goto e_fail;
> +	}
> +
> +	sev = &to_kvm_svm(kvm)->sev_info;
> +
> +	mutex_lock(&sev->guest_req_lock);
> +
> +	rc = snp_setup_guest_buf(svm, &data, req_gpa, resp_gpa);
> +	if (rc)
> +		goto unlock;
> +
> +	rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, &err);


This one goes via sev_issue_cmd_external_user() and uses sev-fd...

> +	if (rc)
> +		/* use the firmware error code */
> +		rc = err;
> +
> +	snp_cleanup_guest_buf(&data, &rc);
> +
> +unlock:
> +	mutex_unlock(&sev->guest_req_lock);
> +
> +e_fail:
> +	svm_set_ghcb_sw_exit_info_2(vcpu, rc);
> +}
> +
> +static void snp_handle_ext_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp_gpa)
> +{
> +	struct sev_data_snp_guest_request req = {0};
> +	struct kvm_vcpu *vcpu = &svm->vcpu;
> +	struct kvm *kvm = vcpu->kvm;
> +	unsigned long data_npages;
> +	struct kvm_sev_info *sev;
> +	unsigned long rc, err;
> +	u64 data_gpa;
> +
> +	if (!sev_snp_guest(vcpu->kvm)) {
> +		rc = SEV_RET_INVALID_GUEST;
> +		goto e_fail;
> +	}
> +
> +	sev = &to_kvm_svm(kvm)->sev_info;
> +
> +	data_gpa = vcpu->arch.regs[VCPU_REGS_RAX];
> +	data_npages = vcpu->arch.regs[VCPU_REGS_RBX];
> +
> +	if (!IS_ALIGNED(data_gpa, PAGE_SIZE)) {
> +		rc = SEV_RET_INVALID_ADDRESS;
> +		goto e_fail;
> +	}
> +
> +	mutex_lock(&sev->guest_req_lock);
> +
> +	rc = snp_setup_guest_buf(svm, &req, req_gpa, resp_gpa);
> +	if (rc)
> +		goto unlock;
> +
> +	rc = snp_guest_ext_guest_request(&req, (unsigned long)sev->snp_certs_data,
> +					 &data_npages, &err);

but this one does not and jump straight to drivers/crypto/ccp/sev-dev.c 
ignoring sev->fd. Why different? Can these two be unified? 
sev_issue_cmd_external_user() only checks if fd is /dev/sev which is 
hardly useful.

"[PATCH RFC v7 32/64] crypto: ccp: Provide APIs to query extended 
attestation report" added this one.

Besides, is sev->fd really needed in the sev struct at all? Thanks,


> +	if (rc) {
> +		/*
> +		 * If buffer length is small then return the expected
> +		 * length in rbx.
> +		 */
> +		if (err == SNP_GUEST_REQ_INVALID_LEN)
> +			vcpu->arch.regs[VCPU_REGS_RBX] = data_npages;
> +
> +		/* pass the firmware error code */
> +		rc = err;
> +		goto cleanup;
> +	}
> +
> +	/* Copy the certificate blob in the guest memory */
> +	if (data_npages &&
> +	    kvm_write_guest(kvm, data_gpa, sev->snp_certs_data, data_npages << PAGE_SHIFT))
> +		rc = SEV_RET_INVALID_ADDRESS;
> +
> +cleanup:
> +	snp_cleanup_guest_buf(&req, &rc);
> +
> +unlock:
> +	mutex_unlock(&sev->guest_req_lock);
> +
> +e_fail:
> +	svm_set_ghcb_sw_exit_info_2(vcpu, rc);
> +}
> +
>   static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
>   {
>   	struct vmcb_control_area *control = &svm->vmcb->control;
> @@ -3629,6 +3788,20 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
>   		vcpu->run->vmgexit.ghcb_msr = ghcb_gpa;
>   		vcpu->arch.complete_userspace_io = snp_complete_psc;
>   		break;
> +	case SVM_VMGEXIT_GUEST_REQUEST: {
> +		snp_handle_guest_request(svm, control->exit_info_1, control->exit_info_2);
> +
> +		ret = 1;
> +		break;
> +	}
> +	case SVM_VMGEXIT_EXT_GUEST_REQUEST: {
> +		snp_handle_ext_guest_request(svm,
> +					     control->exit_info_1,
> +					     control->exit_info_2);
> +
> +		ret = 1;
> +		break;
> +	}
>   	case SVM_VMGEXIT_UNSUPPORTED_EVENT:
>   		vcpu_unimpl(vcpu,
>   			    "vmgexit: unsupported event - exit_info_1=%#llx, exit_info_2=%#llx\n",
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 12b9f4d539fb..7c0f9d00950f 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -101,6 +101,8 @@ struct kvm_sev_info {
>   	u64 snp_init_flags;
>   	void *snp_context;      /* SNP guest context page */
>   	spinlock_t psc_lock;
> +	void *snp_certs_data;
> +	struct mutex guest_req_lock;
>   };
>   
>   struct kvm_svm {

Kalra, Ashish Jan. 9, 2023, 11:41 p.m. UTC | #2

On 1/8/2023 9:33 PM, Alexey Kardashevskiy wrote:
> On 15/12/22 06:40, Michael Roth wrote:
>> From: Brijesh Singh <[e-mail]>
>>
>> Version 2 of GHCB specification added the support for two SNP Guest
>> Request Message NAE events. The events allows for an SEV-SNP guest to
>> make request to the SEV-SNP firmware through hypervisor using the
>> SNP_GUEST_REQUEST API define in the SEV-SNP firmware specification.
>>
>> The SNP_EXT_GUEST_REQUEST is similar to SNP_GUEST_REQUEST with the
>> difference of an additional certificate blob that can be passed through
>> the SNP_SET_CONFIG ioctl defined in the CCP driver. The CCP driver
>> provides snp_guest_ext_guest_request() that is used by the KVM to get
>> both the report and certificate data at once.
>>
>> Signed-off-by: Brijesh Singh <[e-mail]>
>> Signed-off-by: Ashish Kalra <[e-mail]>
>> Signed-off-by: Michael Roth <[e-mail]>
>> ---
>>   arch/x86/kvm/svm/sev.c | 185 +++++++++++++++++++++++++++++++++++++++--
>>   arch/x86/kvm/svm/svm.h |   2 +
>>   2 files changed, 181 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index 5f2b2092cdae..18efa70553c2 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -331,6 +331,7 @@ static int sev_guest_init(struct kvm *kvm, struct 
>> kvm_sev_cmd *argp)
>>           if (ret)
>>               goto e_free;
>> +        mutex_init(&sev->guest_req_lock);
>>           ret = sev_snp_init(&argp->error, false);
>>       } else {
>>           ret = sev_platform_init(&argp->error);
>> @@ -2051,23 +2052,34 @@ int sev_vm_move_enc_context_from(struct kvm 
>> *kvm, unsigned int source_fd)
>>    */
>>   static void *snp_context_create(struct kvm *kvm, struct kvm_sev_cmd 
>> *argp)
>>   {
>> +    struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>>       struct sev_data_snp_addr data = {};
>> -    void *context;
>> +    void *context, *certs_data;
>>       int rc;
>> +    /* Allocate memory used for the certs data in SNP guest request */
>> +    certs_data = kzalloc(SEV_FW_BLOB_MAX_SIZE, GFP_KERNEL_ACCOUNT);
>> +    if (!certs_data)
>> +        return NULL;
>> +
>>       /* Allocate memory for context page */
>>       context = snp_alloc_firmware_page(GFP_KERNEL_ACCOUNT);
>>       if (!context)
>> -        return NULL;
>> +        goto e_free;
>>       data.gctx_paddr = __psp_pa(context);
>>       rc = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_GCTX_CREATE, 
>> &data, &argp->error);
>> -    if (rc) {
>> -        snp_free_firmware_page(context);
>> -        return NULL;
>> -    }
>> +    if (rc)
>> +        goto e_free;
>> +
>> +    sev->snp_certs_data = certs_data;
>>       return context;
>> +
>> +e_free:
>> +    snp_free_firmware_page(context);
>> +    kfree(certs_data);
>> +    return NULL;
>>   }
>>   static int snp_bind_asid(struct kvm *kvm, int *error)
>> @@ -2653,6 +2665,8 @@ static int snp_decommission_context(struct kvm 
>> *kvm)
>>       snp_free_firmware_page(sev->snp_context);
>>       sev->snp_context = NULL;
>> +    kfree(sev->snp_certs_data);
>> +
>>       return 0;
>>   }
>> @@ -3174,6 +3188,8 @@ static int sev_es_validate_vmgexit(struct 
>> vcpu_svm *svm, u64 *exit_code)
>>       case SVM_VMGEXIT_UNSUPPORTED_EVENT:
>>       case SVM_VMGEXIT_HV_FEATURES:
>>       case SVM_VMGEXIT_PSC:
>> +    case SVM_VMGEXIT_GUEST_REQUEST:
>> +    case SVM_VMGEXIT_EXT_GUEST_REQUEST:
>>           break;
>>       default:
>>           reason = GHCB_ERR_INVALID_EVENT;
>> @@ -3396,6 +3412,149 @@ static int snp_complete_psc(struct kvm_vcpu 
>> *vcpu)
>>       return 1;
>>   }
>> +static unsigned long snp_setup_guest_buf(struct vcpu_svm *svm,
>> +                     struct sev_data_snp_guest_request *data,
>> +                     gpa_t req_gpa, gpa_t resp_gpa)
>> +{
>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>> +    struct kvm *kvm = vcpu->kvm;
>> +    kvm_pfn_t req_pfn, resp_pfn;
>> +    struct kvm_sev_info *sev;
>> +
>> +    sev = &to_kvm_svm(kvm)->sev_info;
>> +
>> +    if (!IS_ALIGNED(req_gpa, PAGE_SIZE) || !IS_ALIGNED(resp_gpa, 
>> PAGE_SIZE))
>> +        return SEV_RET_INVALID_PARAM;
>> +
>> +    req_pfn = gfn_to_pfn(kvm, gpa_to_gfn(req_gpa));
>> +    if (is_error_noslot_pfn(req_pfn))
>> +        return SEV_RET_INVALID_ADDRESS;
>> +
>> +    resp_pfn = gfn_to_pfn(kvm, gpa_to_gfn(resp_gpa));
>> +    if (is_error_noslot_pfn(resp_pfn))
>> +        return SEV_RET_INVALID_ADDRESS;
>> +
>> +    if (rmp_make_private(resp_pfn, 0, PG_LEVEL_4K, 0, true))
>> +        return SEV_RET_INVALID_ADDRESS;
>> +
>> +    data->gctx_paddr = __psp_pa(sev->snp_context);
>> +    data->req_paddr = __sme_set(req_pfn << PAGE_SHIFT);
>> +    data->res_paddr = __sme_set(resp_pfn << PAGE_SHIFT);
>> +
>> +    return 0;
>> +}
>> +
>> +static void snp_cleanup_guest_buf(struct sev_data_snp_guest_request 
>> *data, unsigned long *rc)
>> +{
>> +    u64 pfn = __sme_clr(data->res_paddr) >> PAGE_SHIFT;
>> +    int ret;
>> +
>> +    ret = snp_page_reclaim(pfn);
>> +    if (ret)
>> +        *rc = SEV_RET_INVALID_ADDRESS;
>> +
>> +    ret = rmp_make_shared(pfn, PG_LEVEL_4K);
>> +    if (ret)
>> +        *rc = SEV_RET_INVALID_ADDRESS;
>> +}
>> +
>> +static void snp_handle_guest_request(struct vcpu_svm *svm, gpa_t 
>> req_gpa, gpa_t resp_gpa)
>> +{
>> +    struct sev_data_snp_guest_request data = {0};
>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>> +    struct kvm *kvm = vcpu->kvm;
>> +    struct kvm_sev_info *sev;
>> +    unsigned long rc;
>> +    int err;
>> +
>> +    if (!sev_snp_guest(vcpu->kvm)) {
>> +        rc = SEV_RET_INVALID_GUEST;
>> +        goto e_fail;
>> +    }
>> +
>> +    sev = &to_kvm_svm(kvm)->sev_info;
>> +
>> +    mutex_lock(&sev->guest_req_lock);
>> +
>> +    rc = snp_setup_guest_buf(svm, &data, req_gpa, resp_gpa);
>> +    if (rc)
>> +        goto unlock;
>> +
>> +    rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, &err);
> 
> 
> This one goes via sev_issue_cmd_external_user() and uses sev-fd...
> 
>> +    if (rc)
>> +        /* use the firmware error code */
>> +        rc = err;
>> +
>> +    snp_cleanup_guest_buf(&data, &rc);
>> +
>> +unlock:
>> +    mutex_unlock(&sev->guest_req_lock);
>> +
>> +e_fail:
>> +    svm_set_ghcb_sw_exit_info_2(vcpu, rc);
>> +}
>> +
>> +static void snp_handle_ext_guest_request(struct vcpu_svm *svm, gpa_t 
>> req_gpa, gpa_t resp_gpa)
>> +{
>> +    struct sev_data_snp_guest_request req = {0};
>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>> +    struct kvm *kvm = vcpu->kvm;
>> +    unsigned long data_npages;
>> +    struct kvm_sev_info *sev;
>> +    unsigned long rc, err;
>> +    u64 data_gpa;
>> +
>> +    if (!sev_snp_guest(vcpu->kvm)) {
>> +        rc = SEV_RET_INVALID_GUEST;
>> +        goto e_fail;
>> +    }
>> +
>> +    sev = &to_kvm_svm(kvm)->sev_info;
>> +
>> +    data_gpa = vcpu->arch.regs[VCPU_REGS_RAX];
>> +    data_npages = vcpu->arch.regs[VCPU_REGS_RBX];
>> +
>> +    if (!IS_ALIGNED(data_gpa, PAGE_SIZE)) {
>> +        rc = SEV_RET_INVALID_ADDRESS;
>> +        goto e_fail;
>> +    }
>> +
>> +    mutex_lock(&sev->guest_req_lock);
>> +
>> +    rc = snp_setup_guest_buf(svm, &req, req_gpa, resp_gpa);
>> +    if (rc)
>> +        goto unlock;
>> +
>> +    rc = snp_guest_ext_guest_request(&req, (unsigned 
>> long)sev->snp_certs_data,
>> +                     &data_npages, &err);
> 
> but this one does not and jump straight to drivers/crypto/ccp/sev-dev.c 
> ignoring sev->fd. Why different? Can these two be unified? 
> sev_issue_cmd_external_user() only checks if fd is /dev/sev which is 
> hardly useful.
> 
> "[PATCH RFC v7 32/64] crypto: ccp: Provide APIs to query extended 
> attestation report" added this one.

SNP_EXT_GUEST_REQUEST additionally returns a certificate blob and that's 
why it goes through the CCP driver interface 
snp_guest_ext_guest_request() that is used to get both the report and 
certificate data/blob at the same time.

All the FW API calls on the KVM side go through sev_issue_cmd() and 
sev_issue_cmd_external_user() interfaces and that i believe uses sev->fd 
more of as a sanity check.

Thanks,
Ashish

> 
> Besides, is sev->fd really needed in the sev struct at all? Thanks,
> 
> 
>> +    if (rc) {
>> +        /*
>> +         * If buffer length is small then return the expected
>> +         * length in rbx.
>> +         */
>> +        if (err == SNP_GUEST_REQ_INVALID_LEN)
>> +            vcpu->arch.regs[VCPU_REGS_RBX] = data_npages;
>> +
>> +        /* pass the firmware error code */
>> +        rc = err;
>> +        goto cleanup;
>> +    }
>> +
>> +    /* Copy the certificate blob in the guest memory */
>> +    if (data_npages &&
>> +        kvm_write_guest(kvm, data_gpa, sev->snp_certs_data, 
>> data_npages << PAGE_SHIFT))
>> +        rc = SEV_RET_INVALID_ADDRESS;
>> +
>> +cleanup:
>> +    snp_cleanup_guest_buf(&req, &rc);
>> +
>> +unlock:
>> +    mutex_unlock(&sev->guest_req_lock);
>> +
>> +e_fail:
>> +    svm_set_ghcb_sw_exit_info_2(vcpu, rc);
>> +}
>> +
>>   static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
>>   {
>>       struct vmcb_control_area *control = &svm->vmcb->control;
>> @@ -3629,6 +3788,20 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
>>           vcpu->run->vmgexit.ghcb_msr = ghcb_gpa;
>>           vcpu->arch.complete_userspace_io = snp_complete_psc;
>>           break;
>> +    case SVM_VMGEXIT_GUEST_REQUEST: {
>> +        snp_handle_guest_request(svm, control->exit_info_1, 
>> control->exit_info_2);
>> +
>> +        ret = 1;
>> +        break;
>> +    }
>> +    case SVM_VMGEXIT_EXT_GUEST_REQUEST: {
>> +        snp_handle_ext_guest_request(svm,
>> +                         control->exit_info_1,
>> +                         control->exit_info_2);
>> +
>> +        ret = 1;
>> +        break;
>> +    }
>>       case SVM_VMGEXIT_UNSUPPORTED_EVENT:
>>           vcpu_unimpl(vcpu,
>>                   "vmgexit: unsupported event - exit_info_1=%#llx, 
>> exit_info_2=%#llx\n",
>> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
>> index 12b9f4d539fb..7c0f9d00950f 100644
>> --- a/arch/x86/kvm/svm/svm.h
>> +++ b/arch/x86/kvm/svm/svm.h
>> @@ -101,6 +101,8 @@ struct kvm_sev_info {
>>       u64 snp_init_flags;
>>       void *snp_context;      /* SNP guest context page */
>>       spinlock_t psc_lock;
>> +    void *snp_certs_data;
>> +    struct mutex guest_req_lock;
>>   };
>>   struct kvm_svm {
>

Alexey Kardashevskiy Jan. 10, 2023, 2:28 a.m. UTC | #3

On 10/1/23 10:41, Kalra, Ashish wrote:
> On 1/8/2023 9:33 PM, Alexey Kardashevskiy wrote:
>> On 15/12/22 06:40, Michael Roth wrote:
>>> From: Brijesh Singh <[e-mail]>
>>>
>>> Version 2 of GHCB specification added the support for two SNP Guest
>>> Request Message NAE events. The events allows for an SEV-SNP guest to
>>> make request to the SEV-SNP firmware through hypervisor using the
>>> SNP_GUEST_REQUEST API define in the SEV-SNP firmware specification.
>>>
>>> The SNP_EXT_GUEST_REQUEST is similar to SNP_GUEST_REQUEST with the
>>> difference of an additional certificate blob that can be passed through
>>> the SNP_SET_CONFIG ioctl defined in the CCP driver. The CCP driver
>>> provides snp_guest_ext_guest_request() that is used by the KVM to get
>>> both the report and certificate data at once.
>>>
>>> Signed-off-by: Brijesh Singh <[e-mail]>
>>> Signed-off-by: Ashish Kalra <[e-mail]>
>>> Signed-off-by: Michael Roth <[e-mail]>
>>> ---
>>>   arch/x86/kvm/svm/sev.c | 185 +++++++++++++++++++++++++++++++++++++++--
>>>   arch/x86/kvm/svm/svm.h |   2 +
>>>   2 files changed, 181 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>> index 5f2b2092cdae..18efa70553c2 100644
>>> --- a/arch/x86/kvm/svm/sev.c
>>> +++ b/arch/x86/kvm/svm/sev.c
>>> @@ -331,6 +331,7 @@ static int sev_guest_init(struct kvm *kvm, struct 
>>> kvm_sev_cmd *argp)
>>>           if (ret)
>>>               goto e_free;
>>> +        mutex_init(&sev->guest_req_lock);
>>>           ret = sev_snp_init(&argp->error, false);
>>>       } else {
>>>           ret = sev_platform_init(&argp->error);
>>> @@ -2051,23 +2052,34 @@ int sev_vm_move_enc_context_from(struct kvm 
>>> *kvm, unsigned int source_fd)
>>>    */
>>>   static void *snp_context_create(struct kvm *kvm, struct kvm_sev_cmd 
>>> *argp)
>>>   {
>>> +    struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>>>       struct sev_data_snp_addr data = {};
>>> -    void *context;
>>> +    void *context, *certs_data;
>>>       int rc;
>>> +    /* Allocate memory used for the certs data in SNP guest request */
>>> +    certs_data = kzalloc(SEV_FW_BLOB_MAX_SIZE, GFP_KERNEL_ACCOUNT);
>>> +    if (!certs_data)
>>> +        return NULL;
>>> +
>>>       /* Allocate memory for context page */
>>>       context = snp_alloc_firmware_page(GFP_KERNEL_ACCOUNT);
>>>       if (!context)
>>> -        return NULL;
>>> +        goto e_free;
>>>       data.gctx_paddr = __psp_pa(context);
>>>       rc = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_GCTX_CREATE, 
>>> &data, &argp->error);
>>> -    if (rc) {
>>> -        snp_free_firmware_page(context);
>>> -        return NULL;
>>> -    }
>>> +    if (rc)
>>> +        goto e_free;
>>> +
>>> +    sev->snp_certs_data = certs_data;
>>>       return context;
>>> +
>>> +e_free:
>>> +    snp_free_firmware_page(context);
>>> +    kfree(certs_data);
>>> +    return NULL;
>>>   }
>>>   static int snp_bind_asid(struct kvm *kvm, int *error)
>>> @@ -2653,6 +2665,8 @@ static int snp_decommission_context(struct kvm 
>>> *kvm)
>>>       snp_free_firmware_page(sev->snp_context);
>>>       sev->snp_context = NULL;
>>> +    kfree(sev->snp_certs_data);
>>> +
>>>       return 0;
>>>   }
>>> @@ -3174,6 +3188,8 @@ static int sev_es_validate_vmgexit(struct 
>>> vcpu_svm *svm, u64 *exit_code)
>>>       case SVM_VMGEXIT_UNSUPPORTED_EVENT:
>>>       case SVM_VMGEXIT_HV_FEATURES:
>>>       case SVM_VMGEXIT_PSC:
>>> +    case SVM_VMGEXIT_GUEST_REQUEST:
>>> +    case SVM_VMGEXIT_EXT_GUEST_REQUEST:
>>>           break;
>>>       default:
>>>           reason = GHCB_ERR_INVALID_EVENT;
>>> @@ -3396,6 +3412,149 @@ static int snp_complete_psc(struct kvm_vcpu 
>>> *vcpu)
>>>       return 1;
>>>   }
>>> +static unsigned long snp_setup_guest_buf(struct vcpu_svm *svm,
>>> +                     struct sev_data_snp_guest_request *data,
>>> +                     gpa_t req_gpa, gpa_t resp_gpa)
>>> +{
>>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>>> +    struct kvm *kvm = vcpu->kvm;
>>> +    kvm_pfn_t req_pfn, resp_pfn;
>>> +    struct kvm_sev_info *sev;
>>> +
>>> +    sev = &to_kvm_svm(kvm)->sev_info;
>>> +
>>> +    if (!IS_ALIGNED(req_gpa, PAGE_SIZE) || !IS_ALIGNED(resp_gpa, 
>>> PAGE_SIZE))
>>> +        return SEV_RET_INVALID_PARAM;
>>> +
>>> +    req_pfn = gfn_to_pfn(kvm, gpa_to_gfn(req_gpa));
>>> +    if (is_error_noslot_pfn(req_pfn))
>>> +        return SEV_RET_INVALID_ADDRESS;
>>> +
>>> +    resp_pfn = gfn_to_pfn(kvm, gpa_to_gfn(resp_gpa));
>>> +    if (is_error_noslot_pfn(resp_pfn))
>>> +        return SEV_RET_INVALID_ADDRESS;
>>> +
>>> +    if (rmp_make_private(resp_pfn, 0, PG_LEVEL_4K, 0, true))
>>> +        return SEV_RET_INVALID_ADDRESS;
>>> +
>>> +    data->gctx_paddr = __psp_pa(sev->snp_context);
>>> +    data->req_paddr = __sme_set(req_pfn << PAGE_SHIFT);
>>> +    data->res_paddr = __sme_set(resp_pfn << PAGE_SHIFT);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void snp_cleanup_guest_buf(struct sev_data_snp_guest_request 
>>> *data, unsigned long *rc)
>>> +{
>>> +    u64 pfn = __sme_clr(data->res_paddr) >> PAGE_SHIFT;
>>> +    int ret;
>>> +
>>> +    ret = snp_page_reclaim(pfn);
>>> +    if (ret)
>>> +        *rc = SEV_RET_INVALID_ADDRESS;
>>> +
>>> +    ret = rmp_make_shared(pfn, PG_LEVEL_4K);
>>> +    if (ret)
>>> +        *rc = SEV_RET_INVALID_ADDRESS;
>>> +}
>>> +
>>> +static void snp_handle_guest_request(struct vcpu_svm *svm, gpa_t 
>>> req_gpa, gpa_t resp_gpa)
>>> +{
>>> +    struct sev_data_snp_guest_request data = {0};
>>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>>> +    struct kvm *kvm = vcpu->kvm;
>>> +    struct kvm_sev_info *sev;
>>> +    unsigned long rc;
>>> +    int err;
>>> +
>>> +    if (!sev_snp_guest(vcpu->kvm)) {
>>> +        rc = SEV_RET_INVALID_GUEST;
>>> +        goto e_fail;
>>> +    }
>>> +
>>> +    sev = &to_kvm_svm(kvm)->sev_info;
>>> +
>>> +    mutex_lock(&sev->guest_req_lock);
>>> +
>>> +    rc = snp_setup_guest_buf(svm, &data, req_gpa, resp_gpa);
>>> +    if (rc)
>>> +        goto unlock;
>>> +
>>> +    rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, &err);
>>
>>
>> This one goes via sev_issue_cmd_external_user() and uses sev-fd...
>>
>>> +    if (rc)
>>> +        /* use the firmware error code */
>>> +        rc = err;
>>> +
>>> +    snp_cleanup_guest_buf(&data, &rc);
>>> +
>>> +unlock:
>>> +    mutex_unlock(&sev->guest_req_lock);
>>> +
>>> +e_fail:
>>> +    svm_set_ghcb_sw_exit_info_2(vcpu, rc);
>>> +}
>>> +
>>> +static void snp_handle_ext_guest_request(struct vcpu_svm *svm, gpa_t 
>>> req_gpa, gpa_t resp_gpa)
>>> +{
>>> +    struct sev_data_snp_guest_request req = {0};
>>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>>> +    struct kvm *kvm = vcpu->kvm;
>>> +    unsigned long data_npages;
>>> +    struct kvm_sev_info *sev;
>>> +    unsigned long rc, err;
>>> +    u64 data_gpa;
>>> +
>>> +    if (!sev_snp_guest(vcpu->kvm)) {
>>> +        rc = SEV_RET_INVALID_GUEST;
>>> +        goto e_fail;
>>> +    }
>>> +
>>> +    sev = &to_kvm_svm(kvm)->sev_info;
>>> +
>>> +    data_gpa = vcpu->arch.regs[VCPU_REGS_RAX];
>>> +    data_npages = vcpu->arch.regs[VCPU_REGS_RBX];
>>> +
>>> +    if (!IS_ALIGNED(data_gpa, PAGE_SIZE)) {
>>> +        rc = SEV_RET_INVALID_ADDRESS;
>>> +        goto e_fail;
>>> +    }
>>> +
>>> +    mutex_lock(&sev->guest_req_lock);
>>> +
>>> +    rc = snp_setup_guest_buf(svm, &req, req_gpa, resp_gpa);
>>> +    if (rc)
>>> +        goto unlock;
>>> +
>>> +    rc = snp_guest_ext_guest_request(&req, (unsigned 
>>> long)sev->snp_certs_data,
>>> +                     &data_npages, &err);
>>
>> but this one does not and jump straight to 
>> drivers/crypto/ccp/sev-dev.c ignoring sev->fd. Why different? Can 
>> these two be unified? sev_issue_cmd_external_user() only checks if fd 
>> is /dev/sev which is hardly useful.
>>
>> "[PATCH RFC v7 32/64] crypto: ccp: Provide APIs to query extended 
>> attestation report" added this one.
> 
> SNP_EXT_GUEST_REQUEST additionally returns a certificate blob and that's 
> why it goes through the CCP driver interface 
> snp_guest_ext_guest_request() that is used to get both the report and 
> certificate data/blob at the same time.

True. I thought though that this calls for extending sev_issue_cmd() to 
take care of these extra parameters rather than just skipping the sev->fd.


> All the FW API calls on the KVM side go through sev_issue_cmd() and 
> sev_issue_cmd_external_user() interfaces and that i believe uses sev->fd 
> more of as a sanity check.

Does not look like it:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/crypto/ccp/sev-dev.c?h=v6.2-rc3#n1290

===
int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd,
				void *data, int *error)
{
	if (!filep || filep->f_op != &sev_fops)
		return -EBADF;

	return sev_do_cmd(cmd, data, error);
}
EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
===

The only "more" is that it requires sev->fd to be a valid open fd, what 
is the value in that? I may easily miss the bigger picture here. Thanks,


> Thanks,
> Ashish
> 
>>
>> Besides, is sev->fd really needed in the sev struct at all? Thanks,
>>
>>
>>> +    if (rc) {
>>> +        /*
>>> +         * If buffer length is small then return the expected
>>> +         * length in rbx.
>>> +         */
>>> +        if (err == SNP_GUEST_REQ_INVALID_LEN)
>>> +            vcpu->arch.regs[VCPU_REGS_RBX] = data_npages;
>>> +
>>> +        /* pass the firmware error code */
>>> +        rc = err;
>>> +        goto cleanup;
>>> +    }
>>> +
>>> +    /* Copy the certificate blob in the guest memory */
>>> +    if (data_npages &&
>>> +        kvm_write_guest(kvm, data_gpa, sev->snp_certs_data, 
>>> data_npages << PAGE_SHIFT))
>>> +        rc = SEV_RET_INVALID_ADDRESS;
>>> +
>>> +cleanup:
>>> +    snp_cleanup_guest_buf(&req, &rc);
>>> +
>>> +unlock:
>>> +    mutex_unlock(&sev->guest_req_lock);
>>> +
>>> +e_fail:
>>> +    svm_set_ghcb_sw_exit_info_2(vcpu, rc);
>>> +}
>>> +
>>>   static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
>>>   {
>>>       struct vmcb_control_area *control = &svm->vmcb->control;
>>> @@ -3629,6 +3788,20 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
>>>           vcpu->run->vmgexit.ghcb_msr = ghcb_gpa;
>>>           vcpu->arch.complete_userspace_io = snp_complete_psc;
>>>           break;
>>> +    case SVM_VMGEXIT_GUEST_REQUEST: {
>>> +        snp_handle_guest_request(svm, control->exit_info_1, 
>>> control->exit_info_2);
>>> +
>>> +        ret = 1;
>>> +        break;
>>> +    }
>>> +    case SVM_VMGEXIT_EXT_GUEST_REQUEST: {
>>> +        snp_handle_ext_guest_request(svm,
>>> +                         control->exit_info_1,
>>> +                         control->exit_info_2);
>>> +
>>> +        ret = 1;
>>> +        break;
>>> +    }
>>>       case SVM_VMGEXIT_UNSUPPORTED_EVENT:
>>>           vcpu_unimpl(vcpu,
>>>                   "vmgexit: unsupported event - exit_info_1=%#llx, 
>>> exit_info_2=%#llx\n",
>>> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
>>> index 12b9f4d539fb..7c0f9d00950f 100644
>>> --- a/arch/x86/kvm/svm/svm.h
>>> +++ b/arch/x86/kvm/svm/svm.h
>>> @@ -101,6 +101,8 @@ struct kvm_sev_info {
>>>       u64 snp_init_flags;
>>>       void *snp_context;      /* SNP guest context page */
>>>       spinlock_t psc_lock;
>>> +    void *snp_certs_data;
>>> +    struct mutex guest_req_lock;
>>>   };
>>>   struct kvm_svm {
>>

Kalra, Ashish Jan. 10, 2023, 8:33 a.m. UTC | #4

On 1/9/2023 8:28 PM, Alexey Kardashevskiy wrote:
> 
> 
> On 10/1/23 10:41, Kalra, Ashish wrote:
>> On 1/8/2023 9:33 PM, Alexey Kardashevskiy wrote:
>>> On 15/12/22 06:40, Michael Roth wrote:
>>>> From: Brijesh Singh <[e-mail]>
>>>>
>>>> Version 2 of GHCB specification added the support for two SNP Guest
>>>> Request Message NAE events. The events allows for an SEV-SNP guest to
>>>> make request to the SEV-SNP firmware through hypervisor using the
>>>> SNP_GUEST_REQUEST API define in the SEV-SNP firmware specification.
>>>>
>>>> The SNP_EXT_GUEST_REQUEST is similar to SNP_GUEST_REQUEST with the
>>>> difference of an additional certificate blob that can be passed through
>>>> the SNP_SET_CONFIG ioctl defined in the CCP driver. The CCP driver
>>>> provides snp_guest_ext_guest_request() that is used by the KVM to get
>>>> both the report and certificate data at once.
>>>>
>>>> Signed-off-by: Brijesh Singh <[e-mail]>
>>>> Signed-off-by: Ashish Kalra <[e-mail]>
>>>> Signed-off-by: Michael Roth <[e-mail]>
>>>> ---
>>>>   arch/x86/kvm/svm/sev.c | 185 
>>>> +++++++++++++++++++++++++++++++++++++++--
>>>>   arch/x86/kvm/svm/svm.h |   2 +
>>>>   2 files changed, 181 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>>> index 5f2b2092cdae..18efa70553c2 100644
>>>> --- a/arch/x86/kvm/svm/sev.c
>>>> +++ b/arch/x86/kvm/svm/sev.c
>>>> @@ -331,6 +331,7 @@ static int sev_guest_init(struct kvm *kvm, 
>>>> struct kvm_sev_cmd *argp)
>>>>           if (ret)
>>>>               goto e_free;
>>>> +        mutex_init(&sev->guest_req_lock);
>>>>           ret = sev_snp_init(&argp->error, false);
>>>>       } else {
>>>>           ret = sev_platform_init(&argp->error);
>>>> @@ -2051,23 +2052,34 @@ int sev_vm_move_enc_context_from(struct kvm 
>>>> *kvm, unsigned int source_fd)
>>>>    */
>>>>   static void *snp_context_create(struct kvm *kvm, struct 
>>>> kvm_sev_cmd *argp)
>>>>   {
>>>> +    struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>>>>       struct sev_data_snp_addr data = {};
>>>> -    void *context;
>>>> +    void *context, *certs_data;
>>>>       int rc;
>>>> +    /* Allocate memory used for the certs data in SNP guest request */
>>>> +    certs_data = kzalloc(SEV_FW_BLOB_MAX_SIZE, GFP_KERNEL_ACCOUNT);
>>>> +    if (!certs_data)
>>>> +        return NULL;
>>>> +
>>>>       /* Allocate memory for context page */
>>>>       context = snp_alloc_firmware_page(GFP_KERNEL_ACCOUNT);
>>>>       if (!context)
>>>> -        return NULL;
>>>> +        goto e_free;
>>>>       data.gctx_paddr = __psp_pa(context);
>>>>       rc = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_GCTX_CREATE, 
>>>> &data, &argp->error);
>>>> -    if (rc) {
>>>> -        snp_free_firmware_page(context);
>>>> -        return NULL;
>>>> -    }
>>>> +    if (rc)
>>>> +        goto e_free;
>>>> +
>>>> +    sev->snp_certs_data = certs_data;
>>>>       return context;
>>>> +
>>>> +e_free:
>>>> +    snp_free_firmware_page(context);
>>>> +    kfree(certs_data);
>>>> +    return NULL;
>>>>   }
>>>>   static int snp_bind_asid(struct kvm *kvm, int *error)
>>>> @@ -2653,6 +2665,8 @@ static int snp_decommission_context(struct kvm 
>>>> *kvm)
>>>>       snp_free_firmware_page(sev->snp_context);
>>>>       sev->snp_context = NULL;
>>>> +    kfree(sev->snp_certs_data);
>>>> +
>>>>       return 0;
>>>>   }
>>>> @@ -3174,6 +3188,8 @@ static int sev_es_validate_vmgexit(struct 
>>>> vcpu_svm *svm, u64 *exit_code)
>>>>       case SVM_VMGEXIT_UNSUPPORTED_EVENT:
>>>>       case SVM_VMGEXIT_HV_FEATURES:
>>>>       case SVM_VMGEXIT_PSC:
>>>> +    case SVM_VMGEXIT_GUEST_REQUEST:
>>>> +    case SVM_VMGEXIT_EXT_GUEST_REQUEST:
>>>>           break;
>>>>       default:
>>>>           reason = GHCB_ERR_INVALID_EVENT;
>>>> @@ -3396,6 +3412,149 @@ static int snp_complete_psc(struct kvm_vcpu 
>>>> *vcpu)
>>>>       return 1;
>>>>   }
>>>> +static unsigned long snp_setup_guest_buf(struct vcpu_svm *svm,
>>>> +                     struct sev_data_snp_guest_request *data,
>>>> +                     gpa_t req_gpa, gpa_t resp_gpa)
>>>> +{
>>>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>>>> +    struct kvm *kvm = vcpu->kvm;
>>>> +    kvm_pfn_t req_pfn, resp_pfn;
>>>> +    struct kvm_sev_info *sev;
>>>> +
>>>> +    sev = &to_kvm_svm(kvm)->sev_info;
>>>> +
>>>> +    if (!IS_ALIGNED(req_gpa, PAGE_SIZE) || !IS_ALIGNED(resp_gpa, 
>>>> PAGE_SIZE))
>>>> +        return SEV_RET_INVALID_PARAM;
>>>> +
>>>> +    req_pfn = gfn_to_pfn(kvm, gpa_to_gfn(req_gpa));
>>>> +    if (is_error_noslot_pfn(req_pfn))
>>>> +        return SEV_RET_INVALID_ADDRESS;
>>>> +
>>>> +    resp_pfn = gfn_to_pfn(kvm, gpa_to_gfn(resp_gpa));
>>>> +    if (is_error_noslot_pfn(resp_pfn))
>>>> +        return SEV_RET_INVALID_ADDRESS;
>>>> +
>>>> +    if (rmp_make_private(resp_pfn, 0, PG_LEVEL_4K, 0, true))
>>>> +        return SEV_RET_INVALID_ADDRESS;
>>>> +
>>>> +    data->gctx_paddr = __psp_pa(sev->snp_context);
>>>> +    data->req_paddr = __sme_set(req_pfn << PAGE_SHIFT);
>>>> +    data->res_paddr = __sme_set(resp_pfn << PAGE_SHIFT);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static void snp_cleanup_guest_buf(struct sev_data_snp_guest_request 
>>>> *data, unsigned long *rc)
>>>> +{
>>>> +    u64 pfn = __sme_clr(data->res_paddr) >> PAGE_SHIFT;
>>>> +    int ret;
>>>> +
>>>> +    ret = snp_page_reclaim(pfn);
>>>> +    if (ret)
>>>> +        *rc = SEV_RET_INVALID_ADDRESS;
>>>> +
>>>> +    ret = rmp_make_shared(pfn, PG_LEVEL_4K);
>>>> +    if (ret)
>>>> +        *rc = SEV_RET_INVALID_ADDRESS;
>>>> +}
>>>> +
>>>> +static void snp_handle_guest_request(struct vcpu_svm *svm, gpa_t 
>>>> req_gpa, gpa_t resp_gpa)
>>>> +{
>>>> +    struct sev_data_snp_guest_request data = {0};
>>>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>>>> +    struct kvm *kvm = vcpu->kvm;
>>>> +    struct kvm_sev_info *sev;
>>>> +    unsigned long rc;
>>>> +    int err;
>>>> +
>>>> +    if (!sev_snp_guest(vcpu->kvm)) {
>>>> +        rc = SEV_RET_INVALID_GUEST;
>>>> +        goto e_fail;
>>>> +    }
>>>> +
>>>> +    sev = &to_kvm_svm(kvm)->sev_info;
>>>> +
>>>> +    mutex_lock(&sev->guest_req_lock);
>>>> +
>>>> +    rc = snp_setup_guest_buf(svm, &data, req_gpa, resp_gpa);
>>>> +    if (rc)
>>>> +        goto unlock;
>>>> +
>>>> +    rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, &err);
>>>
>>>
>>> This one goes via sev_issue_cmd_external_user() and uses sev-fd...
>>>
>>>> +    if (rc)
>>>> +        /* use the firmware error code */
>>>> +        rc = err;
>>>> +
>>>> +    snp_cleanup_guest_buf(&data, &rc);
>>>> +
>>>> +unlock:
>>>> +    mutex_unlock(&sev->guest_req_lock);
>>>> +
>>>> +e_fail:
>>>> +    svm_set_ghcb_sw_exit_info_2(vcpu, rc);
>>>> +}
>>>> +
>>>> +static void snp_handle_ext_guest_request(struct vcpu_svm *svm, 
>>>> gpa_t req_gpa, gpa_t resp_gpa)
>>>> +{
>>>> +    struct sev_data_snp_guest_request req = {0};
>>>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>>>> +    struct kvm *kvm = vcpu->kvm;
>>>> +    unsigned long data_npages;
>>>> +    struct kvm_sev_info *sev;
>>>> +    unsigned long rc, err;
>>>> +    u64 data_gpa;
>>>> +
>>>> +    if (!sev_snp_guest(vcpu->kvm)) {
>>>> +        rc = SEV_RET_INVALID_GUEST;
>>>> +        goto e_fail;
>>>> +    }
>>>> +
>>>> +    sev = &to_kvm_svm(kvm)->sev_info;
>>>> +
>>>> +    data_gpa = vcpu->arch.regs[VCPU_REGS_RAX];
>>>> +    data_npages = vcpu->arch.regs[VCPU_REGS_RBX];
>>>> +
>>>> +    if (!IS_ALIGNED(data_gpa, PAGE_SIZE)) {
>>>> +        rc = SEV_RET_INVALID_ADDRESS;
>>>> +        goto e_fail;
>>>> +    }
>>>> +
>>>> +    mutex_lock(&sev->guest_req_lock);
>>>> +
>>>> +    rc = snp_setup_guest_buf(svm, &req, req_gpa, resp_gpa);
>>>> +    if (rc)
>>>> +        goto unlock;
>>>> +
>>>> +    rc = snp_guest_ext_guest_request(&req, (unsigned 
>>>> long)sev->snp_certs_data,
>>>> +                     &data_npages, &err);
>>>
>>> but this one does not and jump straight to 
>>> drivers/crypto/ccp/sev-dev.c ignoring sev->fd. Why different? Can 
>>> these two be unified? sev_issue_cmd_external_user() only checks if fd 
>>> is /dev/sev which is hardly useful.
>>>
>>> "[PATCH RFC v7 32/64] crypto: ccp: Provide APIs to query extended 
>>> attestation report" added this one.
>>
>> SNP_EXT_GUEST_REQUEST additionally returns a certificate blob and 
>> that's why it goes through the CCP driver interface 
>> snp_guest_ext_guest_request() that is used to get both the report and 
>> certificate data/blob at the same time.
> 
> True. I thought though that this calls for extending sev_issue_cmd() to 
> take care of these extra parameters rather than just skipping the sev->fd.
> 
> 
>> All the FW API calls on the KVM side go through sev_issue_cmd() and 
>> sev_issue_cmd_external_user() interfaces and that i believe uses 
>> sev->fd more of as a sanity check.
> 
> Does not look like it:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/crypto/ccp/sev-dev.c?h=v6.2-rc3#n1290 
> 
> 
> ===
> int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd,
>                  void *data, int *error)
> {
>      if (!filep || filep->f_op != &sev_fops)
>          return -EBADF;
> 
>      return sev_do_cmd(cmd, data, error);
> }
> EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
> ===
> 
> The only "more" is that it requires sev->fd to be a valid open fd, what 
> is the value in that? I may easily miss the bigger picture here. Thanks,
> 
> 

Have a look at following functions in drivers/crypto/ccp/sev-dev.c:
sev_dev_init() and sev_misc_init().

static int sev_misc_init(struct sev_device *sev)
{
         struct device *dev = sev->dev;
         int ret;

         /*
          * SEV feature support can be detected on multiple devices but
          * the SEV FW commands must be issued on the master. During
          * probe, we do not know the master hence we create /dev/sev on
          * the first device probe.
          * sev_do_cmd() finds the right master device to which to issue
          * the command to the firmware.
	 */
...
...

Hence, sev_issue_cmd_external_user() needs to ensure that the correct 
device (master device) is being operated upon and that's why there is 
the check for file operations matching sev_fops as below :

int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd,
                                 void *data, int *error)
{
         if (!filep || filep->f_op != &sev_fops)
                 return -EBADF;
..
..

Essentially, sev->fd is the misc. device created for the master PSP 
device on which the SEV/SNP firmware commands are issued, hence,
sev_issue_cmd() uses sev->fd.

Thanks,
Ashish

Alexey Kardashevskiy Jan. 11, 2023, 12:48 a.m. UTC | #5

On 10/1/23 19:33, Kalra, Ashish wrote:
> 
> On 1/9/2023 8:28 PM, Alexey Kardashevskiy wrote:
>>
>>
>> On 10/1/23 10:41, Kalra, Ashish wrote:
>>> On 1/8/2023 9:33 PM, Alexey Kardashevskiy wrote:
>>>> On 15/12/22 06:40, Michael Roth wrote:
>>>>> From: Brijesh Singh <[e-mail]>
>>>>>
>>>>> Version 2 of GHCB specification added the support for two SNP Guest
>>>>> Request Message NAE events. The events allows for an SEV-SNP guest to
>>>>> make request to the SEV-SNP firmware through hypervisor using the
>>>>> SNP_GUEST_REQUEST API define in the SEV-SNP firmware specification.
>>>>>
>>>>> The SNP_EXT_GUEST_REQUEST is similar to SNP_GUEST_REQUEST with the
>>>>> difference of an additional certificate blob that can be passed 
>>>>> through
>>>>> the SNP_SET_CONFIG ioctl defined in the CCP driver. The CCP driver
>>>>> provides snp_guest_ext_guest_request() that is used by the KVM to get
>>>>> both the report and certificate data at once.
>>>>>
>>>>> Signed-off-by: Brijesh Singh <[e-mail]>
>>>>> Signed-off-by: Ashish Kalra <[e-mail]>
>>>>> Signed-off-by: Michael Roth <[e-mail]>
>>>>> ---
>>>>>   arch/x86/kvm/svm/sev.c | 185 
>>>>> +++++++++++++++++++++++++++++++++++++++--
>>>>>   arch/x86/kvm/svm/svm.h |   2 +
>>>>>   2 files changed, 181 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>>>> index 5f2b2092cdae..18efa70553c2 100644
>>>>> --- a/arch/x86/kvm/svm/sev.c
>>>>> +++ b/arch/x86/kvm/svm/sev.c
>>>>> @@ -331,6 +331,7 @@ static int sev_guest_init(struct kvm *kvm, 
>>>>> struct kvm_sev_cmd *argp)
>>>>>           if (ret)
>>>>>               goto e_free;
>>>>> +        mutex_init(&sev->guest_req_lock);
>>>>>           ret = sev_snp_init(&argp->error, false);
>>>>>       } else {
>>>>>           ret = sev_platform_init(&argp->error);
>>>>> @@ -2051,23 +2052,34 @@ int sev_vm_move_enc_context_from(struct kvm 
>>>>> *kvm, unsigned int source_fd)
>>>>>    */
>>>>>   static void *snp_context_create(struct kvm *kvm, struct 
>>>>> kvm_sev_cmd *argp)
>>>>>   {
>>>>> +    struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>>>>>       struct sev_data_snp_addr data = {};
>>>>> -    void *context;
>>>>> +    void *context, *certs_data;
>>>>>       int rc;
>>>>> +    /* Allocate memory used for the certs data in SNP guest 
>>>>> request */
>>>>> +    certs_data = kzalloc(SEV_FW_BLOB_MAX_SIZE, GFP_KERNEL_ACCOUNT);
>>>>> +    if (!certs_data)
>>>>> +        return NULL;
>>>>> +
>>>>>       /* Allocate memory for context page */
>>>>>       context = snp_alloc_firmware_page(GFP_KERNEL_ACCOUNT);
>>>>>       if (!context)
>>>>> -        return NULL;
>>>>> +        goto e_free;
>>>>>       data.gctx_paddr = __psp_pa(context);
>>>>>       rc = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_GCTX_CREATE, 
>>>>> &data, &argp->error);
>>>>> -    if (rc) {
>>>>> -        snp_free_firmware_page(context);
>>>>> -        return NULL;
>>>>> -    }
>>>>> +    if (rc)
>>>>> +        goto e_free;
>>>>> +
>>>>> +    sev->snp_certs_data = certs_data;
>>>>>       return context;
>>>>> +
>>>>> +e_free:
>>>>> +    snp_free_firmware_page(context);
>>>>> +    kfree(certs_data);
>>>>> +    return NULL;
>>>>>   }
>>>>>   static int snp_bind_asid(struct kvm *kvm, int *error)
>>>>> @@ -2653,6 +2665,8 @@ static int snp_decommission_context(struct 
>>>>> kvm *kvm)
>>>>>       snp_free_firmware_page(sev->snp_context);
>>>>>       sev->snp_context = NULL;
>>>>> +    kfree(sev->snp_certs_data);
>>>>> +
>>>>>       return 0;
>>>>>   }
>>>>> @@ -3174,6 +3188,8 @@ static int sev_es_validate_vmgexit(struct 
>>>>> vcpu_svm *svm, u64 *exit_code)
>>>>>       case SVM_VMGEXIT_UNSUPPORTED_EVENT:
>>>>>       case SVM_VMGEXIT_HV_FEATURES:
>>>>>       case SVM_VMGEXIT_PSC:
>>>>> +    case SVM_VMGEXIT_GUEST_REQUEST:
>>>>> +    case SVM_VMGEXIT_EXT_GUEST_REQUEST:
>>>>>           break;
>>>>>       default:
>>>>>           reason = GHCB_ERR_INVALID_EVENT;
>>>>> @@ -3396,6 +3412,149 @@ static int snp_complete_psc(struct kvm_vcpu 
>>>>> *vcpu)
>>>>>       return 1;
>>>>>   }
>>>>> +static unsigned long snp_setup_guest_buf(struct vcpu_svm *svm,
>>>>> +                     struct sev_data_snp_guest_request *data,
>>>>> +                     gpa_t req_gpa, gpa_t resp_gpa)
>>>>> +{
>>>>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>>>>> +    struct kvm *kvm = vcpu->kvm;
>>>>> +    kvm_pfn_t req_pfn, resp_pfn;
>>>>> +    struct kvm_sev_info *sev;
>>>>> +
>>>>> +    sev = &to_kvm_svm(kvm)->sev_info;
>>>>> +
>>>>> +    if (!IS_ALIGNED(req_gpa, PAGE_SIZE) || !IS_ALIGNED(resp_gpa, 
>>>>> PAGE_SIZE))
>>>>> +        return SEV_RET_INVALID_PARAM;
>>>>> +
>>>>> +    req_pfn = gfn_to_pfn(kvm, gpa_to_gfn(req_gpa));
>>>>> +    if (is_error_noslot_pfn(req_pfn))
>>>>> +        return SEV_RET_INVALID_ADDRESS;
>>>>> +
>>>>> +    resp_pfn = gfn_to_pfn(kvm, gpa_to_gfn(resp_gpa));
>>>>> +    if (is_error_noslot_pfn(resp_pfn))
>>>>> +        return SEV_RET_INVALID_ADDRESS;
>>>>> +
>>>>> +    if (rmp_make_private(resp_pfn, 0, PG_LEVEL_4K, 0, true))
>>>>> +        return SEV_RET_INVALID_ADDRESS;
>>>>> +
>>>>> +    data->gctx_paddr = __psp_pa(sev->snp_context);
>>>>> +    data->req_paddr = __sme_set(req_pfn << PAGE_SHIFT);
>>>>> +    data->res_paddr = __sme_set(resp_pfn << PAGE_SHIFT);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static void snp_cleanup_guest_buf(struct 
>>>>> sev_data_snp_guest_request *data, unsigned long *rc)
>>>>> +{
>>>>> +    u64 pfn = __sme_clr(data->res_paddr) >> PAGE_SHIFT;
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = snp_page_reclaim(pfn);
>>>>> +    if (ret)
>>>>> +        *rc = SEV_RET_INVALID_ADDRESS;
>>>>> +
>>>>> +    ret = rmp_make_shared(pfn, PG_LEVEL_4K);
>>>>> +    if (ret)
>>>>> +        *rc = SEV_RET_INVALID_ADDRESS;
>>>>> +}
>>>>> +
>>>>> +static void snp_handle_guest_request(struct vcpu_svm *svm, gpa_t 
>>>>> req_gpa, gpa_t resp_gpa)
>>>>> +{
>>>>> +    struct sev_data_snp_guest_request data = {0};
>>>>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>>>>> +    struct kvm *kvm = vcpu->kvm;
>>>>> +    struct kvm_sev_info *sev;
>>>>> +    unsigned long rc;
>>>>> +    int err;
>>>>> +
>>>>> +    if (!sev_snp_guest(vcpu->kvm)) {
>>>>> +        rc = SEV_RET_INVALID_GUEST;
>>>>> +        goto e_fail;
>>>>> +    }
>>>>> +
>>>>> +    sev = &to_kvm_svm(kvm)->sev_info;
>>>>> +
>>>>> +    mutex_lock(&sev->guest_req_lock);
>>>>> +
>>>>> +    rc = snp_setup_guest_buf(svm, &data, req_gpa, resp_gpa);
>>>>> +    if (rc)
>>>>> +        goto unlock;
>>>>> +
>>>>> +    rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, &err);
>>>>
>>>>
>>>> This one goes via sev_issue_cmd_external_user() and uses sev-fd...
>>>>
>>>>> +    if (rc)
>>>>> +        /* use the firmware error code */
>>>>> +        rc = err;
>>>>> +
>>>>> +    snp_cleanup_guest_buf(&data, &rc);
>>>>> +
>>>>> +unlock:
>>>>> +    mutex_unlock(&sev->guest_req_lock);
>>>>> +
>>>>> +e_fail:
>>>>> +    svm_set_ghcb_sw_exit_info_2(vcpu, rc);
>>>>> +}
>>>>> +
>>>>> +static void snp_handle_ext_guest_request(struct vcpu_svm *svm, 
>>>>> gpa_t req_gpa, gpa_t resp_gpa)
>>>>> +{
>>>>> +    struct sev_data_snp_guest_request req = {0};
>>>>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>>>>> +    struct kvm *kvm = vcpu->kvm;
>>>>> +    unsigned long data_npages;
>>>>> +    struct kvm_sev_info *sev;
>>>>> +    unsigned long rc, err;
>>>>> +    u64 data_gpa;
>>>>> +
>>>>> +    if (!sev_snp_guest(vcpu->kvm)) {
>>>>> +        rc = SEV_RET_INVALID_GUEST;
>>>>> +        goto e_fail;
>>>>> +    }
>>>>> +
>>>>> +    sev = &to_kvm_svm(kvm)->sev_info;
>>>>> +
>>>>> +    data_gpa = vcpu->arch.regs[VCPU_REGS_RAX];
>>>>> +    data_npages = vcpu->arch.regs[VCPU_REGS_RBX];
>>>>> +
>>>>> +    if (!IS_ALIGNED(data_gpa, PAGE_SIZE)) {
>>>>> +        rc = SEV_RET_INVALID_ADDRESS;
>>>>> +        goto e_fail;
>>>>> +    }
>>>>> +
>>>>> +    mutex_lock(&sev->guest_req_lock);
>>>>> +
>>>>> +    rc = snp_setup_guest_buf(svm, &req, req_gpa, resp_gpa);
>>>>> +    if (rc)
>>>>> +        goto unlock;
>>>>> +
>>>>> +    rc = snp_guest_ext_guest_request(&req, (unsigned 
>>>>> long)sev->snp_certs_data,
>>>>> +                     &data_npages, &err);
>>>>
>>>> but this one does not and jump straight to 
>>>> drivers/crypto/ccp/sev-dev.c ignoring sev->fd. Why different? Can 
>>>> these two be unified? sev_issue_cmd_external_user() only checks if 
>>>> fd is /dev/sev which is hardly useful.
>>>>
>>>> "[PATCH RFC v7 32/64] crypto: ccp: Provide APIs to query extended 
>>>> attestation report" added this one.
>>>
>>> SNP_EXT_GUEST_REQUEST additionally returns a certificate blob and 
>>> that's why it goes through the CCP driver interface 
>>> snp_guest_ext_guest_request() that is used to get both the report and 
>>> certificate data/blob at the same time.
>>
>> True. I thought though that this calls for extending sev_issue_cmd() 
>> to take care of these extra parameters rather than just skipping the 
>> sev->fd.
>>
>>
>>> All the FW API calls on the KVM side go through sev_issue_cmd() and 
>>> sev_issue_cmd_external_user() interfaces and that i believe uses 
>>> sev->fd more of as a sanity check.
>>
>> Does not look like it:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/crypto/ccp/sev-dev.c?h=v6.2-rc3#n1290
>>
>> ===
>> int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd,
>>                  void *data, int *error)
>> {
>>      if (!filep || filep->f_op != &sev_fops)
>>          return -EBADF;
>>
>>      return sev_do_cmd(cmd, data, error);
>> }
>> EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
>> ===
>>
>> The only "more" is that it requires sev->fd to be a valid open fd, 
>> what is the value in that? I may easily miss the bigger picture here. 
>> Thanks,
>>
>>
> 
> Have a look at following functions in drivers/crypto/ccp/sev-dev.c:
> sev_dev_init() and sev_misc_init().
> 
> static int sev_misc_init(struct sev_device *sev)
> {
>          struct device *dev = sev->dev;
>          int ret;
> 
>          /*
>           * SEV feature support can be detected on multiple devices but
>           * the SEV FW commands must be issued on the master. During
>           * probe, we do not know the master hence we create /dev/sev on
>           * the first device probe.
>           * sev_do_cmd() finds the right master device to which to issue
>           * the command to the firmware.
>       */


It is still a single /dev/sev node and the userspace cannot get it 
wrong, it does not have to choose between (for instance) /dev/sev0 and 
/dev/sev1 on a 2 SOC system.

> ...
> ...
> 
> Hence, sev_issue_cmd_external_user() needs to ensure that the correct 
> device (master device) is being operated upon and that's why there is 
> the check for file operations matching sev_fops as below :
> 
> int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd,
>                                  void *data, int *error)
> {
>          if (!filep || filep->f_op != &sev_fops)
>                  return -EBADF;
> ..
> ..
> 
> Essentially, sev->fd is the misc. device created for the master PSP 
> device on which the SEV/SNP firmware commands are issued, hence,
> sev_issue_cmd() uses sev->fd.

There is always just one fd which always uses psp_master, nothing from 
that fd is used.

More to the point, if sev->fd is still important, why is it ok to skip 
it for snp_handle_ext_guest_request()? Thanks,

Kalra, Ashish Jan. 11, 2023, 2:01 a.m. UTC | #6

On 1/10/2023 6:48 PM, Alexey Kardashevskiy wrote:
> On 10/1/23 19:33, Kalra, Ashish wrote:
>>
>> On 1/9/2023 8:28 PM, Alexey Kardashevskiy wrote:
>>>
>>>
>>> On 10/1/23 10:41, Kalra, Ashish wrote:
>>>> On 1/8/2023 9:33 PM, Alexey Kardashevskiy wrote:
>>>>> On 15/12/22 06:40, Michael Roth wrote:
>>>>>> From: Brijesh Singh <[e-mail]>
>>>>>>
>>>>>> Version 2 of GHCB specification added the support for two SNP Guest
>>>>>> Request Message NAE events. The events allows for an SEV-SNP guest to
>>>>>> make request to the SEV-SNP firmware through hypervisor using the
>>>>>> SNP_GUEST_REQUEST API define in the SEV-SNP firmware specification.
>>>>>>
>>>>>> The SNP_EXT_GUEST_REQUEST is similar to SNP_GUEST_REQUEST with the
>>>>>> difference of an additional certificate blob that can be passed 
>>>>>> through
>>>>>> the SNP_SET_CONFIG ioctl defined in the CCP driver. The CCP driver
>>>>>> provides snp_guest_ext_guest_request() that is used by the KVM to get
>>>>>> both the report and certificate data at once.
>>>>>>
>>>>>> Signed-off-by: Brijesh Singh <[e-mail]>
>>>>>> Signed-off-by: Ashish Kalra <[e-mail]>
>>>>>> Signed-off-by: Michael Roth <[e-mail]>
>>>>>> ---
>>>>>>   arch/x86/kvm/svm/sev.c | 185 
>>>>>> +++++++++++++++++++++++++++++++++++++++--
>>>>>>   arch/x86/kvm/svm/svm.h |   2 +
>>>>>>   2 files changed, 181 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>>>>> index 5f2b2092cdae..18efa70553c2 100644
>>>>>> --- a/arch/x86/kvm/svm/sev.c
>>>>>> +++ b/arch/x86/kvm/svm/sev.c
>>>>>> @@ -331,6 +331,7 @@ static int sev_guest_init(struct kvm *kvm, 
>>>>>> struct kvm_sev_cmd *argp)
>>>>>>           if (ret)
>>>>>>               goto e_free;
>>>>>> +        mutex_init(&sev->guest_req_lock);
>>>>>>           ret = sev_snp_init(&argp->error, false);
>>>>>>       } else {
>>>>>>           ret = sev_platform_init(&argp->error);
>>>>>> @@ -2051,23 +2052,34 @@ int sev_vm_move_enc_context_from(struct 
>>>>>> kvm *kvm, unsigned int source_fd)
>>>>>>    */
>>>>>>   static void *snp_context_create(struct kvm *kvm, struct 
>>>>>> kvm_sev_cmd *argp)
>>>>>>   {
>>>>>> +    struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>>>>>>       struct sev_data_snp_addr data = {};
>>>>>> -    void *context;
>>>>>> +    void *context, *certs_data;
>>>>>>       int rc;
>>>>>> +    /* Allocate memory used for the certs data in SNP guest 
>>>>>> request */
>>>>>> +    certs_data = kzalloc(SEV_FW_BLOB_MAX_SIZE, GFP_KERNEL_ACCOUNT);
>>>>>> +    if (!certs_data)
>>>>>> +        return NULL;
>>>>>> +
>>>>>>       /* Allocate memory for context page */
>>>>>>       context = snp_alloc_firmware_page(GFP_KERNEL_ACCOUNT);
>>>>>>       if (!context)
>>>>>> -        return NULL;
>>>>>> +        goto e_free;
>>>>>>       data.gctx_paddr = __psp_pa(context);
>>>>>>       rc = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_GCTX_CREATE, 
>>>>>> &data, &argp->error);
>>>>>> -    if (rc) {
>>>>>> -        snp_free_firmware_page(context);
>>>>>> -        return NULL;
>>>>>> -    }
>>>>>> +    if (rc)
>>>>>> +        goto e_free;
>>>>>> +
>>>>>> +    sev->snp_certs_data = certs_data;
>>>>>>       return context;
>>>>>> +
>>>>>> +e_free:
>>>>>> +    snp_free_firmware_page(context);
>>>>>> +    kfree(certs_data);
>>>>>> +    return NULL;
>>>>>>   }
>>>>>>   static int snp_bind_asid(struct kvm *kvm, int *error)
>>>>>> @@ -2653,6 +2665,8 @@ static int snp_decommission_context(struct 
>>>>>> kvm *kvm)
>>>>>>       snp_free_firmware_page(sev->snp_context);
>>>>>>       sev->snp_context = NULL;
>>>>>> +    kfree(sev->snp_certs_data);
>>>>>> +
>>>>>>       return 0;
>>>>>>   }
>>>>>> @@ -3174,6 +3188,8 @@ static int sev_es_validate_vmgexit(struct 
>>>>>> vcpu_svm *svm, u64 *exit_code)
>>>>>>       case SVM_VMGEXIT_UNSUPPORTED_EVENT:
>>>>>>       case SVM_VMGEXIT_HV_FEATURES:
>>>>>>       case SVM_VMGEXIT_PSC:
>>>>>> +    case SVM_VMGEXIT_GUEST_REQUEST:
>>>>>> +    case SVM_VMGEXIT_EXT_GUEST_REQUEST:
>>>>>>           break;
>>>>>>       default:
>>>>>>           reason = GHCB_ERR_INVALID_EVENT;
>>>>>> @@ -3396,6 +3412,149 @@ static int snp_complete_psc(struct 
>>>>>> kvm_vcpu *vcpu)
>>>>>>       return 1;
>>>>>>   }
>>>>>> +static unsigned long snp_setup_guest_buf(struct vcpu_svm *svm,
>>>>>> +                     struct sev_data_snp_guest_request *data,
>>>>>> +                     gpa_t req_gpa, gpa_t resp_gpa)
>>>>>> +{
>>>>>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>>>>>> +    struct kvm *kvm = vcpu->kvm;
>>>>>> +    kvm_pfn_t req_pfn, resp_pfn;
>>>>>> +    struct kvm_sev_info *sev;
>>>>>> +
>>>>>> +    sev = &to_kvm_svm(kvm)->sev_info;
>>>>>> +
>>>>>> +    if (!IS_ALIGNED(req_gpa, PAGE_SIZE) || !IS_ALIGNED(resp_gpa, 
>>>>>> PAGE_SIZE))
>>>>>> +        return SEV_RET_INVALID_PARAM;
>>>>>> +
>>>>>> +    req_pfn = gfn_to_pfn(kvm, gpa_to_gfn(req_gpa));
>>>>>> +    if (is_error_noslot_pfn(req_pfn))
>>>>>> +        return SEV_RET_INVALID_ADDRESS;
>>>>>> +
>>>>>> +    resp_pfn = gfn_to_pfn(kvm, gpa_to_gfn(resp_gpa));
>>>>>> +    if (is_error_noslot_pfn(resp_pfn))
>>>>>> +        return SEV_RET_INVALID_ADDRESS;
>>>>>> +
>>>>>> +    if (rmp_make_private(resp_pfn, 0, PG_LEVEL_4K, 0, true))
>>>>>> +        return SEV_RET_INVALID_ADDRESS;
>>>>>> +
>>>>>> +    data->gctx_paddr = __psp_pa(sev->snp_context);
>>>>>> +    data->req_paddr = __sme_set(req_pfn << PAGE_SHIFT);
>>>>>> +    data->res_paddr = __sme_set(resp_pfn << PAGE_SHIFT);
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static void snp_cleanup_guest_buf(struct 
>>>>>> sev_data_snp_guest_request *data, unsigned long *rc)
>>>>>> +{
>>>>>> +    u64 pfn = __sme_clr(data->res_paddr) >> PAGE_SHIFT;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    ret = snp_page_reclaim(pfn);
>>>>>> +    if (ret)
>>>>>> +        *rc = SEV_RET_INVALID_ADDRESS;
>>>>>> +
>>>>>> +    ret = rmp_make_shared(pfn, PG_LEVEL_4K);
>>>>>> +    if (ret)
>>>>>> +        *rc = SEV_RET_INVALID_ADDRESS;
>>>>>> +}
>>>>>> +
>>>>>> +static void snp_handle_guest_request(struct vcpu_svm *svm, gpa_t 
>>>>>> req_gpa, gpa_t resp_gpa)
>>>>>> +{
>>>>>> +    struct sev_data_snp_guest_request data = {0};
>>>>>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>>>>>> +    struct kvm *kvm = vcpu->kvm;
>>>>>> +    struct kvm_sev_info *sev;
>>>>>> +    unsigned long rc;
>>>>>> +    int err;
>>>>>> +
>>>>>> +    if (!sev_snp_guest(vcpu->kvm)) {
>>>>>> +        rc = SEV_RET_INVALID_GUEST;
>>>>>> +        goto e_fail;
>>>>>> +    }
>>>>>> +
>>>>>> +    sev = &to_kvm_svm(kvm)->sev_info;
>>>>>> +
>>>>>> +    mutex_lock(&sev->guest_req_lock);
>>>>>> +
>>>>>> +    rc = snp_setup_guest_buf(svm, &data, req_gpa, resp_gpa);
>>>>>> +    if (rc)
>>>>>> +        goto unlock;
>>>>>> +
>>>>>> +    rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, &err);
>>>>>
>>>>>
>>>>> This one goes via sev_issue_cmd_external_user() and uses sev-fd...
>>>>>
>>>>>> +    if (rc)
>>>>>> +        /* use the firmware error code */
>>>>>> +        rc = err;
>>>>>> +
>>>>>> +    snp_cleanup_guest_buf(&data, &rc);
>>>>>> +
>>>>>> +unlock:
>>>>>> +    mutex_unlock(&sev->guest_req_lock);
>>>>>> +
>>>>>> +e_fail:
>>>>>> +    svm_set_ghcb_sw_exit_info_2(vcpu, rc);
>>>>>> +}
>>>>>> +
>>>>>> +static void snp_handle_ext_guest_request(struct vcpu_svm *svm, 
>>>>>> gpa_t req_gpa, gpa_t resp_gpa)
>>>>>> +{
>>>>>> +    struct sev_data_snp_guest_request req = {0};
>>>>>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>>>>>> +    struct kvm *kvm = vcpu->kvm;
>>>>>> +    unsigned long data_npages;
>>>>>> +    struct kvm_sev_info *sev;
>>>>>> +    unsigned long rc, err;
>>>>>> +    u64 data_gpa;
>>>>>> +
>>>>>> +    if (!sev_snp_guest(vcpu->kvm)) {
>>>>>> +        rc = SEV_RET_INVALID_GUEST;
>>>>>> +        goto e_fail;
>>>>>> +    }
>>>>>> +
>>>>>> +    sev = &to_kvm_svm(kvm)->sev_info;
>>>>>> +
>>>>>> +    data_gpa = vcpu->arch.regs[VCPU_REGS_RAX];
>>>>>> +    data_npages = vcpu->arch.regs[VCPU_REGS_RBX];
>>>>>> +
>>>>>> +    if (!IS_ALIGNED(data_gpa, PAGE_SIZE)) {
>>>>>> +        rc = SEV_RET_INVALID_ADDRESS;
>>>>>> +        goto e_fail;
>>>>>> +    }
>>>>>> +
>>>>>> +    mutex_lock(&sev->guest_req_lock);
>>>>>> +
>>>>>> +    rc = snp_setup_guest_buf(svm, &req, req_gpa, resp_gpa);
>>>>>> +    if (rc)
>>>>>> +        goto unlock;
>>>>>> +
>>>>>> +    rc = snp_guest_ext_guest_request(&req, (unsigned 
>>>>>> long)sev->snp_certs_data,
>>>>>> +                     &data_npages, &err);
>>>>>
>>>>> but this one does not and jump straight to 
>>>>> drivers/crypto/ccp/sev-dev.c ignoring sev->fd. Why different? Can 
>>>>> these two be unified? sev_issue_cmd_external_user() only checks if 
>>>>> fd is /dev/sev which is hardly useful.
>>>>>
>>>>> "[PATCH RFC v7 32/64] crypto: ccp: Provide APIs to query extended 
>>>>> attestation report" added this one.
>>>>
>>>> SNP_EXT_GUEST_REQUEST additionally returns a certificate blob and 
>>>> that's why it goes through the CCP driver interface 
>>>> snp_guest_ext_guest_request() that is used to get both the report 
>>>> and certificate data/blob at the same time.
>>>
>>> True. I thought though that this calls for extending sev_issue_cmd() 
>>> to take care of these extra parameters rather than just skipping the 
>>> sev->fd.
>>>
>>>
>>>> All the FW API calls on the KVM side go through sev_issue_cmd() and 
>>>> sev_issue_cmd_external_user() interfaces and that i believe uses 
>>>> sev->fd more of as a sanity check.
>>>
>>> Does not look like it:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/crypto/ccp/sev-dev.c?h=v6.2-rc3#n1290 
>>>
>>>
>>> ===
>>> int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd,
>>>                  void *data, int *error)
>>> {
>>>      if (!filep || filep->f_op != &sev_fops)
>>>          return -EBADF;
>>>
>>>      return sev_do_cmd(cmd, data, error);
>>> }
>>> EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
>>> ===
>>>
>>> The only "more" is that it requires sev->fd to be a valid open fd, 
>>> what is the value in that? I may easily miss the bigger picture here. 
>>> Thanks,
>>>
>>>
>>
>> Have a look at following functions in drivers/crypto/ccp/sev-dev.c:
>> sev_dev_init() and sev_misc_init().
>>
>> static int sev_misc_init(struct sev_device *sev)
>> {
>>          struct device *dev = sev->dev;
>>          int ret;
>>
>>          /*
>>           * SEV feature support can be detected on multiple devices but
>>           * the SEV FW commands must be issued on the master. During
>>           * probe, we do not know the master hence we create /dev/sev on
>>           * the first device probe.
>>           * sev_do_cmd() finds the right master device to which to issue
>>           * the command to the firmware.
>>       */
> 
> 
> It is still a single /dev/sev node and the userspace cannot get it 
> wrong, it does not have to choose between (for instance) /dev/sev0 and 
> /dev/sev1 on a 2 SOC system.
> 
>> ...
>> ...
>>
>> Hence, sev_issue_cmd_external_user() needs to ensure that the correct 
>> device (master device) is being operated upon and that's why there is 
>> the check for file operations matching sev_fops as below :
>>
>> int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd,
>>                                  void *data, int *error)
>> {
>>          if (!filep || filep->f_op != &sev_fops)
>>                  return -EBADF;
>> ..
>> ..
>>
>> Essentially, sev->fd is the misc. device created for the master PSP 
>> device on which the SEV/SNP firmware commands are issued, hence,
>> sev_issue_cmd() uses sev->fd.
> 
> There is always just one fd which always uses psp_master, nothing from 
> that fd is used.

It also ensures that we can only issue commands (sev_issue_cmd) after 
SEV/SNP guest has launched. We don't have a valid fd to use before the 
guest launch. The file descriptor is passed as part of the guest launch 
flow, for example, in snp_launch_start().

> 
> More to the point, if sev->fd is still important, why is it ok to skip 
> it for snp_handle_ext_guest_request()? Thanks,
> 
> 
Then, we should do the same for snp_handle_ext_guest_request().

Thanks,
Ashish

Dionna Amalie Glaze Jan. 19, 2023, 8:35 p.m. UTC | #7

> +
> +static void snp_handle_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp_gpa)
> +{

Both regular,

> +
> +static void snp_handle_ext_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp_gpa)
> +{

and extended guest requests should be subject to rate limiting, since
they take a lock on the shared resource that is the AMD-SP (psp?). I
proposed a mechanism with empirically chosen defaults in

[PATCH v2 0/2] kvm: sev: Add SNP guest request throttling
[PATCH v2 1/2] kvm: sev: Add SEV-SNP guest request throttling
[PATCH v2 2/2] kvm: sev: If ccp is busy, report throttled to guest

http://129.79.113.48/hypermail/linux/kernel/2211.2/03107.html
http://129.79.113.48/hypermail/linux/kernel/2211.2/03110.html
http://129.79.113.48/hypermail/linux/kernel/2211.2/03111.html

But I don't see these on lore. Would you like me to repost these?

Kalra, Ashish Jan. 19, 2023, 8:54 p.m. UTC | #8

On 1/19/2023 2:35 PM, Dionna Amalie Glaze wrote:
>> +
>> +static void snp_handle_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp_gpa)
>> +{
> 
> Both regular,
> 
>> +
>> +static void snp_handle_ext_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp_gpa)
>> +{
> 
> and extended guest requests should be subject to rate limiting, since
> they take a lock on the shared resource that is the AMD-SP (psp?). I
> proposed a mechanism with empirically chosen defaults in
> 
> [PATCH v2 0/2] kvm: sev: Add SNP guest request throttling
> [PATCH v2 1/2] kvm: sev: Add SEV-SNP guest request throttling
> [PATCH v2 2/2] kvm: sev: If ccp is busy, report throttled to guest
> 
> http://129.79.113.48/hypermail/linux/kernel/2211.2/03107.html
> http://129.79.113.48/hypermail/linux/kernel/2211.2/03110.html
> http://129.79.113.48/hypermail/linux/kernel/2211.2/03111.html
> 
> But I don't see these on lore. Would you like me to repost these?
> 

Yes, please.

Thanks,
Ashish

Dov Murik Jan. 19, 2023, 9:06 p.m. UTC | #9

On 19/01/2023 22:54, Kalra, Ashish wrote:
> 
> On 1/19/2023 2:35 PM, Dionna Amalie Glaze wrote:
>>> +
>>> +static void snp_handle_guest_request(struct vcpu_svm *svm, gpa_t
>>> req_gpa, gpa_t resp_gpa)
>>> +{
>>
>> Both regular,
>>
>>> +
>>> +static void snp_handle_ext_guest_request(struct vcpu_svm *svm, gpa_t
>>> req_gpa, gpa_t resp_gpa)
>>> +{
>>
>> and extended guest requests should be subject to rate limiting, since
>> they take a lock on the shared resource that is the AMD-SP (psp?). I
>> proposed a mechanism with empirically chosen defaults in
>>
>> [PATCH v2 0/2] kvm: sev: Add SNP guest request throttling
>> [PATCH v2 1/2] kvm: sev: Add SEV-SNP guest request throttling
>> [PATCH v2 2/2] kvm: sev: If ccp is busy, report throttled to guest
>>
>> http://129.79.113.48/hypermail/linux/kernel/2211.2/03107.html
>> http://129.79.113.48/hypermail/linux/kernel/2211.2/03110.html
>> http://129.79.113.48/hypermail/linux/kernel/2211.2/03111.html
>>
>> But I don't see these on lore. Would you like me to repost these?
>>
> 
> Yes, please.
> 

I think it's this series:

https://lore.kernel.org/all/[e-mail]/

-Dov

Alexey Kardashevskiy Jan. 31, 2023, 1:54 a.m. UTC | #10

On 11/1/23 13:01, Kalra, Ashish wrote:
> On 1/10/2023 6:48 PM, Alexey Kardashevskiy wrote:
>> On 10/1/23 19:33, Kalra, Ashish wrote:
>>>
>>> On 1/9/2023 8:28 PM, Alexey Kardashevskiy wrote:
>>>>
>>>>
>>>> On 10/1/23 10:41, Kalra, Ashish wrote:
>>>>> On 1/8/2023 9:33 PM, Alexey Kardashevskiy wrote:
>>>>>> On 15/12/22 06:40, Michael Roth wrote:
>>>>>>> From: Brijesh Singh <[e-mail]>
>>>>>>>
>>>>>>> Version 2 of GHCB specification added the support for two SNP Guest
>>>>>>> Request Message NAE events. The events allows for an SEV-SNP 
>>>>>>> guest to
>>>>>>> make request to the SEV-SNP firmware through hypervisor using the
>>>>>>> SNP_GUEST_REQUEST API define in the SEV-SNP firmware specification.
>>>>>>>
>>>>>>> The SNP_EXT_GUEST_REQUEST is similar to SNP_GUEST_REQUEST with the
>>>>>>> difference of an additional certificate blob that can be passed 
>>>>>>> through
>>>>>>> the SNP_SET_CONFIG ioctl defined in the CCP driver. The CCP driver
>>>>>>> provides snp_guest_ext_guest_request() that is used by the KVM to 
>>>>>>> get
>>>>>>> both the report and certificate data at once.
>>>>>>>
>>>>>>> Signed-off-by: Brijesh Singh <[e-mail]>
>>>>>>> Signed-off-by: Ashish Kalra <[e-mail]>
>>>>>>> Signed-off-by: Michael Roth <[e-mail]>
>>>>>>> ---
>>>>>>>   arch/x86/kvm/svm/sev.c | 185 
>>>>>>> +++++++++++++++++++++++++++++++++++++++--
>>>>>>>   arch/x86/kvm/svm/svm.h |   2 +
>>>>>>>   2 files changed, 181 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>>>>>> index 5f2b2092cdae..18efa70553c2 100644
>>>>>>> --- a/arch/x86/kvm/svm/sev.c
>>>>>>> +++ b/arch/x86/kvm/svm/sev.c
>>>>>>> @@ -331,6 +331,7 @@ static int sev_guest_init(struct kvm *kvm, 
>>>>>>> struct kvm_sev_cmd *argp)
>>>>>>>           if (ret)
>>>>>>>               goto e_free;
>>>>>>> +        mutex_init(&sev->guest_req_lock);
>>>>>>>           ret = sev_snp_init(&argp->error, false);
>>>>>>>       } else {
>>>>>>>           ret = sev_platform_init(&argp->error);
>>>>>>> @@ -2051,23 +2052,34 @@ int sev_vm_move_enc_context_from(struct 
>>>>>>> kvm *kvm, unsigned int source_fd)
>>>>>>>    */
>>>>>>>   static void *snp_context_create(struct kvm *kvm, struct 
>>>>>>> kvm_sev_cmd *argp)
>>>>>>>   {
>>>>>>> +    struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>>>>>>>       struct sev_data_snp_addr data = {};
>>>>>>> -    void *context;
>>>>>>> +    void *context, *certs_data;
>>>>>>>       int rc;
>>>>>>> +    /* Allocate memory used for the certs data in SNP guest 
>>>>>>> request */
>>>>>>> +    certs_data = kzalloc(SEV_FW_BLOB_MAX_SIZE, GFP_KERNEL_ACCOUNT);
>>>>>>> +    if (!certs_data)
>>>>>>> +        return NULL;
>>>>>>> +
>>>>>>>       /* Allocate memory for context page */
>>>>>>>       context = snp_alloc_firmware_page(GFP_KERNEL_ACCOUNT);
>>>>>>>       if (!context)
>>>>>>> -        return NULL;
>>>>>>> +        goto e_free;
>>>>>>>       data.gctx_paddr = __psp_pa(context);
>>>>>>>       rc = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_GCTX_CREATE, 
>>>>>>> &data, &argp->error);
>>>>>>> -    if (rc) {
>>>>>>> -        snp_free_firmware_page(context);
>>>>>>> -        return NULL;
>>>>>>> -    }
>>>>>>> +    if (rc)
>>>>>>> +        goto e_free;
>>>>>>> +
>>>>>>> +    sev->snp_certs_data = certs_data;
>>>>>>>       return context;
>>>>>>> +
>>>>>>> +e_free:
>>>>>>> +    snp_free_firmware_page(context);
>>>>>>> +    kfree(certs_data);
>>>>>>> +    return NULL;
>>>>>>>   }
>>>>>>>   static int snp_bind_asid(struct kvm *kvm, int *error)
>>>>>>> @@ -2653,6 +2665,8 @@ static int snp_decommission_context(struct 
>>>>>>> kvm *kvm)
>>>>>>>       snp_free_firmware_page(sev->snp_context);
>>>>>>>       sev->snp_context = NULL;
>>>>>>> +    kfree(sev->snp_certs_data);
>>>>>>> +
>>>>>>>       return 0;
>>>>>>>   }
>>>>>>> @@ -3174,6 +3188,8 @@ static int sev_es_validate_vmgexit(struct 
>>>>>>> vcpu_svm *svm, u64 *exit_code)
>>>>>>>       case SVM_VMGEXIT_UNSUPPORTED_EVENT:
>>>>>>>       case SVM_VMGEXIT_HV_FEATURES:
>>>>>>>       case SVM_VMGEXIT_PSC:
>>>>>>> +    case SVM_VMGEXIT_GUEST_REQUEST:
>>>>>>> +    case SVM_VMGEXIT_EXT_GUEST_REQUEST:
>>>>>>>           break;
>>>>>>>       default:
>>>>>>>           reason = GHCB_ERR_INVALID_EVENT;
>>>>>>> @@ -3396,6 +3412,149 @@ static int snp_complete_psc(struct 
>>>>>>> kvm_vcpu *vcpu)
>>>>>>>       return 1;
>>>>>>>   }
>>>>>>> +static unsigned long snp_setup_guest_buf(struct vcpu_svm *svm,
>>>>>>> +                     struct sev_data_snp_guest_request *data,
>>>>>>> +                     gpa_t req_gpa, gpa_t resp_gpa)
>>>>>>> +{
>>>>>>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>>>>>>> +    struct kvm *kvm = vcpu->kvm;
>>>>>>> +    kvm_pfn_t req_pfn, resp_pfn;
>>>>>>> +    struct kvm_sev_info *sev;
>>>>>>> +
>>>>>>> +    sev = &to_kvm_svm(kvm)->sev_info;
>>>>>>> +
>>>>>>> +    if (!IS_ALIGNED(req_gpa, PAGE_SIZE) || !IS_ALIGNED(resp_gpa, 
>>>>>>> PAGE_SIZE))
>>>>>>> +        return SEV_RET_INVALID_PARAM;
>>>>>>> +
>>>>>>> +    req_pfn = gfn_to_pfn(kvm, gpa_to_gfn(req_gpa));
>>>>>>> +    if (is_error_noslot_pfn(req_pfn))
>>>>>>> +        return SEV_RET_INVALID_ADDRESS;
>>>>>>> +
>>>>>>> +    resp_pfn = gfn_to_pfn(kvm, gpa_to_gfn(resp_gpa));
>>>>>>> +    if (is_error_noslot_pfn(resp_pfn))
>>>>>>> +        return SEV_RET_INVALID_ADDRESS;
>>>>>>> +
>>>>>>> +    if (rmp_make_private(resp_pfn, 0, PG_LEVEL_4K, 0, true))
>>>>>>> +        return SEV_RET_INVALID_ADDRESS;
>>>>>>> +
>>>>>>> +    data->gctx_paddr = __psp_pa(sev->snp_context);
>>>>>>> +    data->req_paddr = __sme_set(req_pfn << PAGE_SHIFT);
>>>>>>> +    data->res_paddr = __sme_set(resp_pfn << PAGE_SHIFT);
>>>>>>> +
>>>>>>> +    return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void snp_cleanup_guest_buf(struct 
>>>>>>> sev_data_snp_guest_request *data, unsigned long *rc)
>>>>>>> +{
>>>>>>> +    u64 pfn = __sme_clr(data->res_paddr) >> PAGE_SHIFT;
>>>>>>> +    int ret;
>>>>>>> +
>>>>>>> +    ret = snp_page_reclaim(pfn);
>>>>>>> +    if (ret)
>>>>>>> +        *rc = SEV_RET_INVALID_ADDRESS;
>>>>>>> +
>>>>>>> +    ret = rmp_make_shared(pfn, PG_LEVEL_4K);
>>>>>>> +    if (ret)
>>>>>>> +        *rc = SEV_RET_INVALID_ADDRESS;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void snp_handle_guest_request(struct vcpu_svm *svm, gpa_t 
>>>>>>> req_gpa, gpa_t resp_gpa)
>>>>>>> +{
>>>>>>> +    struct sev_data_snp_guest_request data = {0};
>>>>>>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>>>>>>> +    struct kvm *kvm = vcpu->kvm;
>>>>>>> +    struct kvm_sev_info *sev;
>>>>>>> +    unsigned long rc;
>>>>>>> +    int err;
>>>>>>> +
>>>>>>> +    if (!sev_snp_guest(vcpu->kvm)) {
>>>>>>> +        rc = SEV_RET_INVALID_GUEST;
>>>>>>> +        goto e_fail;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    sev = &to_kvm_svm(kvm)->sev_info;
>>>>>>> +
>>>>>>> +    mutex_lock(&sev->guest_req_lock);
>>>>>>> +
>>>>>>> +    rc = snp_setup_guest_buf(svm, &data, req_gpa, resp_gpa);
>>>>>>> +    if (rc)
>>>>>>> +        goto unlock;
>>>>>>> +
>>>>>>> +    rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, 
>>>>>>> &err);
>>>>>>
>>>>>>
>>>>>> This one goes via sev_issue_cmd_external_user() and uses sev-fd...
>>>>>>
>>>>>>> +    if (rc)
>>>>>>> +        /* use the firmware error code */
>>>>>>> +        rc = err;
>>>>>>> +
>>>>>>> +    snp_cleanup_guest_buf(&data, &rc);
>>>>>>> +
>>>>>>> +unlock:
>>>>>>> +    mutex_unlock(&sev->guest_req_lock);
>>>>>>> +
>>>>>>> +e_fail:
>>>>>>> +    svm_set_ghcb_sw_exit_info_2(vcpu, rc);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void snp_handle_ext_guest_request(struct vcpu_svm *svm, 
>>>>>>> gpa_t req_gpa, gpa_t resp_gpa)
>>>>>>> +{
>>>>>>> +    struct sev_data_snp_guest_request req = {0};
>>>>>>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>>>>>>> +    struct kvm *kvm = vcpu->kvm;
>>>>>>> +    unsigned long data_npages;
>>>>>>> +    struct kvm_sev_info *sev;
>>>>>>> +    unsigned long rc, err;
>>>>>>> +    u64 data_gpa;
>>>>>>> +
>>>>>>> +    if (!sev_snp_guest(vcpu->kvm)) {
>>>>>>> +        rc = SEV_RET_INVALID_GUEST;
>>>>>>> +        goto e_fail;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    sev = &to_kvm_svm(kvm)->sev_info;
>>>>>>> +
>>>>>>> +    data_gpa = vcpu->arch.regs[VCPU_REGS_RAX];
>>>>>>> +    data_npages = vcpu->arch.regs[VCPU_REGS_RBX];
>>>>>>> +
>>>>>>> +    if (!IS_ALIGNED(data_gpa, PAGE_SIZE)) {
>>>>>>> +        rc = SEV_RET_INVALID_ADDRESS;
>>>>>>> +        goto e_fail;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    mutex_lock(&sev->guest_req_lock);
>>>>>>> +
>>>>>>> +    rc = snp_setup_guest_buf(svm, &req, req_gpa, resp_gpa);
>>>>>>> +    if (rc)
>>>>>>> +        goto unlock;
>>>>>>> +
>>>>>>> +    rc = snp_guest_ext_guest_request(&req, (unsigned 
>>>>>>> long)sev->snp_certs_data,
>>>>>>> +                     &data_npages, &err);
>>>>>>
>>>>>> but this one does not and jump straight to 
>>>>>> drivers/crypto/ccp/sev-dev.c ignoring sev->fd. Why different? Can 
>>>>>> these two be unified? sev_issue_cmd_external_user() only checks if 
>>>>>> fd is /dev/sev which is hardly useful.
>>>>>>
>>>>>> "[PATCH RFC v7 32/64] crypto: ccp: Provide APIs to query extended 
>>>>>> attestation report" added this one.
>>>>>
>>>>> SNP_EXT_GUEST_REQUEST additionally returns a certificate blob and 
>>>>> that's why it goes through the CCP driver interface 
>>>>> snp_guest_ext_guest_request() that is used to get both the report 
>>>>> and certificate data/blob at the same time.
>>>>
>>>> True. I thought though that this calls for extending sev_issue_cmd() 
>>>> to take care of these extra parameters rather than just skipping the 
>>>> sev->fd.
>>>>
>>>>
>>>>> All the FW API calls on the KVM side go through sev_issue_cmd() and 
>>>>> sev_issue_cmd_external_user() interfaces and that i believe uses 
>>>>> sev->fd more of as a sanity check.
>>>>
>>>> Does not look like it:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/crypto/ccp/sev-dev.c?h=v6.2-rc3#n1290
>>>>
>>>> ===
>>>> int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd,
>>>>                  void *data, int *error)
>>>> {
>>>>      if (!filep || filep->f_op != &sev_fops)
>>>>          return -EBADF;
>>>>
>>>>      return sev_do_cmd(cmd, data, error);
>>>> }
>>>> EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
>>>> ===
>>>>
>>>> The only "more" is that it requires sev->fd to be a valid open fd, 
>>>> what is the value in that? I may easily miss the bigger picture 
>>>> here. Thanks,
>>>>
>>>>
>>>
>>> Have a look at following functions in drivers/crypto/ccp/sev-dev.c:
>>> sev_dev_init() and sev_misc_init().
>>>
>>> static int sev_misc_init(struct sev_device *sev)
>>> {
>>>          struct device *dev = sev->dev;
>>>          int ret;
>>>
>>>          /*
>>>           * SEV feature support can be detected on multiple devices but
>>>           * the SEV FW commands must be issued on the master. During
>>>           * probe, we do not know the master hence we create /dev/sev on
>>>           * the first device probe.
>>>           * sev_do_cmd() finds the right master device to which to issue
>>>           * the command to the firmware.
>>>       */
>>
>>
>> It is still a single /dev/sev node and the userspace cannot get it 
>> wrong, it does not have to choose between (for instance) /dev/sev0 and 
>> /dev/sev1 on a 2 SOC system.
>>
>>> ...
>>> ...
>>>
>>> Hence, sev_issue_cmd_external_user() needs to ensure that the correct 
>>> device (master device) is being operated upon and that's why there is 
>>> the check for file operations matching sev_fops as below :
>>>
>>> int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd,
>>>                                  void *data, int *error)
>>> {
>>>          if (!filep || filep->f_op != &sev_fops)
>>>                  return -EBADF;
>>> ..
>>> ..
>>>
>>> Essentially, sev->fd is the misc. device created for the master PSP 
>>> device on which the SEV/SNP firmware commands are issued, hence,
>>> sev_issue_cmd() uses sev->fd.
>>
>> There is always just one fd which always uses psp_master, nothing from 
>> that fd is used.
> 
> It also ensures that we can only issue commands (sev_issue_cmd) after 
> SEV/SNP guest has launched.

I can open /dev/sev and start sending commands to the firmware with no 
KVM running at all. Oh well, we discussed this offline :)

> We don't have a valid fd to use before the 
> guest launch. The file descriptor is passed as part of the guest launch 
> flow, for example, in snp_launch_start().
>>
>> More to the point, if sev->fd is still important, why is it ok to skip 
>> it for snp_handle_ext_guest_request()? Thanks,
>>
>>
> Then, we should do the same for snp_handle_ext_guest_request().

Okay.

This snp_handle_ext_guest_request() helper is for returning "Table 21. 
ATTESTATION_REPORT Structure" along with the certificate(s) used to sign 
the report: "This usage allows the attestation report and the 
certificates required to verify the report to be returned at the same time".

I can see:
1) KVM_SEV_SNP_{G,S}ET_CERTS ioctls on KVM VM and
2) SNP_{SET,GET}_EXT_CONFIG ioctls on /dev/sev
Both store the passed blob and neither communicate it to the firmware. 
This makes me wonder - how does the attestation report (cooked by the 
firmware) get signed with those certificates passed on by the HV userspace?

Also, the cached blob in /dev/sev seems redundand - the attestation 
report is retuned for a specific guest so having a blob in the KVM VM 
makes sense and KVM unconditionally reserves memory for it anyway. And 
for the HV itself the blob is useless (?) so why bother with caching it 
in /dev/sev.

And GET ioctls() return what SET passed on (not something the firware 
returned, for example), what is ever going to call SET? The userspace 
can as well cache what it passed and save a bit of the code/memory in 
the kernel.

btw SNP_{SET,GET}_EXT_CONFIG are documented in 
Documentation/virt/coco/sev-guest.rst but implemented in 
drivers/crypto/ccp/sev-dev.c (not sev-guest.c).

What do I miss in the big picture here? :) Thanks,

Tom Lendacky Jan. 31, 2023, 4:23 p.m. UTC | #11

On 1/30/23 19:54, Alexey Kardashevskiy wrote:
> 
> 
> On 11/1/23 13:01, Kalra, Ashish wrote:
>> On 1/10/2023 6:48 PM, Alexey Kardashevskiy wrote:
>>> On 10/1/23 19:33, Kalra, Ashish wrote:
>>>>
>>>> On 1/9/2023 8:28 PM, Alexey Kardashevskiy wrote:
>>>>>
>>>>>
>>>>> On 10/1/23 10:41, Kalra, Ashish wrote:
>>>>>> On 1/8/2023 9:33 PM, Alexey Kardashevskiy wrote:
>>>>>>> On 15/12/22 06:40, Michael Roth wrote:
>>>>>>>> From: Brijesh Singh <[e-mail]>
>>>>>>>>
>>>>>>>> Version 2 of GHCB specification added the support for two SNP Guest
>>>>>>>> Request Message NAE events. The events allows for an SEV-SNP guest to
>>>>>>>> make request to the SEV-SNP firmware through hypervisor using the
>>>>>>>> SNP_GUEST_REQUEST API define in the SEV-SNP firmware specification.
>>>>>>>>
>>>>>>>> The SNP_EXT_GUEST_REQUEST is similar to SNP_GUEST_REQUEST with the
>>>>>>>> difference of an additional certificate blob that can be passed 
>>>>>>>> through
>>>>>>>> the SNP_SET_CONFIG ioctl defined in the CCP driver. The CCP driver
>>>>>>>> provides snp_guest_ext_guest_request() that is used by the KVM to get
>>>>>>>> both the report and certificate data at once.
>>>>>>>>
>>>>>>>> Signed-off-by: Brijesh Singh <[e-mail]>
>>>>>>>> Signed-off-by: Ashish Kalra <[e-mail]>
>>>>>>>> Signed-off-by: Michael Roth <[e-mail]>
>>>>>>>> ---
>>>>>>>>   arch/x86/kvm/svm/sev.c | 185 
>>>>>>>> +++++++++++++++++++++++++++++++++++++++--
>>>>>>>>   arch/x86/kvm/svm/svm.h |   2 +
>>>>>>>>   2 files changed, 181 insertions(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>>>>>>> index 5f2b2092cdae..18efa70553c2 100644
>>>>>>>> --- a/arch/x86/kvm/svm/sev.c
>>>>>>>> +++ b/arch/x86/kvm/svm/sev.c
>>>>>>>> @@ -331,6 +331,7 @@ static int sev_guest_init(struct kvm *kvm, 
>>>>>>>> struct kvm_sev_cmd *argp)
>>>>>>>>           if (ret)
>>>>>>>>               goto e_free;
>>>>>>>> +        mutex_init(&sev->guest_req_lock);
>>>>>>>>           ret = sev_snp_init(&argp->error, false);
>>>>>>>>       } else {
>>>>>>>>           ret = sev_platform_init(&argp->error);
>>>>>>>> @@ -2051,23 +2052,34 @@ int sev_vm_move_enc_context_from(struct 
>>>>>>>> kvm *kvm, unsigned int source_fd)
>>>>>>>>    */
>>>>>>>>   static void *snp_context_create(struct kvm *kvm, struct 
>>>>>>>> kvm_sev_cmd *argp)
>>>>>>>>   {
>>>>>>>> +    struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>>>>>>>>       struct sev_data_snp_addr data = {};
>>>>>>>> -    void *context;
>>>>>>>> +    void *context, *certs_data;
>>>>>>>>       int rc;
>>>>>>>> +    /* Allocate memory used for the certs data in SNP guest 
>>>>>>>> request */
>>>>>>>> +    certs_data = kzalloc(SEV_FW_BLOB_MAX_SIZE, GFP_KERNEL_ACCOUNT);
>>>>>>>> +    if (!certs_data)
>>>>>>>> +        return NULL;
>>>>>>>> +
>>>>>>>>       /* Allocate memory for context page */
>>>>>>>>       context = snp_alloc_firmware_page(GFP_KERNEL_ACCOUNT);
>>>>>>>>       if (!context)
>>>>>>>> -        return NULL;
>>>>>>>> +        goto e_free;
>>>>>>>>       data.gctx_paddr = __psp_pa(context);
>>>>>>>>       rc = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_GCTX_CREATE, 
>>>>>>>> &data, &argp->error);
>>>>>>>> -    if (rc) {
>>>>>>>> -        snp_free_firmware_page(context);
>>>>>>>> -        return NULL;
>>>>>>>> -    }
>>>>>>>> +    if (rc)
>>>>>>>> +        goto e_free;
>>>>>>>> +
>>>>>>>> +    sev->snp_certs_data = certs_data;
>>>>>>>>       return context;
>>>>>>>> +
>>>>>>>> +e_free:
>>>>>>>> +    snp_free_firmware_page(context);
>>>>>>>> +    kfree(certs_data);
>>>>>>>> +    return NULL;
>>>>>>>>   }
>>>>>>>>   static int snp_bind_asid(struct kvm *kvm, int *error)
>>>>>>>> @@ -2653,6 +2665,8 @@ static int snp_decommission_context(struct 
>>>>>>>> kvm *kvm)
>>>>>>>>       snp_free_firmware_page(sev->snp_context);
>>>>>>>>       sev->snp_context = NULL;
>>>>>>>> +    kfree(sev->snp_certs_data);
>>>>>>>> +
>>>>>>>>       return 0;
>>>>>>>>   }
>>>>>>>> @@ -3174,6 +3188,8 @@ static int sev_es_validate_vmgexit(struct 
>>>>>>>> vcpu_svm *svm, u64 *exit_code)
>>>>>>>>       case SVM_VMGEXIT_UNSUPPORTED_EVENT:
>>>>>>>>       case SVM_VMGEXIT_HV_FEATURES:
>>>>>>>>       case SVM_VMGEXIT_PSC:
>>>>>>>> +    case SVM_VMGEXIT_GUEST_REQUEST:
>>>>>>>> +    case SVM_VMGEXIT_EXT_GUEST_REQUEST:
>>>>>>>>           break;
>>>>>>>>       default:
>>>>>>>>           reason = GHCB_ERR_INVALID_EVENT;
>>>>>>>> @@ -3396,6 +3412,149 @@ static int snp_complete_psc(struct 
>>>>>>>> kvm_vcpu *vcpu)
>>>>>>>>       return 1;
>>>>>>>>   }
>>>>>>>> +static unsigned long snp_setup_guest_buf(struct vcpu_svm *svm,
>>>>>>>> +                     struct sev_data_snp_guest_request *data,
>>>>>>>> +                     gpa_t req_gpa, gpa_t resp_gpa)
>>>>>>>> +{
>>>>>>>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>>>>>>>> +    struct kvm *kvm = vcpu->kvm;
>>>>>>>> +    kvm_pfn_t req_pfn, resp_pfn;
>>>>>>>> +    struct kvm_sev_info *sev;
>>>>>>>> +
>>>>>>>> +    sev = &to_kvm_svm(kvm)->sev_info;
>>>>>>>> +
>>>>>>>> +    if (!IS_ALIGNED(req_gpa, PAGE_SIZE) || !IS_ALIGNED(resp_gpa, 
>>>>>>>> PAGE_SIZE))
>>>>>>>> +        return SEV_RET_INVALID_PARAM;
>>>>>>>> +
>>>>>>>> +    req_pfn = gfn_to_pfn(kvm, gpa_to_gfn(req_gpa));
>>>>>>>> +    if (is_error_noslot_pfn(req_pfn))
>>>>>>>> +        return SEV_RET_INVALID_ADDRESS;
>>>>>>>> +
>>>>>>>> +    resp_pfn = gfn_to_pfn(kvm, gpa_to_gfn(resp_gpa));
>>>>>>>> +    if (is_error_noslot_pfn(resp_pfn))
>>>>>>>> +        return SEV_RET_INVALID_ADDRESS;
>>>>>>>> +
>>>>>>>> +    if (rmp_make_private(resp_pfn, 0, PG_LEVEL_4K, 0, true))
>>>>>>>> +        return SEV_RET_INVALID_ADDRESS;
>>>>>>>> +
>>>>>>>> +    data->gctx_paddr = __psp_pa(sev->snp_context);
>>>>>>>> +    data->req_paddr = __sme_set(req_pfn << PAGE_SHIFT);
>>>>>>>> +    data->res_paddr = __sme_set(resp_pfn << PAGE_SHIFT);
>>>>>>>> +
>>>>>>>> +    return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static void snp_cleanup_guest_buf(struct 
>>>>>>>> sev_data_snp_guest_request *data, unsigned long *rc)
>>>>>>>> +{
>>>>>>>> +    u64 pfn = __sme_clr(data->res_paddr) >> PAGE_SHIFT;
>>>>>>>> +    int ret;
>>>>>>>> +
>>>>>>>> +    ret = snp_page_reclaim(pfn);
>>>>>>>> +    if (ret)
>>>>>>>> +        *rc = SEV_RET_INVALID_ADDRESS;
>>>>>>>> +
>>>>>>>> +    ret = rmp_make_shared(pfn, PG_LEVEL_4K);
>>>>>>>> +    if (ret)
>>>>>>>> +        *rc = SEV_RET_INVALID_ADDRESS;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static void snp_handle_guest_request(struct vcpu_svm *svm, gpa_t 
>>>>>>>> req_gpa, gpa_t resp_gpa)
>>>>>>>> +{
>>>>>>>> +    struct sev_data_snp_guest_request data = {0};
>>>>>>>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>>>>>>>> +    struct kvm *kvm = vcpu->kvm;
>>>>>>>> +    struct kvm_sev_info *sev;
>>>>>>>> +    unsigned long rc;
>>>>>>>> +    int err;
>>>>>>>> +
>>>>>>>> +    if (!sev_snp_guest(vcpu->kvm)) {
>>>>>>>> +        rc = SEV_RET_INVALID_GUEST;
>>>>>>>> +        goto e_fail;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    sev = &to_kvm_svm(kvm)->sev_info;
>>>>>>>> +
>>>>>>>> +    mutex_lock(&sev->guest_req_lock);
>>>>>>>> +
>>>>>>>> +    rc = snp_setup_guest_buf(svm, &data, req_gpa, resp_gpa);
>>>>>>>> +    if (rc)
>>>>>>>> +        goto unlock;
>>>>>>>> +
>>>>>>>> +    rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, &err);
>>>>>>>
>>>>>>>
>>>>>>> This one goes via sev_issue_cmd_external_user() and uses sev-fd...
>>>>>>>
>>>>>>>> +    if (rc)
>>>>>>>> +        /* use the firmware error code */
>>>>>>>> +        rc = err;
>>>>>>>> +
>>>>>>>> +    snp_cleanup_guest_buf(&data, &rc);
>>>>>>>> +
>>>>>>>> +unlock:
>>>>>>>> +    mutex_unlock(&sev->guest_req_lock);
>>>>>>>> +
>>>>>>>> +e_fail:
>>>>>>>> +    svm_set_ghcb_sw_exit_info_2(vcpu, rc);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static void snp_handle_ext_guest_request(struct vcpu_svm *svm, 
>>>>>>>> gpa_t req_gpa, gpa_t resp_gpa)
>>>>>>>> +{
>>>>>>>> +    struct sev_data_snp_guest_request req = {0};
>>>>>>>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>>>>>>>> +    struct kvm *kvm = vcpu->kvm;
>>>>>>>> +    unsigned long data_npages;
>>>>>>>> +    struct kvm_sev_info *sev;
>>>>>>>> +    unsigned long rc, err;
>>>>>>>> +    u64 data_gpa;
>>>>>>>> +
>>>>>>>> +    if (!sev_snp_guest(vcpu->kvm)) {
>>>>>>>> +        rc = SEV_RET_INVALID_GUEST;
>>>>>>>> +        goto e_fail;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    sev = &to_kvm_svm(kvm)->sev_info;
>>>>>>>> +
>>>>>>>> +    data_gpa = vcpu->arch.regs[VCPU_REGS_RAX];
>>>>>>>> +    data_npages = vcpu->arch.regs[VCPU_REGS_RBX];
>>>>>>>> +
>>>>>>>> +    if (!IS_ALIGNED(data_gpa, PAGE_SIZE)) {
>>>>>>>> +        rc = SEV_RET_INVALID_ADDRESS;
>>>>>>>> +        goto e_fail;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    mutex_lock(&sev->guest_req_lock);
>>>>>>>> +
>>>>>>>> +    rc = snp_setup_guest_buf(svm, &req, req_gpa, resp_gpa);
>>>>>>>> +    if (rc)
>>>>>>>> +        goto unlock;
>>>>>>>> +
>>>>>>>> +    rc = snp_guest_ext_guest_request(&req, (unsigned 
>>>>>>>> long)sev->snp_certs_data,
>>>>>>>> +                     &data_npages, &err);
>>>>>>>
>>>>>>> but this one does not and jump straight to 
>>>>>>> drivers/crypto/ccp/sev-dev.c ignoring sev->fd. Why different? Can 
>>>>>>> these two be unified? sev_issue_cmd_external_user() only checks if 
>>>>>>> fd is /dev/sev which is hardly useful.
>>>>>>>
>>>>>>> "[PATCH RFC v7 32/64] crypto: ccp: Provide APIs to query extended 
>>>>>>> attestation report" added this one.
>>>>>>
>>>>>> SNP_EXT_GUEST_REQUEST additionally returns a certificate blob and 
>>>>>> that's why it goes through the CCP driver interface 
>>>>>> snp_guest_ext_guest_request() that is used to get both the report 
>>>>>> and certificate data/blob at the same time.
>>>>>
>>>>> True. I thought though that this calls for extending sev_issue_cmd() 
>>>>> to take care of these extra parameters rather than just skipping the 
>>>>> sev->fd.
>>>>>
>>>>>
>>>>>> All the FW API calls on the KVM side go through sev_issue_cmd() and 
>>>>>> sev_issue_cmd_external_user() interfaces and that i believe uses 
>>>>>> sev->fd more of as a sanity check.
>>>>>
>>>>> Does not look like it:
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/crypto/ccp/sev-dev.c?h=v6.2-rc3#n1290
>>>>>
>>>>> ===
>>>>> int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd,
>>>>>                  void *data, int *error)
>>>>> {
>>>>>      if (!filep || filep->f_op != &sev_fops)
>>>>>          return -EBADF;
>>>>>
>>>>>      return sev_do_cmd(cmd, data, error);
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
>>>>> ===
>>>>>
>>>>> The only "more" is that it requires sev->fd to be a valid open fd, 
>>>>> what is the value in that? I may easily miss the bigger picture here. 
>>>>> Thanks,
>>>>>
>>>>>
>>>>
>>>> Have a look at following functions in drivers/crypto/ccp/sev-dev.c:
>>>> sev_dev_init() and sev_misc_init().
>>>>
>>>> static int sev_misc_init(struct sev_device *sev)
>>>> {
>>>>          struct device *dev = sev->dev;
>>>>          int ret;
>>>>
>>>>          /*
>>>>           * SEV feature support can be detected on multiple devices but
>>>>           * the SEV FW commands must be issued on the master. During
>>>>           * probe, we do not know the master hence we create /dev/sev on
>>>>           * the first device probe.
>>>>           * sev_do_cmd() finds the right master device to which to issue
>>>>           * the command to the firmware.
>>>>       */
>>>
>>>
>>> It is still a single /dev/sev node and the userspace cannot get it 
>>> wrong, it does not have to choose between (for instance) /dev/sev0 and 
>>> /dev/sev1 on a 2 SOC system.
>>>
>>>> ...
>>>> ...
>>>>
>>>> Hence, sev_issue_cmd_external_user() needs to ensure that the correct 
>>>> device (master device) is being operated upon and that's why there is 
>>>> the check for file operations matching sev_fops as below :
>>>>
>>>> int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd,
>>>>                                  void *data, int *error)
>>>> {
>>>>          if (!filep || filep->f_op != &sev_fops)
>>>>                  return -EBADF;
>>>> ..
>>>> ..
>>>>
>>>> Essentially, sev->fd is the misc. device created for the master PSP 
>>>> device on which the SEV/SNP firmware commands are issued, hence,
>>>> sev_issue_cmd() uses sev->fd.
>>>
>>> There is always just one fd which always uses psp_master, nothing from 
>>> that fd is used.
>>
>> It also ensures that we can only issue commands (sev_issue_cmd) after 
>> SEV/SNP guest has launched.
> 
> I can open /dev/sev and start sending commands to the firmware with no KVM 
> running at all. Oh well, we discussed this offline :)
> 
>> We don't have a valid fd to use before the guest launch. The file 
>> descriptor is passed as part of the guest launch flow, for example, in 
>> snp_launch_start().
>>>
>>> More to the point, if sev->fd is still important, why is it ok to skip 
>>> it for snp_handle_ext_guest_request()? Thanks,
>>>
>>>
>> Then, we should do the same for snp_handle_ext_guest_request().
> 
> Okay.
> 
> This snp_handle_ext_guest_request() helper is for returning "Table 21. 
> ATTESTATION_REPORT Structure" along with the certificate(s) used to sign 
> the report: "This usage allows the attestation report and the certificates 
> required to verify the report to be returned at the same time".
> 
> I can see:
> 1) KVM_SEV_SNP_{G,S}ET_CERTS ioctls on KVM VM and

This allows the VMM to (optionally) supply per-VM certificates that the 
guest can use to validate the attestation report, instead of the guest 
requesting separately.

> 2) SNP_{SET,GET}_EXT_CONFIG ioctls on /dev/sev

This allows the VMM to (optionally) supply certificates used for all VMs, 
i.e., there is no need for per-VM certificates.

> Both store the passed blob and neither communicate it to the firmware. 
> This makes me wonder - how does the attestation report (cooked by the 
> firmware) get signed with those certificates passed on by the HV userspace?

These are for use by the guest to validate the attestation report. It 
allows the guest to obtain the certificate information without having to 
use another method to request the certificates.

By having this certificate store, the hypervisor can request the 
certificates from the KDS once, rather than every time a guest requests an 
attestation report.

> 
> Also, the cached blob in /dev/sev seems redundand - the attestation report 
> is retuned for a specific guest so having a blob in the KVM VM makes sense 
> and KVM unconditionally reserves memory for it anyway. And for the HV 
> itself the blob is useless (?) so why bother with caching it in /dev/sev.

In general, the certificates are for the machine (VCEK, ASK, ARK), so they 
can be for all VMs on the machine. The per-VM blob allows a VMM to supply 
additional per-VM certficates, if it desires, but is not required.

> 
> And GET ioctls() return what SET passed on (not something the firware 
> returned, for example), what is ever going to call SET? The userspace can 

As stated above, the firmware already has the information needed to sign 
the attestation report. The SET IOCTL is used to supply the certficates to 
the guest for validation of the attestation report. This reduces the 
traffic and complexity of the guest requesting the certficates from the KDS.

> as well cache what it passed and save a bit of the code/memory in the kernel.
> 
> btw SNP_{SET,GET}_EXT_CONFIG are documented in 
> Documentation/virt/coco/sev-guest.rst but implemented in 
> drivers/crypto/ccp/sev-dev.c (not sev-guest.c).
> 
> What do I miss in the big picture here? :) Thanks,

The reason for the extended request is to make the attestation request 
appear atomic to the guest. If you had to make two calls to request the 
information, in the future, when live migration is possible, there is no 
guarantee that the guest couldn't have been migrated in between the calls 
to obtain the certificates and the call to obtain the attestation report 
and thus validation of the attestation report could fail.

Thanks,
Tom

> 
>

Kalra, Ashish Jan. 31, 2023, 5:52 p.m. UTC | #12

On 1/30/2023 7:54 PM, Alexey Kardashevskiy wrote:
> 
> 
> On 11/1/23 13:01, Kalra, Ashish wrote:
>> On 1/10/2023 6:48 PM, Alexey Kardashevskiy wrote:
>>> On 10/1/23 19:33, Kalra, Ashish wrote:
>>>>
>>>> On 1/9/2023 8:28 PM, Alexey Kardashevskiy wrote:
>>>>>
>>>>>
>>>>> On 10/1/23 10:41, Kalra, Ashish wrote:
>>>>>> On 1/8/2023 9:33 PM, Alexey Kardashevskiy wrote:
>>>>>>> On 15/12/22 06:40, Michael Roth wrote:
>>>>>>>> From: Brijesh Singh <[e-mail]>
>>>>>>>>
>>>>>>>> Version 2 of GHCB specification added the support for two SNP Guest
>>>>>>>> Request Message NAE events. The events allows for an SEV-SNP 
>>>>>>>> guest to
>>>>>>>> make request to the SEV-SNP firmware through hypervisor using the
>>>>>>>> SNP_GUEST_REQUEST API define in the SEV-SNP firmware specification.
>>>>>>>>
>>>>>>>> The SNP_EXT_GUEST_REQUEST is similar to SNP_GUEST_REQUEST with the
>>>>>>>> difference of an additional certificate blob that can be passed 
>>>>>>>> through
>>>>>>>> the SNP_SET_CONFIG ioctl defined in the CCP driver. The CCP driver
>>>>>>>> provides snp_guest_ext_guest_request() that is used by the KVM 
>>>>>>>> to get
>>>>>>>> both the report and certificate data at once.
>>>>>>>>
>>>>>>>> Signed-off-by: Brijesh Singh <[e-mail]>
>>>>>>>> Signed-off-by: Ashish Kalra <[e-mail]>
>>>>>>>> Signed-off-by: Michael Roth <[e-mail]>
>>>>>>>> ---
>>>>>>>>   arch/x86/kvm/svm/sev.c | 185 
>>>>>>>> +++++++++++++++++++++++++++++++++++++++--
>>>>>>>>   arch/x86/kvm/svm/svm.h |   2 +
>>>>>>>>   2 files changed, 181 insertions(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>>>>>>> index 5f2b2092cdae..18efa70553c2 100644
>>>>>>>> --- a/arch/x86/kvm/svm/sev.c
>>>>>>>> +++ b/arch/x86/kvm/svm/sev.c
>>>>>>>> @@ -331,6 +331,7 @@ static int sev_guest_init(struct kvm *kvm, 
>>>>>>>> struct kvm_sev_cmd *argp)
>>>>>>>>           if (ret)
>>>>>>>>               goto e_free;
>>>>>>>> +        mutex_init(&sev->guest_req_lock);
>>>>>>>>           ret = sev_snp_init(&argp->error, false);
>>>>>>>>       } else {
>>>>>>>>           ret = sev_platform_init(&argp->error);
>>>>>>>> @@ -2051,23 +2052,34 @@ int sev_vm_move_enc_context_from(struct 
>>>>>>>> kvm *kvm, unsigned int source_fd)
>>>>>>>>    */
>>>>>>>>   static void *snp_context_create(struct kvm *kvm, struct 
>>>>>>>> kvm_sev_cmd *argp)
>>>>>>>>   {
>>>>>>>> +    struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>>>>>>>>       struct sev_data_snp_addr data = {};
>>>>>>>> -    void *context;
>>>>>>>> +    void *context, *certs_data;
>>>>>>>>       int rc;
>>>>>>>> +    /* Allocate memory used for the certs data in SNP guest 
>>>>>>>> request */
>>>>>>>> +    certs_data = kzalloc(SEV_FW_BLOB_MAX_SIZE, 
>>>>>>>> GFP_KERNEL_ACCOUNT);
>>>>>>>> +    if (!certs_data)
>>>>>>>> +        return NULL;
>>>>>>>> +
>>>>>>>>       /* Allocate memory for context page */
>>>>>>>>       context = snp_alloc_firmware_page(GFP_KERNEL_ACCOUNT);
>>>>>>>>       if (!context)
>>>>>>>> -        return NULL;
>>>>>>>> +        goto e_free;
>>>>>>>>       data.gctx_paddr = __psp_pa(context);
>>>>>>>>       rc = __sev_issue_cmd(argp->sev_fd, 
>>>>>>>> SEV_CMD_SNP_GCTX_CREATE, &data, &argp->error);
>>>>>>>> -    if (rc) {
>>>>>>>> -        snp_free_firmware_page(context);
>>>>>>>> -        return NULL;
>>>>>>>> -    }
>>>>>>>> +    if (rc)
>>>>>>>> +        goto e_free;
>>>>>>>> +
>>>>>>>> +    sev->snp_certs_data = certs_data;
>>>>>>>>       return context;
>>>>>>>> +
>>>>>>>> +e_free:
>>>>>>>> +    snp_free_firmware_page(context);
>>>>>>>> +    kfree(certs_data);
>>>>>>>> +    return NULL;
>>>>>>>>   }
>>>>>>>>   static int snp_bind_asid(struct kvm *kvm, int *error)
>>>>>>>> @@ -2653,6 +2665,8 @@ static int snp_decommission_context(struct 
>>>>>>>> kvm *kvm)
>>>>>>>>       snp_free_firmware_page(sev->snp_context);
>>>>>>>>       sev->snp_context = NULL;
>>>>>>>> +    kfree(sev->snp_certs_data);
>>>>>>>> +
>>>>>>>>       return 0;
>>>>>>>>   }
>>>>>>>> @@ -3174,6 +3188,8 @@ static int sev_es_validate_vmgexit(struct 
>>>>>>>> vcpu_svm *svm, u64 *exit_code)
>>>>>>>>       case SVM_VMGEXIT_UNSUPPORTED_EVENT:
>>>>>>>>       case SVM_VMGEXIT_HV_FEATURES:
>>>>>>>>       case SVM_VMGEXIT_PSC:
>>>>>>>> +    case SVM_VMGEXIT_GUEST_REQUEST:
>>>>>>>> +    case SVM_VMGEXIT_EXT_GUEST_REQUEST:
>>>>>>>>           break;
>>>>>>>>       default:
>>>>>>>>           reason = GHCB_ERR_INVALID_EVENT;
>>>>>>>> @@ -3396,6 +3412,149 @@ static int snp_complete_psc(struct 
>>>>>>>> kvm_vcpu *vcpu)
>>>>>>>>       return 1;
>>>>>>>>   }
>>>>>>>> +static unsigned long snp_setup_guest_buf(struct vcpu_svm *svm,
>>>>>>>> +                     struct sev_data_snp_guest_request *data,
>>>>>>>> +                     gpa_t req_gpa, gpa_t resp_gpa)
>>>>>>>> +{
>>>>>>>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>>>>>>>> +    struct kvm *kvm = vcpu->kvm;
>>>>>>>> +    kvm_pfn_t req_pfn, resp_pfn;
>>>>>>>> +    struct kvm_sev_info *sev;
>>>>>>>> +
>>>>>>>> +    sev = &to_kvm_svm(kvm)->sev_info;
>>>>>>>> +
>>>>>>>> +    if (!IS_ALIGNED(req_gpa, PAGE_SIZE) || 
>>>>>>>> !IS_ALIGNED(resp_gpa, PAGE_SIZE))
>>>>>>>> +        return SEV_RET_INVALID_PARAM;
>>>>>>>> +
>>>>>>>> +    req_pfn = gfn_to_pfn(kvm, gpa_to_gfn(req_gpa));
>>>>>>>> +    if (is_error_noslot_pfn(req_pfn))
>>>>>>>> +        return SEV_RET_INVALID_ADDRESS;
>>>>>>>> +
>>>>>>>> +    resp_pfn = gfn_to_pfn(kvm, gpa_to_gfn(resp_gpa));
>>>>>>>> +    if (is_error_noslot_pfn(resp_pfn))
>>>>>>>> +        return SEV_RET_INVALID_ADDRESS;
>>>>>>>> +
>>>>>>>> +    if (rmp_make_private(resp_pfn, 0, PG_LEVEL_4K, 0, true))
>>>>>>>> +        return SEV_RET_INVALID_ADDRESS;
>>>>>>>> +
>>>>>>>> +    data->gctx_paddr = __psp_pa(sev->snp_context);
>>>>>>>> +    data->req_paddr = __sme_set(req_pfn << PAGE_SHIFT);
>>>>>>>> +    data->res_paddr = __sme_set(resp_pfn << PAGE_SHIFT);
>>>>>>>> +
>>>>>>>> +    return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static void snp_cleanup_guest_buf(struct 
>>>>>>>> sev_data_snp_guest_request *data, unsigned long *rc)
>>>>>>>> +{
>>>>>>>> +    u64 pfn = __sme_clr(data->res_paddr) >> PAGE_SHIFT;
>>>>>>>> +    int ret;
>>>>>>>> +
>>>>>>>> +    ret = snp_page_reclaim(pfn);
>>>>>>>> +    if (ret)
>>>>>>>> +        *rc = SEV_RET_INVALID_ADDRESS;
>>>>>>>> +
>>>>>>>> +    ret = rmp_make_shared(pfn, PG_LEVEL_4K);
>>>>>>>> +    if (ret)
>>>>>>>> +        *rc = SEV_RET_INVALID_ADDRESS;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static void snp_handle_guest_request(struct vcpu_svm *svm, 
>>>>>>>> gpa_t req_gpa, gpa_t resp_gpa)
>>>>>>>> +{
>>>>>>>> +    struct sev_data_snp_guest_request data = {0};
>>>>>>>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>>>>>>>> +    struct kvm *kvm = vcpu->kvm;
>>>>>>>> +    struct kvm_sev_info *sev;
>>>>>>>> +    unsigned long rc;
>>>>>>>> +    int err;
>>>>>>>> +
>>>>>>>> +    if (!sev_snp_guest(vcpu->kvm)) {
>>>>>>>> +        rc = SEV_RET_INVALID_GUEST;
>>>>>>>> +        goto e_fail;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    sev = &to_kvm_svm(kvm)->sev_info;
>>>>>>>> +
>>>>>>>> +    mutex_lock(&sev->guest_req_lock);
>>>>>>>> +
>>>>>>>> +    rc = snp_setup_guest_buf(svm, &data, req_gpa, resp_gpa);
>>>>>>>> +    if (rc)
>>>>>>>> +        goto unlock;
>>>>>>>> +
>>>>>>>> +    rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, 
>>>>>>>> &err);
>>>>>>>
>>>>>>>
>>>>>>> This one goes via sev_issue_cmd_external_user() and uses sev-fd...
>>>>>>>
>>>>>>>> +    if (rc)
>>>>>>>> +        /* use the firmware error code */
>>>>>>>> +        rc = err;
>>>>>>>> +
>>>>>>>> +    snp_cleanup_guest_buf(&data, &rc);
>>>>>>>> +
>>>>>>>> +unlock:
>>>>>>>> +    mutex_unlock(&sev->guest_req_lock);
>>>>>>>> +
>>>>>>>> +e_fail:
>>>>>>>> +    svm_set_ghcb_sw_exit_info_2(vcpu, rc);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static void snp_handle_ext_guest_request(struct vcpu_svm *svm, 
>>>>>>>> gpa_t req_gpa, gpa_t resp_gpa)
>>>>>>>> +{
>>>>>>>> +    struct sev_data_snp_guest_request req = {0};
>>>>>>>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>>>>>>>> +    struct kvm *kvm = vcpu->kvm;
>>>>>>>> +    unsigned long data_npages;
>>>>>>>> +    struct kvm_sev_info *sev;
>>>>>>>> +    unsigned long rc, err;
>>>>>>>> +    u64 data_gpa;
>>>>>>>> +
>>>>>>>> +    if (!sev_snp_guest(vcpu->kvm)) {
>>>>>>>> +        rc = SEV_RET_INVALID_GUEST;
>>>>>>>> +        goto e_fail;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    sev = &to_kvm_svm(kvm)->sev_info;
>>>>>>>> +
>>>>>>>> +    data_gpa = vcpu->arch.regs[VCPU_REGS_RAX];
>>>>>>>> +    data_npages = vcpu->arch.regs[VCPU_REGS_RBX];
>>>>>>>> +
>>>>>>>> +    if (!IS_ALIGNED(data_gpa, PAGE_SIZE)) {
>>>>>>>> +        rc = SEV_RET_INVALID_ADDRESS;
>>>>>>>> +        goto e_fail;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    mutex_lock(&sev->guest_req_lock);
>>>>>>>> +
>>>>>>>> +    rc = snp_setup_guest_buf(svm, &req, req_gpa, resp_gpa);
>>>>>>>> +    if (rc)
>>>>>>>> +        goto unlock;
>>>>>>>> +
>>>>>>>> +    rc = snp_guest_ext_guest_request(&req, (unsigned 
>>>>>>>> long)sev->snp_certs_data,
>>>>>>>> +                     &data_npages, &err);
>>>>>>>
>>>>>>> but this one does not and jump straight to 
>>>>>>> drivers/crypto/ccp/sev-dev.c ignoring sev->fd. Why different? Can 
>>>>>>> these two be unified? sev_issue_cmd_external_user() only checks 
>>>>>>> if fd is /dev/sev which is hardly useful.
>>>>>>>
>>>>>>> "[PATCH RFC v7 32/64] crypto: ccp: Provide APIs to query extended 
>>>>>>> attestation report" added this one.
>>>>>>
>>>>>> SNP_EXT_GUEST_REQUEST additionally returns a certificate blob and 
>>>>>> that's why it goes through the CCP driver interface 
>>>>>> snp_guest_ext_guest_request() that is used to get both the report 
>>>>>> and certificate data/blob at the same time.
>>>>>
>>>>> True. I thought though that this calls for extending 
>>>>> sev_issue_cmd() to take care of these extra parameters rather than 
>>>>> just skipping the sev->fd.
>>>>>
>>>>>
>>>>>> All the FW API calls on the KVM side go through sev_issue_cmd() 
>>>>>> and sev_issue_cmd_external_user() interfaces and that i believe 
>>>>>> uses sev->fd more of as a sanity check.
>>>>>
>>>>> Does not look like it:
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/crypto/ccp/sev-dev.c?h=v6.2-rc3#n1290 
>>>>>
>>>>>
>>>>> ===
>>>>> int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd,
>>>>>                  void *data, int *error)
>>>>> {
>>>>>      if (!filep || filep->f_op != &sev_fops)
>>>>>          return -EBADF;
>>>>>
>>>>>      return sev_do_cmd(cmd, data, error);
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
>>>>> ===
>>>>>
>>>>> The only "more" is that it requires sev->fd to be a valid open fd, 
>>>>> what is the value in that? I may easily miss the bigger picture 
>>>>> here. Thanks,
>>>>>
>>>>>
>>>>
>>>> Have a look at following functions in drivers/crypto/ccp/sev-dev.c:
>>>> sev_dev_init() and sev_misc_init().
>>>>
>>>> static int sev_misc_init(struct sev_device *sev)
>>>> {
>>>>          struct device *dev = sev->dev;
>>>>          int ret;
>>>>
>>>>          /*
>>>>           * SEV feature support can be detected on multiple devices but
>>>>           * the SEV FW commands must be issued on the master. During
>>>>           * probe, we do not know the master hence we create 
>>>> /dev/sev on
>>>>           * the first device probe.
>>>>           * sev_do_cmd() finds the right master device to which to 
>>>> issue
>>>>           * the command to the firmware.
>>>>       */
>>>
>>>
>>> It is still a single /dev/sev node and the userspace cannot get it 
>>> wrong, it does not have to choose between (for instance) /dev/sev0 
>>> and /dev/sev1 on a 2 SOC system.
>>>
>>>> ...
>>>> ...
>>>>
>>>> Hence, sev_issue_cmd_external_user() needs to ensure that the 
>>>> correct device (master device) is being operated upon and that's why 
>>>> there is the check for file operations matching sev_fops as below :
>>>>
>>>> int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd,
>>>>                                  void *data, int *error)
>>>> {
>>>>          if (!filep || filep->f_op != &sev_fops)
>>>>                  return -EBADF;
>>>> ..
>>>> ..
>>>>
>>>> Essentially, sev->fd is the misc. device created for the master PSP 
>>>> device on which the SEV/SNP firmware commands are issued, hence,
>>>> sev_issue_cmd() uses sev->fd.
>>>
>>> There is always just one fd which always uses psp_master, nothing 
>>> from that fd is used.
>>
>> It also ensures that we can only issue commands (sev_issue_cmd) after 
>> SEV/SNP guest has launched.
> 
> I can open /dev/sev and start sending commands to the firmware with no 
> KVM running at all. Oh well, we discussed this offline :)
> 

Yes, and as we already discussed we need to support that to get SEV/SNP 
platform status (SNP_PLATFORM_STATUS) and also for legacy SEV commands 
like certificate generation and import/export which can be issued before 
a VM is launched.

Thanks,
Ashish

Alexey Kardashevskiy Jan. 31, 2023, 8:21 p.m. UTC | #13

On 01/02/2023 03:23, Tom Lendacky wrote:
> On 1/30/23 19:54, Alexey Kardashevskiy wrote:
>>
>>
>> On 11/1/23 13:01, Kalra, Ashish wrote:
>>> On 1/10/2023 6:48 PM, Alexey Kardashevskiy wrote:
>>>> On 10/1/23 19:33, Kalra, Ashish wrote:
>>>>>
>>>>> On 1/9/2023 8:28 PM, Alexey Kardashevskiy wrote:
>>>>>>
>>>>>>
>>>>>> On 10/1/23 10:41, Kalra, Ashish wrote:
>>>>>>> On 1/8/2023 9:33 PM, Alexey Kardashevskiy wrote:
>>>>>>>> On 15/12/22 06:40, Michael Roth wrote:
>>>>>>>>> From: Brijesh Singh <[e-mail]>
>>>>>>>>>
>>>>>>>>> Version 2 of GHCB specification added the support for two SNP 
>>>>>>>>> Guest
>>>>>>>>> Request Message NAE events. The events allows for an SEV-SNP 
>>>>>>>>> guest to
>>>>>>>>> make request to the SEV-SNP firmware through hypervisor using the
>>>>>>>>> SNP_GUEST_REQUEST API define in the SEV-SNP firmware 
>>>>>>>>> specification.
>>>>>>>>>
>>>>>>>>> The SNP_EXT_GUEST_REQUEST is similar to SNP_GUEST_REQUEST with the
>>>>>>>>> difference of an additional certificate blob that can be passed 
>>>>>>>>> through
>>>>>>>>> the SNP_SET_CONFIG ioctl defined in the CCP driver. The CCP driver
>>>>>>>>> provides snp_guest_ext_guest_request() that is used by the KVM 
>>>>>>>>> to get
>>>>>>>>> both the report and certificate data at once.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Brijesh Singh <[e-mail]>
>>>>>>>>> Signed-off-by: Ashish Kalra <[e-mail]>
>>>>>>>>> Signed-off-by: Michael Roth <[e-mail]>
>>>>>>>>> ---
>>>>>>>>>   arch/x86/kvm/svm/sev.c | 185 
>>>>>>>>> +++++++++++++++++++++++++++++++++++++++--
>>>>>>>>>   arch/x86/kvm/svm/svm.h |   2 +
>>>>>>>>>   2 files changed, 181 insertions(+), 6 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>>>>>>>> index 5f2b2092cdae..18efa70553c2 100644
>>>>>>>>> --- a/arch/x86/kvm/svm/sev.c
>>>>>>>>> +++ b/arch/x86/kvm/svm/sev.c
>>>>>>>>> @@ -331,6 +331,7 @@ static int sev_guest_init(struct kvm *kvm, 
>>>>>>>>> struct kvm_sev_cmd *argp)
>>>>>>>>>           if (ret)
>>>>>>>>>               goto e_free;
>>>>>>>>> +        mutex_init(&sev->guest_req_lock);
>>>>>>>>>           ret = sev_snp_init(&argp->error, false);
>>>>>>>>>       } else {
>>>>>>>>>           ret = sev_platform_init(&argp->error);
>>>>>>>>> @@ -2051,23 +2052,34 @@ int sev_vm_move_enc_context_from(struct 
>>>>>>>>> kvm *kvm, unsigned int source_fd)
>>>>>>>>>    */
>>>>>>>>>   static void *snp_context_create(struct kvm *kvm, struct 
>>>>>>>>> kvm_sev_cmd *argp)
>>>>>>>>>   {
>>>>>>>>> +    struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>>>>>>>>>       struct sev_data_snp_addr data = {};
>>>>>>>>> -    void *context;
>>>>>>>>> +    void *context, *certs_data;
>>>>>>>>>       int rc;
>>>>>>>>> +    /* Allocate memory used for the certs data in SNP guest 
>>>>>>>>> request */
>>>>>>>>> +    certs_data = kzalloc(SEV_FW_BLOB_MAX_SIZE, 
>>>>>>>>> GFP_KERNEL_ACCOUNT);
>>>>>>>>> +    if (!certs_data)
>>>>>>>>> +        return NULL;
>>>>>>>>> +
>>>>>>>>>       /* Allocate memory for context page */
>>>>>>>>>       context = snp_alloc_firmware_page(GFP_KERNEL_ACCOUNT);
>>>>>>>>>       if (!context)
>>>>>>>>> -        return NULL;
>>>>>>>>> +        goto e_free;
>>>>>>>>>       data.gctx_paddr = __psp_pa(context);
>>>>>>>>>       rc = __sev_issue_cmd(argp->sev_fd, 
>>>>>>>>> SEV_CMD_SNP_GCTX_CREATE, &data, &argp->error);
>>>>>>>>> -    if (rc) {
>>>>>>>>> -        snp_free_firmware_page(context);
>>>>>>>>> -        return NULL;
>>>>>>>>> -    }
>>>>>>>>> +    if (rc)
>>>>>>>>> +        goto e_free;
>>>>>>>>> +
>>>>>>>>> +    sev->snp_certs_data = certs_data;
>>>>>>>>>       return context;
>>>>>>>>> +
>>>>>>>>> +e_free:
>>>>>>>>> +    snp_free_firmware_page(context);
>>>>>>>>> +    kfree(certs_data);
>>>>>>>>> +    return NULL;
>>>>>>>>>   }
>>>>>>>>>   static int snp_bind_asid(struct kvm *kvm, int *error)
>>>>>>>>> @@ -2653,6 +2665,8 @@ static int 
>>>>>>>>> snp_decommission_context(struct kvm *kvm)
>>>>>>>>>       snp_free_firmware_page(sev->snp_context);
>>>>>>>>>       sev->snp_context = NULL;
>>>>>>>>> +    kfree(sev->snp_certs_data);
>>>>>>>>> +
>>>>>>>>>       return 0;
>>>>>>>>>   }
>>>>>>>>> @@ -3174,6 +3188,8 @@ static int sev_es_validate_vmgexit(struct 
>>>>>>>>> vcpu_svm *svm, u64 *exit_code)
>>>>>>>>>       case SVM_VMGEXIT_UNSUPPORTED_EVENT:
>>>>>>>>>       case SVM_VMGEXIT_HV_FEATURES:
>>>>>>>>>       case SVM_VMGEXIT_PSC:
>>>>>>>>> +    case SVM_VMGEXIT_GUEST_REQUEST:
>>>>>>>>> +    case SVM_VMGEXIT_EXT_GUEST_REQUEST:
>>>>>>>>>           break;
>>>>>>>>>       default:
>>>>>>>>>           reason = GHCB_ERR_INVALID_EVENT;
>>>>>>>>> @@ -3396,6 +3412,149 @@ static int snp_complete_psc(struct 
>>>>>>>>> kvm_vcpu *vcpu)
>>>>>>>>>       return 1;
>>>>>>>>>   }
>>>>>>>>> +static unsigned long snp_setup_guest_buf(struct vcpu_svm *svm,
>>>>>>>>> +                     struct sev_data_snp_guest_request *data,
>>>>>>>>> +                     gpa_t req_gpa, gpa_t resp_gpa)
>>>>>>>>> +{
>>>>>>>>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>>>>>>>>> +    struct kvm *kvm = vcpu->kvm;
>>>>>>>>> +    kvm_pfn_t req_pfn, resp_pfn;
>>>>>>>>> +    struct kvm_sev_info *sev;
>>>>>>>>> +
>>>>>>>>> +    sev = &to_kvm_svm(kvm)->sev_info;
>>>>>>>>> +
>>>>>>>>> +    if (!IS_ALIGNED(req_gpa, PAGE_SIZE) || 
>>>>>>>>> !IS_ALIGNED(resp_gpa, PAGE_SIZE))
>>>>>>>>> +        return SEV_RET_INVALID_PARAM;
>>>>>>>>> +
>>>>>>>>> +    req_pfn = gfn_to_pfn(kvm, gpa_to_gfn(req_gpa));
>>>>>>>>> +    if (is_error_noslot_pfn(req_pfn))
>>>>>>>>> +        return SEV_RET_INVALID_ADDRESS;
>>>>>>>>> +
>>>>>>>>> +    resp_pfn = gfn_to_pfn(kvm, gpa_to_gfn(resp_gpa));
>>>>>>>>> +    if (is_error_noslot_pfn(resp_pfn))
>>>>>>>>> +        return SEV_RET_INVALID_ADDRESS;
>>>>>>>>> +
>>>>>>>>> +    if (rmp_make_private(resp_pfn, 0, PG_LEVEL_4K, 0, true))
>>>>>>>>> +        return SEV_RET_INVALID_ADDRESS;
>>>>>>>>> +
>>>>>>>>> +    data->gctx_paddr = __psp_pa(sev->snp_context);
>>>>>>>>> +    data->req_paddr = __sme_set(req_pfn << PAGE_SHIFT);
>>>>>>>>> +    data->res_paddr = __sme_set(resp_pfn << PAGE_SHIFT);
>>>>>>>>> +
>>>>>>>>> +    return 0;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +static void snp_cleanup_guest_buf(struct 
>>>>>>>>> sev_data_snp_guest_request *data, unsigned long *rc)
>>>>>>>>> +{
>>>>>>>>> +    u64 pfn = __sme_clr(data->res_paddr) >> PAGE_SHIFT;
>>>>>>>>> +    int ret;
>>>>>>>>> +
>>>>>>>>> +    ret = snp_page_reclaim(pfn);
>>>>>>>>> +    if (ret)
>>>>>>>>> +        *rc = SEV_RET_INVALID_ADDRESS;
>>>>>>>>> +
>>>>>>>>> +    ret = rmp_make_shared(pfn, PG_LEVEL_4K);
>>>>>>>>> +    if (ret)
>>>>>>>>> +        *rc = SEV_RET_INVALID_ADDRESS;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +static void snp_handle_guest_request(struct vcpu_svm *svm, 
>>>>>>>>> gpa_t req_gpa, gpa_t resp_gpa)
>>>>>>>>> +{
>>>>>>>>> +    struct sev_data_snp_guest_request data = {0};
>>>>>>>>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>>>>>>>>> +    struct kvm *kvm = vcpu->kvm;
>>>>>>>>> +    struct kvm_sev_info *sev;
>>>>>>>>> +    unsigned long rc;
>>>>>>>>> +    int err;
>>>>>>>>> +
>>>>>>>>> +    if (!sev_snp_guest(vcpu->kvm)) {
>>>>>>>>> +        rc = SEV_RET_INVALID_GUEST;
>>>>>>>>> +        goto e_fail;
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    sev = &to_kvm_svm(kvm)->sev_info;
>>>>>>>>> +
>>>>>>>>> +    mutex_lock(&sev->guest_req_lock);
>>>>>>>>> +
>>>>>>>>> +    rc = snp_setup_guest_buf(svm, &data, req_gpa, resp_gpa);
>>>>>>>>> +    if (rc)
>>>>>>>>> +        goto unlock;
>>>>>>>>> +
>>>>>>>>> +    rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, 
>>>>>>>>> &err);
>>>>>>>>
>>>>>>>>
>>>>>>>> This one goes via sev_issue_cmd_external_user() and uses sev-fd...
>>>>>>>>
>>>>>>>>> +    if (rc)
>>>>>>>>> +        /* use the firmware error code */
>>>>>>>>> +        rc = err;
>>>>>>>>> +
>>>>>>>>> +    snp_cleanup_guest_buf(&data, &rc);
>>>>>>>>> +
>>>>>>>>> +unlock:
>>>>>>>>> +    mutex_unlock(&sev->guest_req_lock);
>>>>>>>>> +
>>>>>>>>> +e_fail:
>>>>>>>>> +    svm_set_ghcb_sw_exit_info_2(vcpu, rc);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +static void snp_handle_ext_guest_request(struct vcpu_svm *svm, 
>>>>>>>>> gpa_t req_gpa, gpa_t resp_gpa)
>>>>>>>>> +{
>>>>>>>>> +    struct sev_data_snp_guest_request req = {0};
>>>>>>>>> +    struct kvm_vcpu *vcpu = &svm->vcpu;
>>>>>>>>> +    struct kvm *kvm = vcpu->kvm;
>>>>>>>>> +    unsigned long data_npages;
>>>>>>>>> +    struct kvm_sev_info *sev;
>>>>>>>>> +    unsigned long rc, err;
>>>>>>>>> +    u64 data_gpa;
>>>>>>>>> +
>>>>>>>>> +    if (!sev_snp_guest(vcpu->kvm)) {
>>>>>>>>> +        rc = SEV_RET_INVALID_GUEST;
>>>>>>>>> +        goto e_fail;
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    sev = &to_kvm_svm(kvm)->sev_info;
>>>>>>>>> +
>>>>>>>>> +    data_gpa = vcpu->arch.regs[VCPU_REGS_RAX];
>>>>>>>>> +    data_npages = vcpu->arch.regs[VCPU_REGS_RBX];
>>>>>>>>> +
>>>>>>>>> +    if (!IS_ALIGNED(data_gpa, PAGE_SIZE)) {
>>>>>>>>> +        rc = SEV_RET_INVALID_ADDRESS;
>>>>>>>>> +        goto e_fail;
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    mutex_lock(&sev->guest_req_lock);
>>>>>>>>> +
>>>>>>>>> +    rc = snp_setup_guest_buf(svm, &req, req_gpa, resp_gpa);
>>>>>>>>> +    if (rc)
>>>>>>>>> +        goto unlock;
>>>>>>>>> +
>>>>>>>>> +    rc = snp_guest_ext_guest_request(&req, (unsigned 
>>>>>>>>> long)sev->snp_certs_data,
>>>>>>>>> +                     &data_npages, &err);
>>>>>>>>
>>>>>>>> but this one does not and jump straight to 
>>>>>>>> drivers/crypto/ccp/sev-dev.c ignoring sev->fd. Why different? 
>>>>>>>> Can these two be unified? sev_issue_cmd_external_user() only 
>>>>>>>> checks if fd is /dev/sev which is hardly useful.
>>>>>>>>
>>>>>>>> "[PATCH RFC v7 32/64] crypto: ccp: Provide APIs to query 
>>>>>>>> extended attestation report" added this one.
>>>>>>>
>>>>>>> SNP_EXT_GUEST_REQUEST additionally returns a certificate blob and 
>>>>>>> that's why it goes through the CCP driver interface 
>>>>>>> snp_guest_ext_guest_request() that is used to get both the report 
>>>>>>> and certificate data/blob at the same time.
>>>>>>
>>>>>> True. I thought though that this calls for extending 
>>>>>> sev_issue_cmd() to take care of these extra parameters rather than 
>>>>>> just skipping the sev->fd.
>>>>>>
>>>>>>
>>>>>>> All the FW API calls on the KVM side go through sev_issue_cmd() 
>>>>>>> and sev_issue_cmd_external_user() interfaces and that i believe 
>>>>>>> uses sev->fd more of as a sanity check.
>>>>>>
>>>>>> Does not look like it:
>>>>>>
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/crypto/ccp/sev-dev.c?h=v6.2-rc3#n1290
>>>>>>
>>>>>> ===
>>>>>> int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd,
>>>>>>                  void *data, int *error)
>>>>>> {
>>>>>>      if (!filep || filep->f_op != &sev_fops)
>>>>>>          return -EBADF;
>>>>>>
>>>>>>      return sev_do_cmd(cmd, data, error);
>>>>>> }
>>>>>> EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
>>>>>> ===
>>>>>>
>>>>>> The only "more" is that it requires sev->fd to be a valid open fd, 
>>>>>> what is the value in that? I may easily miss the bigger picture 
>>>>>> here. Thanks,
>>>>>>
>>>>>>
>>>>>
>>>>> Have a look at following functions in drivers/crypto/ccp/sev-dev.c:
>>>>> sev_dev_init() and sev_misc_init().
>>>>>
>>>>> static int sev_misc_init(struct sev_device *sev)
>>>>> {
>>>>>          struct device *dev = sev->dev;
>>>>>          int ret;
>>>>>
>>>>>          /*
>>>>>           * SEV feature support can be detected on multiple devices 
>>>>> but
>>>>>           * the SEV FW commands must be issued on the master. During
>>>>>           * probe, we do not know the master hence we create 
>>>>> /dev/sev on
>>>>>           * the first device probe.
>>>>>           * sev_do_cmd() finds the right master device to which to 
>>>>> issue
>>>>>           * the command to the firmware.
>>>>>       */
>>>>
>>>>
>>>> It is still a single /dev/sev node and the userspace cannot get it 
>>>> wrong, it does not have to choose between (for instance) /dev/sev0 
>>>> and /dev/sev1 on a 2 SOC system.
>>>>
>>>>> ...
>>>>> ...
>>>>>
>>>>> Hence, sev_issue_cmd_external_user() needs to ensure that the 
>>>>> correct device (master device) is being operated upon and that's 
>>>>> why there is the check for file operations matching sev_fops as 
>>>>> below :
>>>>>
>>>>> int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd,
>>>>>                                  void *data, int *error)
>>>>> {
>>>>>          if (!filep || filep->f_op != &sev_fops)
>>>>>                  return -EBADF;
>>>>> ..
>>>>> ..
>>>>>
>>>>> Essentially, sev->fd is the misc. device created for the master PSP 
>>>>> device on which the SEV/SNP firmware commands are issued, hence,
>>>>> sev_issue_cmd() uses sev->fd.
>>>>
>>>> There is always just one fd which always uses psp_master, nothing 
>>>> from that fd is used.
>>>
>>> It also ensures that we can only issue commands (sev_issue_cmd) after 
>>> SEV/SNP guest has launched.
>>
>> I can open /dev/sev and start sending commands to the firmware with no 
>> KVM running at all. Oh well, we discussed this offline :)
>>
>>> We don't have a valid fd to use before the guest launch. The file 
>>> descriptor is passed as part of the guest launch flow, for example, 
>>> in snp_launch_start().
>>>>
>>>> More to the point, if sev->fd is still important, why is it ok to 
>>>> skip it for snp_handle_ext_guest_request()? Thanks,
>>>>
>>>>
>>> Then, we should do the same for snp_handle_ext_guest_request().
>>
>> Okay.
>>
>> This snp_handle_ext_guest_request() helper is for returning "Table 21. 
>> ATTESTATION_REPORT Structure" along with the certificate(s) used to 
>> sign the report: "This usage allows the attestation report and the 
>> certificates required to verify the report to be returned at the same 
>> time".
>>
>> I can see:
>> 1) KVM_SEV_SNP_{G,S}ET_CERTS ioctls on KVM VM and
> 
> This allows the VMM to (optionally) supply per-VM certificates that the 
> guest can use to validate the attestation report, instead of the guest 
> requesting separately.
> 
>> 2) SNP_{SET,GET}_EXT_CONFIG ioctls on /dev/sev
> 
> This allows the VMM to (optionally) supply certificates used for all 
> VMs, i.e., there is no need for per-VM certificates.
> 
>> Both store the passed blob and neither communicate it to the firmware. 
>> This makes me wonder - how does the attestation report (cooked by the 
>> firmware) get signed with those certificates passed on by the HV 
>> userspace?
> 
> These are for use by the guest to validate the attestation report. It 
> allows the guest to obtain the certificate information without having to 
> use another method to request the certificates.
> 
> By having this certificate store, the hypervisor can request the 
> certificates from the KDS once, rather than every time a guest requests 
> an attestation report.
> 
>>
>> Also, the cached blob in /dev/sev seems redundand - the attestation 
>> report is retuned for a specific guest so having a blob in the KVM VM 
>> makes sense and KVM unconditionally reserves memory for it anyway. And 
>> for the HV itself the blob is useless (?) so why bother with caching 
>> it in /dev/sev.
> 
> In general, the certificates are for the machine (VCEK, ASK, ARK), so 
> they can be for all VMs on the machine. The per-VM blob allows a VMM to 
> supply additional per-VM certficates, if it desires, but is not required.
> 
>>
>> And GET ioctls() return what SET passed on (not something the firware 
>> returned, for example), what is ever going to call SET? The userspace can 
> 
> As stated above, the firmware already has the information needed to sign 
> the attestation report. The SET IOCTL is used to supply the certficates 
> to the guest for validation of the attestation report.


Does the firmware have to have all certificates beforehand? How does the 
firmware choose which certificate to use for a specific VM, or just 
signs all reports with all certificates it knows?


> This reduces the 
> traffic and complexity of the guest requesting the certficates from the 
> KDS.

Guest <-> HV interaction is clear, I am only wondering about HV <-> FW.


>> as well cache what it passed and save a bit of the code/memory in the 
>> kernel.
>>
>> btw SNP_{SET,GET}_EXT_CONFIG are documented in 
>> Documentation/virt/coco/sev-guest.rst but implemented in 
>> drivers/crypto/ccp/sev-dev.c (not sev-guest.c).
>>
>> What do I miss in the big picture here? :) Thanks,
> 
> The reason for the extended request is to make the attestation request 
> appear atomic to the guest. If you had to make two calls to request the 
> information, in the future, when live migration is possible, there is no 
> guarantee that the guest couldn't have been migrated in between the 
> calls to obtain the certificates and the call to obtain the attestation 
> report and thus validation of the attestation report could fail.

Tom Lendacky Jan. 31, 2023, 9:21 p.m. UTC | #14

On 1/31/23 14:21, Alexey Kardashevskiy wrote:
> On 01/02/2023 03:23, Tom Lendacky wrote:
>> On 1/30/23 19:54, Alexey Kardashevskiy wrote:
>>> On 11/1/23 13:01, Kalra, Ashish wrote:
>>>> On 1/10/2023 6:48 PM, Alexey Kardashevskiy wrote:
>>>>> On 10/1/23 19:33, Kalra, Ashish wrote:
>>>>>> On 1/9/2023 8:28 PM, Alexey Kardashevskiy wrote:
>>>>>>> On 10/1/23 10:41, Kalra, Ashish wrote:
>>>>>>>> On 1/8/2023 9:33 PM, Alexey Kardashevskiy wrote:
>>>>>>>>> On 15/12/22 06:40, Michael Roth wrote:
>>>>>>>>>> From: Brijesh Singh <[e-mail]>
>>>>>>>>>>
>>>>>>>>>> Version 2 of GHCB specification added the support for two SNP Guest
>>>>>>>>>> Request Message NAE events. The events allows for an SEV-SNP 
>>>>>>>>>> guest to
>>>>>>>>>> make request to the SEV-SNP firmware through hypervisor using the
>>>>>>>>>> SNP_GUEST_REQUEST API define in the SEV-SNP firmware specification.
>>>>>>>>>>
>>>>>>>>>> The SNP_EXT_GUEST_REQUEST is similar to SNP_GUEST_REQUEST with the
>>>>>>>>>> difference of an additional certificate blob that can be passed 
>>>>>>>>>> through
>>>>>>>>>> the SNP_SET_CONFIG ioctl defined in the CCP driver. The CCP driver
>>>>>>>>>> provides snp_guest_ext_guest_request() that is used by the KVM 
>>>>>>>>>> to get
>>>>>>>>>> both the report and certificate data at once.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Brijesh Singh <[e-mail]>
>>>>>>>>>> Signed-off-by: Ashish Kalra <[e-mail]>
>>>>>>>>>> Signed-off-by: Michael Roth <[e-mail]>
>>>>>>>>>> ---

>>>
>>> And GET ioctls() return what SET passed on (not something the firware 
>>> returned, for example), what is ever going to call SET? The userspace can 
>>
>> As stated above, the firmware already has the information needed to sign 
>> the attestation report. The SET IOCTL is used to supply the certficates 
>> to the guest for validation of the attestation report.
> 
> 
> Does the firmware have to have all certificates beforehand? How does the 
> firmware choose which certificate to use for a specific VM, or just signs 
> all reports with all certificates it knows?

 From the SNP API spec, the firmware uses the VCEK, which is derived from 
chip-unique secrets, to sign the attestation report.

The guest can then use the returned VCEK certificate, the ASK certificate 
and ARK certificate from the extended guest request to validate the 
attestation report.

> 
> 
>> This reduces the traffic and complexity of the guest requesting the 
>> certficates from the KDS.
> 
> Guest <-> HV interaction is clear, I am only wondering about HV <-> FW.

I'm not sure what you mean here. The HV doesn't put the signing key in the 
firmware, it is derived.

Thanks,
Tom

Alexey Kardashevskiy Jan. 31, 2023, 10 p.m. UTC | #15

On 01/02/2023 08:21, Tom Lendacky wrote:
> On 1/31/23 14:21, Alexey Kardashevskiy wrote:
>> On 01/02/2023 03:23, Tom Lendacky wrote:
>>> On 1/30/23 19:54, Alexey Kardashevskiy wrote:
>>>> On 11/1/23 13:01, Kalra, Ashish wrote:
>>>>> On 1/10/2023 6:48 PM, Alexey Kardashevskiy wrote:
>>>>>> On 10/1/23 19:33, Kalra, Ashish wrote:
>>>>>>> On 1/9/2023 8:28 PM, Alexey Kardashevskiy wrote:
>>>>>>>> On 10/1/23 10:41, Kalra, Ashish wrote:
>>>>>>>>> On 1/8/2023 9:33 PM, Alexey Kardashevskiy wrote:
>>>>>>>>>> On 15/12/22 06:40, Michael Roth wrote:
>>>>>>>>>>> From: Brijesh Singh <[e-mail]>
>>>>>>>>>>>
>>>>>>>>>>> Version 2 of GHCB specification added the support for two SNP 
>>>>>>>>>>> Guest
>>>>>>>>>>> Request Message NAE events. The events allows for an SEV-SNP 
>>>>>>>>>>> guest to
>>>>>>>>>>> make request to the SEV-SNP firmware through hypervisor using 
>>>>>>>>>>> the
>>>>>>>>>>> SNP_GUEST_REQUEST API define in the SEV-SNP firmware 
>>>>>>>>>>> specification.
>>>>>>>>>>>
>>>>>>>>>>> The SNP_EXT_GUEST_REQUEST is similar to SNP_GUEST_REQUEST 
>>>>>>>>>>> with the
>>>>>>>>>>> difference of an additional certificate blob that can be 
>>>>>>>>>>> passed through
>>>>>>>>>>> the SNP_SET_CONFIG ioctl defined in the CCP driver. The CCP 
>>>>>>>>>>> driver
>>>>>>>>>>> provides snp_guest_ext_guest_request() that is used by the 
>>>>>>>>>>> KVM to get
>>>>>>>>>>> both the report and certificate data at once.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Brijesh Singh <[e-mail]>
>>>>>>>>>>> Signed-off-by: Ashish Kalra <[e-mail]>
>>>>>>>>>>> Signed-off-by: Michael Roth <[e-mail]>
>>>>>>>>>>> ---
> 
>>>>
>>>> And GET ioctls() return what SET passed on (not something the 
>>>> firware returned, for example), what is ever going to call SET? The 
>>>> userspace can 
>>>
>>> As stated above, the firmware already has the information needed to 
>>> sign the attestation report. The SET IOCTL is used to supply the 
>>> certficates to the guest for validation of the attestation report.
>>
>>
>> Does the firmware have to have all certificates beforehand? How does 
>> the firmware choose which certificate to use for a specific VM, or 
>> just signs all reports with all certificates it knows?
> 
>  From the SNP API spec, the firmware uses the VCEK, which is derived 
> from chip-unique secrets, to sign the attestation report.

Does the firmware derive it? How does the guest gets to know it?
(forgive me my ignorance)


> The guest can then use the returned VCEK certificate, the ASK 
> certificate and ARK certificate from the extended guest request to 
> validate the attestation report.

>>
>>
>>> This reduces the traffic and complexity of the guest requesting the 
>>> certficates from the KDS.
>>
>> Guest <-> HV interaction is clear, I am only wondering about HV <-> FW.
> 
> I'm not sure what you mean here. The HV doesn't put the signing key in 
> the firmware, it is derived.


Those ioctls() are in the HV and they take certificates which then get 
sent to the guest but not to the firmware. The firmware signs a report 
with a key and the guest needs another half of it to verify the report. 
Sadly I do not know cryptography enough.

Tom Lendacky Jan. 31, 2023, 10:42 p.m. UTC | #16

On 1/31/23 16:00, Alexey Kardashevskiy wrote:
> On 01/02/2023 08:21, Tom Lendacky wrote:
>> On 1/31/23 14:21, Alexey Kardashevskiy wrote:
>>> On 01/02/2023 03:23, Tom Lendacky wrote:
>>>> On 1/30/23 19:54, Alexey Kardashevskiy wrote:
>>>>> On 11/1/23 13:01, Kalra, Ashish wrote:
>>>>>> On 1/10/2023 6:48 PM, Alexey Kardashevskiy wrote:
>>>>>>> On 10/1/23 19:33, Kalra, Ashish wrote:
>>>>>>>> On 1/9/2023 8:28 PM, Alexey Kardashevskiy wrote:
>>>>>>>>> On 10/1/23 10:41, Kalra, Ashish wrote:
>>>>>>>>>> On 1/8/2023 9:33 PM, Alexey Kardashevskiy wrote:
>>>>>>>>>>> On 15/12/22 06:40, Michael Roth wrote:
>>>>>>>>>>>> From: Brijesh Singh <[e-mail]>
>>>>>>>>>>>>
>>>>>>>>>>>> Version 2 of GHCB specification added the support for two SNP 
>>>>>>>>>>>> Guest
>>>>>>>>>>>> Request Message NAE events. The events allows for an SEV-SNP 
>>>>>>>>>>>> guest to
>>>>>>>>>>>> make request to the SEV-SNP firmware through hypervisor using the
>>>>>>>>>>>> SNP_GUEST_REQUEST API define in the SEV-SNP firmware 
>>>>>>>>>>>> specification.
>>>>>>>>>>>>
>>>>>>>>>>>> The SNP_EXT_GUEST_REQUEST is similar to SNP_GUEST_REQUEST with 
>>>>>>>>>>>> the
>>>>>>>>>>>> difference of an additional certificate blob that can be 
>>>>>>>>>>>> passed through
>>>>>>>>>>>> the SNP_SET_CONFIG ioctl defined in the CCP driver. The CCP 
>>>>>>>>>>>> driver
>>>>>>>>>>>> provides snp_guest_ext_guest_request() that is used by the KVM 
>>>>>>>>>>>> to get
>>>>>>>>>>>> both the report and certificate data at once.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Brijesh Singh <[e-mail]>
>>>>>>>>>>>> Signed-off-by: Ashish Kalra <[e-mail]>
>>>>>>>>>>>> Signed-off-by: Michael Roth <[e-mail]>
>>>>>>>>>>>> ---
>>
>>>>>
>>>>> And GET ioctls() return what SET passed on (not something the firware 
>>>>> returned, for example), what is ever going to call SET? The userspace 
>>>>> can 
>>>>
>>>> As stated above, the firmware already has the information needed to 
>>>> sign the attestation report. The SET IOCTL is used to supply the 
>>>> certficates to the guest for validation of the attestation report.
>>>
>>>
>>> Does the firmware have to have all certificates beforehand? How does 
>>> the firmware choose which certificate to use for a specific VM, or just 
>>> signs all reports with all certificates it knows?
>>
>>  From the SNP API spec, the firmware uses the VCEK, which is derived 
>> from chip-unique secrets, to sign the attestation report.
> 
> Does the firmware derive it? How does the guest gets to know it?
> (forgive me my ignorance)

Yes, the firmware derives the private key. The guest doesn't know the 
private key, it gets the VCEK certificate which has the public key and can 
then validate the attestation report.

> 
> 
>> The guest can then use the returned VCEK certificate, the ASK 
>> certificate and ARK certificate from the extended guest request to 
>> validate the attestation report.
> 
>>>
>>>
>>>> This reduces the traffic and complexity of the guest requesting the 
>>>> certficates from the KDS.
>>>
>>> Guest <-> HV interaction is clear, I am only wondering about HV <-> FW.
>>
>> I'm not sure what you mean here. The HV doesn't put the signing key in 
>> the firmware, it is derived.
> 
> 
> Those ioctls() are in the HV and they take certificates which then get 
> sent to the guest but not to the firmware. The firmware signs a report 
> with a key and the guest needs another half of it to verify the report. 
> Sadly I do not know cryptography enough.

Correct, no need to send the certificates to the firmware. The certs have 
the public key which can be used to verify the report signed with the 
private key.

Thanks,
Tom

> 
> 
>

Dionna Amalie Glaze May 11, 2023, 11:02 p.m. UTC | #17

Would it be okay to request that we add a KVM stat for how often there
are GUEST_REQUEST_NAE exits? I think it'd be good for service
operators to get a better idea how valued the feature is.

Sean Christopherson May 11, 2023, 11:32 p.m. UTC | #18

On Thu, May 11, 2023, Dionna Amalie Glaze wrote:
> Would it be okay to request that we add a KVM stat for how often there
> are GUEST_REQUEST_NAE exits? I think it'd be good for service
> operators to get a better idea how valued the feature is.

Heh, it's always ok to request something, but sometimes the answer will be no.

And in the case, the answer is likely "no stat for you".  A year or so ago, in the
context of us (Google) trying to upstream a pile of stats, we (KVM folks) came to
a rough consensus that KVM should only add upstream stats if they are relatively
generic and (almost) universally useful[*].  IMO, a one-off stat for a specific exit
reason is too narrowly focused, e.g. collecting information on all exit reasons is
superior.  And no, that won't be accepted upstream either, because for some environments
gathering detailed information on all exits is too much overhead (also covered in
the link).

FWIW, we (GCE) plan on carrying stats like this in out-of-tree patches, i.e. your
request for stats is likely something that would get accepted internally (if it
isn't already captured through our generic stats collection).

[*] https://lore.kernel.org/all/[e-mail]

Dionna Amalie Glaze May 15, 2023, 4:45 p.m. UTC | #19

On Thu, May 11, 2023 at 4:33 PM Sean Christopherson <[e-mail]> wrote:
>
> On Thu, May 11, 2023, Dionna Amalie Glaze wrote:
> > Would it be okay to request that we add a KVM stat for how often there
> > are GUEST_REQUEST_NAE exits? I think it'd be good for service
> > operators to get a better idea how valued the feature is.
>
> Heh, it's always ok to request something, but sometimes the answer will be no.
>
> And in the case, the answer is likely "no stat for you".  A year or so ago, in the
> context of us (Google) trying to upstream a pile of stats, we (KVM folks) came to
> a rough consensus that KVM should only add upstream stats if they are relatively
> generic and (almost) universally useful[*].  IMO, a one-off stat for a specific exit
> reason is too narrowly focused, e.g. collecting information on all exit reasons is
> superior.  And no, that won't be accepted upstream either, because for some environments
> gathering detailed information on all exits is too much overhead (also covered in
> the link).
>
> FWIW, we (GCE) plan on carrying stats like this in out-of-tree patches, i.e. your
> request for stats is likely something that would get accepted internally (if it
> isn't already captured through our generic stats collection).
>
> [*] https://lore.kernel.org/all/[e-mail]

Thanks Sean, noted :)

Patch

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 5f2b2092cdae..18efa70553c2 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -331,6 +331,7 @@  static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
 		if (ret)
 			goto e_free;
 
+		mutex_init(&sev->guest_req_lock);
 		ret = sev_snp_init(&argp->error, false);
 	} else {
 		ret = sev_platform_init(&argp->error);
@@ -2051,23 +2052,34 @@  int sev_vm_move_enc_context_from(struct kvm *kvm, unsigned int source_fd)
  */
 static void *snp_context_create(struct kvm *kvm, struct kvm_sev_cmd *argp)
 {
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
 	struct sev_data_snp_addr data = {};
-	void *context;
+	void *context, *certs_data;
 	int rc;
 
+	/* Allocate memory used for the certs data in SNP guest request */
+	certs_data = kzalloc(SEV_FW_BLOB_MAX_SIZE, GFP_KERNEL_ACCOUNT);
+	if (!certs_data)
+		return NULL;
+
 	/* Allocate memory for context page */
 	context = snp_alloc_firmware_page(GFP_KERNEL_ACCOUNT);
 	if (!context)
-		return NULL;
+		goto e_free;
 
 	data.gctx_paddr = __psp_pa(context);
 	rc = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_GCTX_CREATE, &data, &argp->error);
-	if (rc) {
-		snp_free_firmware_page(context);
-		return NULL;
-	}
+	if (rc)
+		goto e_free;
+
+	sev->snp_certs_data = certs_data;
 
 	return context;
+
+e_free:
+	snp_free_firmware_page(context);
+	kfree(certs_data);
+	return NULL;
 }
 
 static int snp_bind_asid(struct kvm *kvm, int *error)
@@ -2653,6 +2665,8 @@  static int snp_decommission_context(struct kvm *kvm)
 	snp_free_firmware_page(sev->snp_context);
 	sev->snp_context = NULL;
 
+	kfree(sev->snp_certs_data);
+
 	return 0;
 }
 
@@ -3174,6 +3188,8 @@  static int sev_es_validate_vmgexit(struct vcpu_svm *svm, u64 *exit_code)
 	case SVM_VMGEXIT_UNSUPPORTED_EVENT:
 	case SVM_VMGEXIT_HV_FEATURES:
 	case SVM_VMGEXIT_PSC:
+	case SVM_VMGEXIT_GUEST_REQUEST:
+	case SVM_VMGEXIT_EXT_GUEST_REQUEST:
 		break;
 	default:
 		reason = GHCB_ERR_INVALID_EVENT;
@@ -3396,6 +3412,149 @@  static int snp_complete_psc(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
+static unsigned long snp_setup_guest_buf(struct vcpu_svm *svm,
+					 struct sev_data_snp_guest_request *data,
+					 gpa_t req_gpa, gpa_t resp_gpa)
+{
+	struct kvm_vcpu *vcpu = &svm->vcpu;
+	struct kvm *kvm = vcpu->kvm;
+	kvm_pfn_t req_pfn, resp_pfn;
+	struct kvm_sev_info *sev;
+
+	sev = &to_kvm_svm(kvm)->sev_info;
+
+	if (!IS_ALIGNED(req_gpa, PAGE_SIZE) || !IS_ALIGNED(resp_gpa, PAGE_SIZE))
+		return SEV_RET_INVALID_PARAM;
+
+	req_pfn = gfn_to_pfn(kvm, gpa_to_gfn(req_gpa));
+	if (is_error_noslot_pfn(req_pfn))
+		return SEV_RET_INVALID_ADDRESS;
+
+	resp_pfn = gfn_to_pfn(kvm, gpa_to_gfn(resp_gpa));
+	if (is_error_noslot_pfn(resp_pfn))
+		return SEV_RET_INVALID_ADDRESS;
+
+	if (rmp_make_private(resp_pfn, 0, PG_LEVEL_4K, 0, true))
+		return SEV_RET_INVALID_ADDRESS;
+
+	data->gctx_paddr = __psp_pa(sev->snp_context);
+	data->req_paddr = __sme_set(req_pfn << PAGE_SHIFT);
+	data->res_paddr = __sme_set(resp_pfn << PAGE_SHIFT);
+
+	return 0;
+}
+
+static void snp_cleanup_guest_buf(struct sev_data_snp_guest_request *data, unsigned long *rc)
+{
+	u64 pfn = __sme_clr(data->res_paddr) >> PAGE_SHIFT;
+	int ret;
+
+	ret = snp_page_reclaim(pfn);
+	if (ret)
+		*rc = SEV_RET_INVALID_ADDRESS;
+
+	ret = rmp_make_shared(pfn, PG_LEVEL_4K);
+	if (ret)
+		*rc = SEV_RET_INVALID_ADDRESS;
+}
+
+static void snp_handle_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp_gpa)
+{
+	struct sev_data_snp_guest_request data = {0};
+	struct kvm_vcpu *vcpu = &svm->vcpu;
+	struct kvm *kvm = vcpu->kvm;
+	struct kvm_sev_info *sev;
+	unsigned long rc;
+	int err;
+
+	if (!sev_snp_guest(vcpu->kvm)) {
+		rc = SEV_RET_INVALID_GUEST;
+		goto e_fail;
+	}
+
+	sev = &to_kvm_svm(kvm)->sev_info;
+
+	mutex_lock(&sev->guest_req_lock);
+
+	rc = snp_setup_guest_buf(svm, &data, req_gpa, resp_gpa);
+	if (rc)
+		goto unlock;
+
+	rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, &err);
+	if (rc)
+		/* use the firmware error code */
+		rc = err;
+
+	snp_cleanup_guest_buf(&data, &rc);
+
+unlock:
+	mutex_unlock(&sev->guest_req_lock);
+
+e_fail:
+	svm_set_ghcb_sw_exit_info_2(vcpu, rc);
+}
+
+static void snp_handle_ext_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp_gpa)
+{
+	struct sev_data_snp_guest_request req = {0};
+	struct kvm_vcpu *vcpu = &svm->vcpu;
+	struct kvm *kvm = vcpu->kvm;
+	unsigned long data_npages;
+	struct kvm_sev_info *sev;
+	unsigned long rc, err;
+	u64 data_gpa;
+
+	if (!sev_snp_guest(vcpu->kvm)) {
+		rc = SEV_RET_INVALID_GUEST;
+		goto e_fail;
+	}
+
+	sev = &to_kvm_svm(kvm)->sev_info;
+
+	data_gpa = vcpu->arch.regs[VCPU_REGS_RAX];
+	data_npages = vcpu->arch.regs[VCPU_REGS_RBX];
+
+	if (!IS_ALIGNED(data_gpa, PAGE_SIZE)) {
+		rc = SEV_RET_INVALID_ADDRESS;
+		goto e_fail;
+	}
+
+	mutex_lock(&sev->guest_req_lock);
+
+	rc = snp_setup_guest_buf(svm, &req, req_gpa, resp_gpa);
+	if (rc)
+		goto unlock;
+
+	rc = snp_guest_ext_guest_request(&req, (unsigned long)sev->snp_certs_data,
+					 &data_npages, &err);
+	if (rc) {
+		/*
+		 * If buffer length is small then return the expected
+		 * length in rbx.
+		 */
+		if (err == SNP_GUEST_REQ_INVALID_LEN)
+			vcpu->arch.regs[VCPU_REGS_RBX] = data_npages;
+
+		/* pass the firmware error code */
+		rc = err;
+		goto cleanup;
+	}
+
+	/* Copy the certificate blob in the guest memory */
+	if (data_npages &&
+	    kvm_write_guest(kvm, data_gpa, sev->snp_certs_data, data_npages << PAGE_SHIFT))
+		rc = SEV_RET_INVALID_ADDRESS;
+
+cleanup:
+	snp_cleanup_guest_buf(&req, &rc);
+
+unlock:
+	mutex_unlock(&sev->guest_req_lock);
+
+e_fail:
+	svm_set_ghcb_sw_exit_info_2(vcpu, rc);
+}
+
 static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
 {
 	struct vmcb_control_area *control = &svm->vmcb->control;
@@ -3629,6 +3788,20 @@  int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
 		vcpu->run->vmgexit.ghcb_msr = ghcb_gpa;
 		vcpu->arch.complete_userspace_io = snp_complete_psc;
 		break;
+	case SVM_VMGEXIT_GUEST_REQUEST: {
+		snp_handle_guest_request(svm, control->exit_info_1, control->exit_info_2);
+
+		ret = 1;
+		break;
+	}
+	case SVM_VMGEXIT_EXT_GUEST_REQUEST: {
+		snp_handle_ext_guest_request(svm,
+					     control->exit_info_1,
+					     control->exit_info_2);
+
+		ret = 1;
+		break;
+	}
 	case SVM_VMGEXIT_UNSUPPORTED_EVENT:
 		vcpu_unimpl(vcpu,
 			    "vmgexit: unsupported event - exit_info_1=%#llx, exit_info_2=%#llx\n",
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 12b9f4d539fb..7c0f9d00950f 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -101,6 +101,8 @@  struct kvm_sev_info {
 	u64 snp_init_flags;
 	void *snp_context;      /* SNP guest context page */
 	spinlock_t psc_lock;
+	void *snp_certs_data;
+	struct mutex guest_req_lock;
 };
 
 struct kvm_svm {