lkml.org 
[lkml]   [2022]   [Nov]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v8 4/4] virt: sev-guest: interpret VMM errors from guest request
From
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@amd.com>
> Cc: Peter Gonda <pgonda@google.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Liam Merwick <liam.merwick@oracle.com>
> Cc: Yang Yingliang <yangyingliang@huawei.com>
> Cc: Haowen Bai <baihaowen@meizu.com>
>
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.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_ */

\
 
 \ /
  Last update: 2022-11-14 22:22    [W:1.706 / U:0.068 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site