Message-ID: <b332c967-cbe5-891a-3d15-f9fbb514d8e6@amd.com>
Date:   Mon, 14 Nov 2022 15:21:17 -0600
From:   Tom Lendacky <thomas.lendacky@....com>
To:     Dionna Glaze <dionnaglaze@...gle.com>,
        linux-kernel@...r.kernel.org, x86@...nel.org
Cc:     Peter Gonda <pgonda@...gle.com>, Borislav Petkov <bp@...e.de>,
        Liam Merwick <liam.merwick@...cle.com>,
        Yang Yingliang <yangyingliang@...wei.com>,
        Haowen Bai <baihaowen@...zu.com>
Subject: Re: [PATCH v8 4/4] virt: sev-guest: interpret VMM errors from guest
 request

On 11/4/22 18:00, Dionna Glaze wrote:
> The GHCB specification states that the upper 32 bits of exitinfo2 are
> for the VMM's error codes. The sev-guest ABI has already locked in
> that the fw_err status of the input will be 64 bits, and that
> BIT_ULL(32) means that the extended guest request's data buffer was too
> small, so we have to keep that ABI.
> 
> We can still interpret the upper 32 bits of exitinfo2 for the user
> anyway in case the request gets throttled. For safety, since the
> encryption algorithm in GHCBv2 is AES_GCM, we cannot return to user
> space without having completed the request with the current sequence
> number. If we were to return and the guest were to make another request
> but with different message contents, then that would be IV reuse.
> 
> When throttled, the driver will reschedule itself and then try
> again after sleeping half its ratelimit time to avoid a big wait queue.
> The ioctl may block indefinitely, but that has always been the case
> when deferring these requests to the host.
> 
> Cc: Tom Lendacky <Thomas.Lendacky@....com>
> Cc: Peter Gonda <pgonda@...gle.com>
> Cc: Borislav Petkov <bp@...e.de>
> Cc: Tom Lendacky <thomas.lendacky@....com>
> Cc: Liam Merwick <liam.merwick@...cle.com>
> Cc: Yang Yingliang <yangyingliang@...wei.com>
> Cc: Haowen Bai <baihaowen@...zu.com>
> 
> Signed-off-by: Dionna Glaze <dionnaglaze@...gle.com>

Reviewed-by: Tom Lendacky <thomas.lendacky@....com>

I'm wondering if this should be targeted at stable so that older kernels 
will be able to handle a host that returns a busy indicator without 
destroying the key (Peter's IV re-use patch).

Thanks,
Tom

> ---
>   drivers/virt/coco/sev-guest/sev-guest.c | 49 ++++++++++++++++++++-----
>   include/uapi/linux/sev-guest.h          | 18 ++++++++-
>   2 files changed, 56 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> index 5615d349b378..e8a9c07ea897 100644
> --- a/drivers/virt/coco/sev-guest/sev-guest.c
> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> @@ -14,6 +14,7 @@
>   #include <linux/io.h>
>   #include <linux/platform_device.h>
>   #include <linux/miscdevice.h>
> +#include <linux/ratelimit.h>
>   #include <linux/set_memory.h>
>   #include <linux/fs.h>
>   #include <crypto/aead.h>
> @@ -48,12 +49,22 @@ struct snp_guest_dev {
>   	struct snp_req_data input;
>   	u32 *os_area_msg_seqno;
>   	u8 *vmpck;
> +
> +	struct ratelimit_state rs;
>   };
>   
>   static u32 vmpck_id;
>   module_param(vmpck_id, uint, 0444);
>   MODULE_PARM_DESC(vmpck_id, "The VMPCK ID to use when communicating with the PSP.");
>   
> +static int rate_s = 1;
> +module_param(rate_s, int, 0444);
> +MODULE_PARM_DESC(rate_s, "The rate limit interval in seconds to limit requests to.");
> +
> +static int rate_burst = 2;
> +module_param(rate_burst, int, 0444);
> +MODULE_PARM_DESC(rate_burst, "The rate limit burst amount to limit requests to.");
> +
>   /* Mutex to serialize the shared buffer access and command handling. */
>   static DEFINE_MUTEX(snp_cmd_mutex);
>   
> @@ -341,9 +352,27 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,
>   	if (rc)
>   		return rc;
>   
> +retry:
> +	/*
> +	 * Rate limit commands internally since the host can also throttle, and
> +	 * we don't want to create a tight request spin that could end up
> +	 * getting this VM throttled more heavily.
> +	 */
> +	if (!__ratelimit(&snp_dev->rs)) {
> +		schedule_timeout_interruptible((rate_s * HZ) / 2);
> +		goto retry;
> +	}
>   	/* Call firmware to process the request */
>   	rc = snp_issue_guest_request(exit_code, &snp_dev->input,
> -				     &arg->fw_err);
> +				     &arg->exitinfo2);
> +
> +	/*
> +	 * The host may return EBUSY if the request has been throttled.
> +	 * We retry in the driver to avoid returning and reusing the message
> +	 * sequence number on a different message.
> +	 */
> +	if (arg->vmm_error == SNP_GUEST_VMM_ERR_BUSY)
> +		goto retry;
>   
>   	/*
>   	 * If the extended guest request fails due to having to small of a
> @@ -351,21 +380,21 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,
>   	 * extended data request.
>   	 */
>   	if (exit_code == SVM_VMGEXIT_EXT_GUEST_REQUEST &&
> -		arg->fw_err == SNP_GUEST_REQ_INVALID_LEN) {
> +		arg->vmm_error == SNP_GUEST_VMM_ERR_INVALID_LEN) {
>   		const unsigned int certs_npages = snp_dev->input.data_npages;
>   
>   		exit_code = SVM_VMGEXIT_GUEST_REQUEST;
>   		rc = snp_issue_guest_request(exit_code, &snp_dev->input,
> -					     &arg->fw_err);
> +					     &arg->exitinfo2);
>   
> -		arg->fw_err = SNP_GUEST_REQ_INVALID_LEN;
> +		arg->vmm_error = SNP_GUEST_VMM_ERR_INVALID_LEN;
>   		snp_dev->input.data_npages = certs_npages;
>   	}
>   
>   	if (rc) {
>   		dev_alert(snp_dev->dev,
> -			  "Detected error from ASP request. rc: %d, fw_err: %llu\n",
> -			  rc, arg->fw_err);
> +			  "Detected error from ASP request. rc: %d, exitinfo2: %llu\n",
> +			  rc, arg->exitinfo2);
>   		goto disable_vmpck;
>   	}
>   
> @@ -518,7 +547,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
>   				   sizeof(req.data), resp->data, resp_len);
>   
>   	/* If certs length is invalid then copy the returned length */
> -	if (arg->fw_err == SNP_GUEST_REQ_INVALID_LEN) {
> +	if (arg->vmm_error == SNP_GUEST_VMM_ERR_INVALID_LEN) {
>   		req.certs_len = snp_dev->input.data_npages << PAGE_SHIFT;
>   
>   		if (copy_to_user((void __user *)arg->req_data, &req, sizeof(req)))
> @@ -553,7 +582,7 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long
>   	if (copy_from_user(&input, argp, sizeof(input)))
>   		return -EFAULT;
>   
> -	input.fw_err = 0xff;
> +	input.exitinfo2 = SEV_RET_NO_FW_CALL;
>   
>   	/* Message version must be non-zero */
>   	if (!input.msg_version)
> @@ -584,7 +613,7 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long
>   
>   	mutex_unlock(&snp_cmd_mutex);
>   
> -	if (input.fw_err && copy_to_user(argp, &input, sizeof(input)))
> +	if (input.exitinfo2 && copy_to_user(argp, &input, sizeof(input)))
>   		return -EFAULT;
>   
>   	return ret;
> @@ -734,6 +763,8 @@ static int __init sev_guest_probe(struct platform_device *pdev)
>   	if (ret)
>   		goto e_free_cert_data;
>   
> +	ratelimit_state_init(&snp_dev->rs, rate_s * HZ, rate_burst);
> +
>   	dev_info(dev, "Initialized SEV guest driver (using vmpck_id %d)\n", vmpck_id);
>   	return 0;
>   
> diff --git a/include/uapi/linux/sev-guest.h b/include/uapi/linux/sev-guest.h
> index 256aaeff7e65..8e4144aa78c9 100644
> --- a/include/uapi/linux/sev-guest.h
> +++ b/include/uapi/linux/sev-guest.h
> @@ -52,8 +52,15 @@ struct snp_guest_request_ioctl {
>   	__u64 req_data;
>   	__u64 resp_data;
>   
> -	/* firmware error code on failure (see psp-sev.h) */
> -	__u64 fw_err;
> +	/* bits[63:32]: VMM error code, bits[31:0] firmware error code (see psp-sev.h) */
> +	union {
> +		__u64 exitinfo2;
> +		__u64 fw_err; /* Name deprecated in favor of others */
> +		struct {
> +			__u32 fw_error;
> +			__u32 vmm_error;
> +		};
> +	};
>   };
>   
>   struct snp_ext_report_req {
> @@ -77,4 +84,11 @@ struct snp_ext_report_req {
>   /* Get SNP extended report as defined in the GHCB specification version 2. */
>   #define SNP_GET_EXT_REPORT _IOWR(SNP_GUEST_REQ_IOC_TYPE, 0x2, struct snp_guest_request_ioctl)
>   
> +/* Guest message request EXIT_INFO_2 constants */
> +#define SNP_GUEST_FW_ERR_MASK		GENMASK_ULL(31, 0)
> +#define SNP_GUEST_VMM_ERR_SHIFT		32
> +
> +#define SNP_GUEST_VMM_ERR_INVALID_LEN	1
> +#define SNP_GUEST_VMM_ERR_BUSY		2
> +
>   #endif /* __UAPI_LINUX_SEV_GUEST_H_ */

Powered by blists - more mailing lists