[PATCH v3] KVM: SEV: Add KVM_SEV_SNP_HV_REPORT_REQ command

Thomas Courrege posted 1 patch 1 month, 3 weeks ago
There is a newer version of this series
.../virt/kvm/x86/amd-memory-encryption.rst    | 27 +++++++++
arch/x86/include/uapi/asm/kvm.h               |  9 +++
arch/x86/kvm/svm/sev.c                        | 60 +++++++++++++++++++
drivers/crypto/ccp/sev-dev.c                  |  1 +
include/linux/psp-sev.h                       | 31 ++++++++++
5 files changed, 128 insertions(+)
[PATCH v3] KVM: SEV: Add KVM_SEV_SNP_HV_REPORT_REQ command
Posted by Thomas Courrege 1 month, 3 weeks ago
Add support for retrieving the SEV-SNP attestation report via the
SNP_HV_REPORT_REQ firmware command and expose it through a new KVM
ioctl for SNP guests.

Signed-off-by: Thomas Courrege <thomas.courrege@vates.tech>
---
 .../virt/kvm/x86/amd-memory-encryption.rst    | 27 +++++++++
 arch/x86/include/uapi/asm/kvm.h               |  9 +++
 arch/x86/kvm/svm/sev.c                        | 60 +++++++++++++++++++
 drivers/crypto/ccp/sev-dev.c                  |  1 +
 include/linux/psp-sev.h                       | 31 ++++++++++
 5 files changed, 128 insertions(+)

diff --git a/Documentation/virt/kvm/x86/amd-memory-encryption.rst b/Documentation/virt/kvm/x86/amd-memory-encryption.rst
index 1ddb6a86ce7f..083ed487764e 100644
--- a/Documentation/virt/kvm/x86/amd-memory-encryption.rst
+++ b/Documentation/virt/kvm/x86/amd-memory-encryption.rst
@@ -572,6 +572,33 @@ Returns: 0 on success, -negative on error
 See SNP_LAUNCH_FINISH in the SEV-SNP specification [snp-fw-abi]_ for further
 details on the input parameters in ``struct kvm_sev_snp_launch_finish``.
 
+21. KVM_SEV_SNP_HV_REPORT_REQ
+-----------------------------
+
+The KVM_SEV_SNP_HV_REPORT_REQ command requests the hypervisor-generated
+SNP attestation report. This report is produced by the PSP using the
+HV-SIGNED key selected by the caller.
+
+The ``key_sel`` field indicates which key the platform will use to sign the
+report:
+  * ``0``: If VLEK is installed, sign with VLEK. Otherwise, sign with VCEK.
+  * ``1``: Sign with VCEK.
+  * ``2``: Sign with VLEK.
+  * Other values are reserved.
+
+Parameters (in): struct kvm_sev_snp_hv_report_req
+
+Returns:  0 on success, -negative on error
+
+::
+        struct kvm_sev_snp_hv_report_req {
+                __u64 report_uaddr;
+                __u64 report_len;
+                __u8 key_sel;
+                __u8 pad0[7];
+                __u64 pad1[4];
+        };
+
 Device attribute API
 ====================
 
diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
index 7ceff6583652..464146bed784 100644
--- a/arch/x86/include/uapi/asm/kvm.h
+++ b/arch/x86/include/uapi/asm/kvm.h
@@ -743,6 +743,7 @@ enum sev_cmd_id {
 	KVM_SEV_SNP_LAUNCH_START = 100,
 	KVM_SEV_SNP_LAUNCH_UPDATE,
 	KVM_SEV_SNP_LAUNCH_FINISH,
+	KVM_SEV_SNP_HV_REPORT_REQ,
 
 	KVM_SEV_NR_MAX,
 };
@@ -871,6 +872,14 @@ struct kvm_sev_receive_update_data {
 	__u32 pad2;
 };
 
+struct kvm_sev_snp_hv_report_req {
+	__u64 report_uaddr;
+	__u64 report_len;
+	__u8 key_sel;
+	__u8 pad0[7];
+	__u64 pad1[4];
+};
+
 struct kvm_sev_snp_launch_start {
 	__u64 policy;
 	__u8 gosvw[16];
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index f59c65abe3cf..ba7a07d132ff 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2261,6 +2261,63 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	return rc;
 }
 
+static int sev_snp_hv_report_request(struct kvm *kvm, struct kvm_sev_cmd *argp)
+{
+	struct sev_data_snp_msg_report_rsp *report_rsp = NULL;
+	struct sev_data_snp_hv_report_req data;
+	struct kvm_sev_snp_hv_report_req params;
+	struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
+	void __user *u_report;
+	void __user *u_params = u64_to_user_ptr(argp->data);
+	size_t rsp_size = sizeof(*report_rsp);
+	int ret;
+
+	if (!sev_snp_guest(kvm))
+		return -ENOTTY;
+	if (copy_from_user(&params, u_params, sizeof(params)))
+		return -EFAULT;
+
+	if (params.report_len < rsp_size)
+		return -ENOSPC;
+
+	u_report = u64_to_user_ptr(params.report_uaddr);
+	if (!u_report)
+		return -EINVAL;
+
+	report_rsp = snp_alloc_firmware_page(GFP_KERNEL_ACCOUNT | __GFP_ZERO);
+	if (!report_rsp)
+		return -ENOMEM;
+
+	data.len = sizeof(data);
+	data.key_sel = params.key_sel;
+	data.gctx_addr = __psp_pa(sev->snp_context);
+	data.hv_report_paddr = __psp_pa(report_rsp);
+
+	ret = sev_issue_cmd(kvm, SEV_CMD_SNP_HV_REPORT_REQ, &data,
+				&argp->error);
+	if (ret)
+		goto e_free_rsp;
+
+	if (!report_rsp->status)
+		rsp_size += report_rsp->report_size;
+
+	if (params.report_len < rsp_size) {
+		rsp_size = sizeof(*report_rsp);
+		ret = -ENOSPC;
+	}
+
+	if (copy_to_user(u_report, report_rsp, rsp_size))
+		ret = -EFAULT;
+
+	params.report_len = sizeof(*report_rsp) + report_rsp->report_size;
+	if (copy_to_user(u_params, &params, sizeof(params)))
+		ret = -EFAULT;
+
+e_free_rsp:
+	snp_free_firmware_page(report_rsp);
+	return ret;
+}
+
 struct sev_gmem_populate_args {
 	__u8 type;
 	int sev_fd;
@@ -2672,6 +2729,9 @@ int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp)
 	case KVM_SEV_SNP_LAUNCH_FINISH:
 		r = snp_launch_finish(kvm, &sev_cmd);
 		break;
+	case KVM_SEV_SNP_HV_REPORT_REQ:
+		r = sev_snp_hv_report_request(kvm, &sev_cmd);
+		break;
 	default:
 		r = -EINVAL;
 		goto out;
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 956ea609d0cc..5dd7c3f0d50d 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -259,6 +259,7 @@ static int sev_cmd_buffer_len(int cmd)
 	case SEV_CMD_SNP_COMMIT:		return sizeof(struct sev_data_snp_commit);
 	case SEV_CMD_SNP_FEATURE_INFO:		return sizeof(struct sev_data_snp_feature_info);
 	case SEV_CMD_SNP_VLEK_LOAD:		return sizeof(struct sev_user_data_snp_vlek_load);
+	case SEV_CMD_SNP_HV_REPORT_REQ:		return sizeof(struct sev_data_snp_hv_report_req);
 	default:				return sev_tio_cmd_buffer_len(cmd);
 	}
 
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index 69ffa4b4d1fa..c651a400d124 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -124,6 +124,7 @@ enum sev_cmd {
 	SEV_CMD_SNP_GCTX_CREATE		= 0x093,
 	SEV_CMD_SNP_GUEST_REQUEST	= 0x094,
 	SEV_CMD_SNP_ACTIVATE_EX		= 0x095,
+	SEV_CMD_SNP_HV_REPORT_REQ	= 0x096,
 	SEV_CMD_SNP_LAUNCH_START	= 0x0A0,
 	SEV_CMD_SNP_LAUNCH_UPDATE	= 0x0A1,
 	SEV_CMD_SNP_LAUNCH_FINISH	= 0x0A2,
@@ -594,6 +595,36 @@ struct sev_data_attestation_report {
 	u32 len;				/* In/Out */
 } __packed;
 
+/**
+ * struct sev_data_snp_hv_report_req - SNP_HV_REPORT_REQ command params
+ *
+ * @len: length of the command buffer in bytes
+ * @key_sel: Selects which key to use for generating the signature.
+ * @gctx_addr: System physical address of guest context page
+ * @hv_report_paddr: System physical address where MSG_EXPORT_RSP will be written
+ */
+struct sev_data_snp_hv_report_req {
+	u32 len;		/* In */
+	u32 key_sel	:2,	/* In */
+	    rsvd	:30;
+	u64 gctx_addr;		/* In */
+	u64 hv_report_paddr;	/* In */
+} __packed;
+
+/**
+ * struct sev_data_snp_msg_export_rsp
+ *
+ * @status: Status : 0h: Success. 16h: Invalid parameters.
+ * @report_size: Size in bytes of the attestation report
+ * @report: attestation report
+ */
+struct sev_data_snp_msg_report_rsp {
+	u32 status;			/* Out */
+	u32 report_size;		/* Out */
+	u8 rsvd[24];
+	u8 report[];
+} __packed;
+
 /**
  * struct sev_data_snp_download_firmware - SNP_DOWNLOAD_FIRMWARE command params
  *
-- 
2.52.0
Re: [PATCH v3] KVM: SEV: Add KVM_SEV_SNP_HV_REPORT_REQ command
Posted by Tom Lendacky 2 weeks, 4 days ago
On 12/15/25 08:14, Thomas Courrege wrote:
> Add support for retrieving the SEV-SNP attestation report via the
> SNP_HV_REPORT_REQ firmware command and expose it through a new KVM
> ioctl for SNP guests.
> 
> Signed-off-by: Thomas Courrege <thomas.courrege@vates.tech>
> ---
>  .../virt/kvm/x86/amd-memory-encryption.rst    | 27 +++++++++
>  arch/x86/include/uapi/asm/kvm.h               |  9 +++
>  arch/x86/kvm/svm/sev.c                        | 60 +++++++++++++++++++
>  drivers/crypto/ccp/sev-dev.c                  |  1 +
>  include/linux/psp-sev.h                       | 31 ++++++++++
>  5 files changed, 128 insertions(+)
> 
> diff --git a/Documentation/virt/kvm/x86/amd-memory-encryption.rst b/Documentation/virt/kvm/x86/amd-memory-encryption.rst
> index 1ddb6a86ce7f..083ed487764e 100644
> --- a/Documentation/virt/kvm/x86/amd-memory-encryption.rst
> +++ b/Documentation/virt/kvm/x86/amd-memory-encryption.rst
> @@ -572,6 +572,33 @@ Returns: 0 on success, -negative on error
>  See SNP_LAUNCH_FINISH in the SEV-SNP specification [snp-fw-abi]_ for further
>  details on the input parameters in ``struct kvm_sev_snp_launch_finish``.
>  
> +21. KVM_SEV_SNP_HV_REPORT_REQ
> +-----------------------------
> +
> +The KVM_SEV_SNP_HV_REPORT_REQ command requests the hypervisor-generated
> +SNP attestation report. This report is produced by the PSP using the
> +HV-SIGNED key selected by the caller.
> +
> +The ``key_sel`` field indicates which key the platform will use to sign the
> +report:
> +  * ``0``: If VLEK is installed, sign with VLEK. Otherwise, sign with VCEK.
> +  * ``1``: Sign with VCEK.
> +  * ``2``: Sign with VLEK.
> +  * Other values are reserved.
> +
> +Parameters (in): struct kvm_sev_snp_hv_report_req
> +
> +Returns:  0 on success, -negative on error
> +
> +::
> +        struct kvm_sev_snp_hv_report_req {
> +                __u64 report_uaddr;
> +                __u64 report_len;
> +                __u8 key_sel;
> +                __u8 pad0[7];
> +                __u64 pad1[4];
> +        };
> +
>  Device attribute API
>  ====================
>  
> diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
> index 7ceff6583652..464146bed784 100644
> --- a/arch/x86/include/uapi/asm/kvm.h
> +++ b/arch/x86/include/uapi/asm/kvm.h
> @@ -743,6 +743,7 @@ enum sev_cmd_id {
>  	KVM_SEV_SNP_LAUNCH_START = 100,
>  	KVM_SEV_SNP_LAUNCH_UPDATE,
>  	KVM_SEV_SNP_LAUNCH_FINISH,
> +	KVM_SEV_SNP_HV_REPORT_REQ,
>  
>  	KVM_SEV_NR_MAX,
>  };
> @@ -871,6 +872,14 @@ struct kvm_sev_receive_update_data {
>  	__u32 pad2;
>  };
>  
> +struct kvm_sev_snp_hv_report_req {
> +	__u64 report_uaddr;
> +	__u64 report_len;
> +	__u8 key_sel;
> +	__u8 pad0[7];
> +	__u64 pad1[4];
> +};
> +
>  struct kvm_sev_snp_launch_start {
>  	__u64 policy;
>  	__u8 gosvw[16];
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index f59c65abe3cf..ba7a07d132ff 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2261,6 +2261,63 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	return rc;
>  }
>  
> +static int sev_snp_hv_report_request(struct kvm *kvm, struct kvm_sev_cmd *argp)
> +{
> +	struct sev_data_snp_msg_report_rsp *report_rsp = NULL;

'= NULL' shouldn't be needed, right? Or do you get a warning because of
the use in the sizeof() below?

> +	struct sev_data_snp_hv_report_req data;
> +	struct kvm_sev_snp_hv_report_req params;
> +	struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
> +	void __user *u_report;
> +	void __user *u_params = u64_to_user_ptr(argp->data);

Do this assignment below just before the copy_from_user().

> +	size_t rsp_size = sizeof(*report_rsp);
> +	int ret;

The declarations above should be in reverse fir tree order.

> +
> +	if (!sev_snp_guest(kvm))
> +		return -ENOTTY;

Add a blank line here.

> +	if (copy_from_user(&params, u_params, sizeof(params)))
> +		return -EFAULT;
> +
> +	if (params.report_len < rsp_size)
> +		return -ENOSPC;
> +
> +	u_report = u64_to_user_ptr(params.report_uaddr);
> +	if (!u_report)
> +		return -EINVAL;
> +
> +	report_rsp = snp_alloc_firmware_page(GFP_KERNEL_ACCOUNT | __GFP_ZERO);
> +	if (!report_rsp)
> +		return -ENOMEM;
> +
> +	data.len = sizeof(data);
> +	data.key_sel = params.key_sel;
> +	data.gctx_addr = __psp_pa(sev->snp_context);
> +	data.hv_report_paddr = __psp_pa(report_rsp);

This would leave the 'rsvd' portion of 'data' unset and possibly
containing random data, so it should be zeroed.
> +
> +	ret = sev_issue_cmd(kvm, SEV_CMD_SNP_HV_REPORT_REQ, &data,
> +				&argp->error);

This last parameter should align with 'kvm' above it, but just make this
all one line.

> +	if (ret)
> +		goto e_free_rsp;
> +
> +	if (!report_rsp->status)
> +		rsp_size += report_rsp->report_size;
> +
> +	if (params.report_len < rsp_size) {
> +		rsp_size = sizeof(*report_rsp);
> +		ret = -ENOSPC;
> +	}

This can be contained within the if above it, right?

if (!report_rsp->status) {
	if (params.report_len < (rsp_size + report_rsp->report_size))
		ret = -ENOSPC;
	else
		rsp_size += report_rsp->report_size;
}

> +
> +	if (copy_to_user(u_report, report_rsp, rsp_size))
> +		ret = -EFAULT;
> +
> +	params.report_len = sizeof(*report_rsp) + report_rsp->report_size;

I'm not sure if we can rely on report_rsp->report_size being valid if
resport_rsp->status is not zero. So maybe just set this to rsp_size.

Thanks,
Tom

> +	if (copy_to_user(u_params, &params, sizeof(params)))
> +		ret = -EFAULT;
> +
> +e_free_rsp:
> +	snp_free_firmware_page(report_rsp);
> +	return ret;
> +}
> +
>  struct sev_gmem_populate_args {
>  	__u8 type;
>  	int sev_fd;
> @@ -2672,6 +2729,9 @@ int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp)
>  	case KVM_SEV_SNP_LAUNCH_FINISH:
>  		r = snp_launch_finish(kvm, &sev_cmd);
>  		break;
> +	case KVM_SEV_SNP_HV_REPORT_REQ:
> +		r = sev_snp_hv_report_request(kvm, &sev_cmd);
> +		break;
>  	default:
>  		r = -EINVAL;
>  		goto out;
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 956ea609d0cc..5dd7c3f0d50d 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -259,6 +259,7 @@ static int sev_cmd_buffer_len(int cmd)
>  	case SEV_CMD_SNP_COMMIT:		return sizeof(struct sev_data_snp_commit);
>  	case SEV_CMD_SNP_FEATURE_INFO:		return sizeof(struct sev_data_snp_feature_info);
>  	case SEV_CMD_SNP_VLEK_LOAD:		return sizeof(struct sev_user_data_snp_vlek_load);
> +	case SEV_CMD_SNP_HV_REPORT_REQ:		return sizeof(struct sev_data_snp_hv_report_req);
>  	default:				return sev_tio_cmd_buffer_len(cmd);
>  	}
>  
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index 69ffa4b4d1fa..c651a400d124 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -124,6 +124,7 @@ enum sev_cmd {
>  	SEV_CMD_SNP_GCTX_CREATE		= 0x093,
>  	SEV_CMD_SNP_GUEST_REQUEST	= 0x094,
>  	SEV_CMD_SNP_ACTIVATE_EX		= 0x095,
> +	SEV_CMD_SNP_HV_REPORT_REQ	= 0x096,
>  	SEV_CMD_SNP_LAUNCH_START	= 0x0A0,
>  	SEV_CMD_SNP_LAUNCH_UPDATE	= 0x0A1,
>  	SEV_CMD_SNP_LAUNCH_FINISH	= 0x0A2,
> @@ -594,6 +595,36 @@ struct sev_data_attestation_report {
>  	u32 len;				/* In/Out */
>  } __packed;
>  
> +/**
> + * struct sev_data_snp_hv_report_req - SNP_HV_REPORT_REQ command params
> + *
> + * @len: length of the command buffer in bytes
> + * @key_sel: Selects which key to use for generating the signature.
> + * @gctx_addr: System physical address of guest context page
> + * @hv_report_paddr: System physical address where MSG_EXPORT_RSP will be written
> + */
> +struct sev_data_snp_hv_report_req {
> +	u32 len;		/* In */
> +	u32 key_sel	:2,	/* In */
> +	    rsvd	:30;
> +	u64 gctx_addr;		/* In */
> +	u64 hv_report_paddr;	/* In */
> +} __packed;
> +
> +/**
> + * struct sev_data_snp_msg_export_rsp
> + *
> + * @status: Status : 0h: Success. 16h: Invalid parameters.
> + * @report_size: Size in bytes of the attestation report
> + * @report: attestation report
> + */
> +struct sev_data_snp_msg_report_rsp {
> +	u32 status;			/* Out */
> +	u32 report_size;		/* Out */
> +	u8 rsvd[24];
> +	u8 report[];
> +} __packed;
> +
>  /**
>   * struct sev_data_snp_download_firmware - SNP_DOWNLOAD_FIRMWARE command params
>   *
Re: [PATCH v3] KVM: SEV: Add KVM_SEV_SNP_HV_REPORT_REQ command
Posted by Thomas Courrege 2 weeks ago
Sorry, i didn't saw the response, i changed the email i use.

On 21-01-2026 00:45, Tom Lendacky wrote:
> On 12/15/25 08:14, Thomas Courrege wrote:
>
>> +	size_t rsp_size = sizeof(*report_rsp);
>> +	int ret;
> The declarations above should be in reverse fir tree order.
    
Like that ?
    struct sev_data_snp_msg_report_rsp *report_rsp;
    struct sev_data_snp_hv_report_req data;
    struct kvm_sev_snp_hv_report_req params;
    struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
    size_t rsp_size = sizeof(*report_rsp);
    void __user *u_report;
    void __user *u_params;
    int ret;
>> +	if (ret)
>> +		goto e_free_rsp;
>> +
>> +	if (!report_rsp->status)
>> +		rsp_size += report_rsp->report_size;
>> +
>> +	if (params.report_len < rsp_size) {
>> +		rsp_size = sizeof(*report_rsp);
>> +		ret = -ENOSPC;
>> +	}
> This can be contained within the if above it, right?
>
> if (!report_rsp->status) {
> 	if (params.report_len < (rsp_size + report_rsp->report_size))
> 		ret = -ENOSPC;
> 	else
> 		rsp_size += report_rsp->report_size;
> }

This leads to an error in case the user wants to query the report size.


Using params.report_len = 32, the nested if is true and thus the user get

back the default rsp_size (= 32), not increased with report_size (= 1184).

>> +
>> +	if (copy_to_user(u_report, report_rsp, rsp_size))
>> +		ret = -EFAULT;
>> +
>> +	params.report_len = sizeof(*report_rsp) + report_rsp->report_size;
> I'm not sure if we can rely on report_rsp->report_size being valid if
> resport_rsp->status is not zero. So maybe just set this to rsp_size.
>
> Thanks,
> Tom
maybe something like this ? to avoid copying on ENOSPC, where this issue come from

    if (!report_rsp->status)
        rsp_size += report_rsp->report_size;

    if (params.report_len < rsp_size) {
        ret = -ENOSPC;
    } else {
        if (copy_to_user(u_report, report_rsp, rsp_size))
            ret = -EFAULT;
    }

    params.report_len = rsp_size;


To test this specific case : 
    https://github.com/Th0rOnDoR/test-length-sev/blob/main/sev_test.c

Thanks, 
Thomas
Re: [PATCH v3] KVM: SEV: Add KVM_SEV_SNP_HV_REPORT_REQ command
Posted by Tom Lendacky 2 weeks ago
On 1/24/26 08:40, Thomas Courrege wrote:
> Sorry, i didn't saw the response, i changed the email i use.
> 
> On 21-01-2026 00:45, Tom Lendacky wrote:
>> On 12/15/25 08:14, Thomas Courrege wrote:
>>
>>> +	size_t rsp_size = sizeof(*report_rsp);
>>> +	int ret;
>> The declarations above should be in reverse fir tree order.
>     
> Like that ?
>     struct sev_data_snp_msg_report_rsp *report_rsp;
>     struct sev_data_snp_hv_report_req data;
>     struct kvm_sev_snp_hv_report_req params;
>     struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
>     size_t rsp_size = sizeof(*report_rsp);
>     void __user *u_report;
>     void __user *u_params;
>     int ret;

	struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
	struct sev_data_snp_msg_report_rsp *report_rsp;
	struct kvm_sev_snp_hv_report_req params;
	struct sev_data_snp_hv_report_req data;
	size_t rsp_size = sizeof(*report_rsp);
	void __user *u_report;
	void __user *u_params;
	int ret;

>>> +	if (ret)
>>> +		goto e_free_rsp;
>>> +
>>> +	if (!report_rsp->status)
>>> +		rsp_size += report_rsp->report_size;
>>> +
>>> +	if (params.report_len < rsp_size) {
>>> +		rsp_size = sizeof(*report_rsp);
>>> +		ret = -ENOSPC;
>>> +	}
>> This can be contained within the if above it, right?
>>
>> if (!report_rsp->status) {
>> 	if (params.report_len < (rsp_size + report_rsp->report_size))
>> 		ret = -ENOSPC;
>> 	else
>> 		rsp_size += report_rsp->report_size;
>> }
> 
> This leads to an error in case the user wants to query the report size.
> 
> 
> Using params.report_len = 32, the nested if is true and thus the user get
> 
> back the default rsp_size (= 32), not increased with report_size (= 1184).

But isn't params.report_len set below to the proper value since it wasn't
using rsp_size? The rsp_size variable is only used for the copy_to_user()
for the report itself. Assuming you want to copy what's in 'rsp' no matter
the return code you get, then can't you just do:

if (!report_rsp->status) {
	if (params.report_len < (rsp_size + report_rsp->report_size))
		ret = -ENOSPC;
	else
		rsp_size += report_rsp->report_size;

	params.report_len = sizeof(*report_rsp) + report_rsp->report_size;
}

if (copy_to_user(u_report, report_rsp, rsp_size))
	ret = -EFAULT;

Thanks,
Tom

> 
>>> +
>>> +	if (copy_to_user(u_report, report_rsp, rsp_size))
>>> +		ret = -EFAULT;
>>> +
>>> +	params.report_len = sizeof(*report_rsp) + report_rsp->report_size;
>> I'm not sure if we can rely on report_rsp->report_size being valid if
>> resport_rsp->status is not zero. So maybe just set this to rsp_size.
>>
>> Thanks,
>> Tom
> maybe something like this ? to avoid copying on ENOSPC, where this issue come from
> 
>     if (!report_rsp->status)
>         rsp_size += report_rsp->report_size;
> 
>     if (params.report_len < rsp_size) {
>         ret = -ENOSPC;
>     } else {
>         if (copy_to_user(u_report, report_rsp, rsp_size))
>             ret = -EFAULT;
>     }
> 
>     params.report_len = rsp_size;
> 
> 
> To test this specific case : 
>     https://github.com/Th0rOnDoR/test-length-sev/blob/main/sev_test.c
> 
> Thanks, 
> Thomas

Re: [PATCH v3] KVM: SEV: Add KVM_SEV_SNP_HV_REPORT_REQ command
Posted by xen 2 weeks ago
On 24-01-2026 17:27, Tom Lendacky wrote:
> On 1/24/26 08:40, Thomas Courrege wrote:
>> Sorry, i didn't saw the response, i changed the email i use.
>>
>> On 21-01-2026 00:45, Tom Lendacky wrote:
>>> On 12/15/25 08:14, Thomas Courrege wrote:
>>>
>>>> +	size_t rsp_size = sizeof(*report_rsp);
>>>> +	int ret;
>>> The declarations above should be in reverse fir tree order.
>>     
>> Like that ?
>>     struct sev_data_snp_msg_report_rsp *report_rsp;
>>     struct sev_data_snp_hv_report_req data;
>>     struct kvm_sev_snp_hv_report_req params;
>>     struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
>>     size_t rsp_size = sizeof(*report_rsp);
>>     void __user *u_report;
>>     void __user *u_params;
>>     int ret;
> 	struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
> 	struct sev_data_snp_msg_report_rsp *report_rsp;
> 	struct kvm_sev_snp_hv_report_req params;
> 	struct sev_data_snp_hv_report_req data;
> 	size_t rsp_size = sizeof(*report_rsp);
> 	void __user *u_report;
> 	void __user *u_params;
> 	int ret;
>
>>>> +	if (ret)
>>>> +		goto e_free_rsp;
>>>> +
>>>> +	if (!report_rsp->status)
>>>> +		rsp_size += report_rsp->report_size;
>>>> +
>>>> +	if (params.report_len < rsp_size) {
>>>> +		rsp_size = sizeof(*report_rsp);
>>>> +		ret = -ENOSPC;
>>>> +	}
>>> This can be contained within the if above it, right?
>>>
>>> if (!report_rsp->status) {
>>> 	if (params.report_len < (rsp_size + report_rsp->report_size))
>>> 		ret = -ENOSPC;
>>> 	else
>>> 		rsp_size += report_rsp->report_size;
>>> }
>> This leads to an error in case the user wants to query the report size.
>>
>>
>> Using params.report_len = 32, the nested if is true and thus the user get
>>
>> back the default rsp_size (= 32), not increased with report_size (= 1184).
> But isn't params.report_len set below to the proper value since it wasn't
> using rsp_size? The rsp_size variable is only used for the copy_to_user()
> for the report itself. Assuming you want to copy what's in 'rsp' no matter
> the return code you get, then can't you just do:
>
> if (!report_rsp->status) {
> 	if (params.report_len < (rsp_size + report_rsp->report_size))
> 		ret = -ENOSPC;
> 	else
> 		rsp_size += report_rsp->report_size;
>
> 	params.report_len = sizeof(*report_rsp) + report_rsp->report_size;
> }
>
> if (copy_to_user(u_report, report_rsp, rsp_size))
> 	ret = -EFAULT;
>
> Thanks,
> Tom
That's a good solution, thank you.

I'll also add a patch note for the next one.

Thanks,
Thomas
>>>> +
>>>> +	if (copy_to_user(u_report, report_rsp, rsp_size))
>>>> +		ret = -EFAULT;
>>>> +
>>>> +	params.report_len = sizeof(*report_rsp) + report_rsp->report_size;
>>> I'm not sure if we can rely on report_rsp->report_size being valid if
>>> resport_rsp->status is not zero. So maybe just set this to rsp_size.
>>>
>>> Thanks,
>>> Tom
>> maybe something like this ? to avoid copying on ENOSPC, where this issue come from
>>
>>     if (!report_rsp->status)
>>         rsp_size += report_rsp->report_size;
>>
>>     if (params.report_len < rsp_size) {
>>         ret = -ENOSPC;
>>     } else {
>>         if (copy_to_user(u_report, report_rsp, rsp_size))
>>             ret = -EFAULT;
>>     }
>>
>>     params.report_len = rsp_size;
>>
>>
>> To test this specific case : 
>>     https://github.com/Th0rOnDoR/test-length-sev/blob/main/sev_test.c
>>
>> Thanks, 
>> Thomas
>
Re: [PATCH v3] KVM: SEV: Add KVM_SEV_SNP_HV_REPORT_REQ command
Posted by Thomas Courrege 3 weeks, 5 days ago
Gentle ping.

Thanks,
Thomas