| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3cb5d149-699a-b649-1986-23d7f6783e6f@amd.com>
Date: Tue, 21 Feb 2023 09:43:19 -0600
From: Tom Lendacky <thomas.lendacky@....com>
To: Borislav Petkov <bp@...en8.de>, LKML <linux-kernel@...r.kernel.org>
Cc: Dionna Glaze <dionnaglaze@...gle.com>,
Joerg Roedel <jroedel@...e.de>,
Michael Roth <michael.roth@....com>,
Nikunj A Dadhania <nikunj@....com>,
Peter Gonda <pgonda@...gle.com>, linux-coco@...ts.linux.dev,
x86@...nel.org
Subject: Re: [PATCH -v2 11/11] x86/sev: Change snp_guest_issue_request()'s
fw_err argument
On 2/21/23 05:34, Borislav Petkov wrote:
> From: Dionna Glaze <dionnaglaze@...gle.com>
>
> The GHCB specification declares that the firmware error value for
> a guest request will be stored in the lower 32 bits of EXIT_INFO_2. The
> upper 32 bits are for the VMM's own error code. The fw_err argument to
> snp_guest_issue_request() is thus a misnomer, and callers will need
> access to all 64 bits.
>
> The type of unsigned long also causes problems, since sw_exit_info2 is
> u64 (unsigned long long) vs the argument's unsigned long*. Change this
> type for issuing the guest request. Pass the ioctl command struct's error
> field directly instead of in a local variable, since an incomplete guest
> request may not set the error code, and uninitialized stack memory would
> be written back to user space.
>
> The firmware might not even be called, so bookend the call with the no
> firmware call error and clear the error.
>
> Since the "fw_err" field is really exitinfo2 split into the upper bits'
> vmm error code and lower bits' firmware error code, convert the 64 bit
> value to a union.
>
> [ bp: Massage commit message, adjust code. ]
>
> Signed-off-by: Dionna Glaze <dionnaglaze@...gle.com>
> Signed-off-by: Borislav Petkov (AMD) <bp@...en8.de>
> Link: https://lore.kernel.org/r/20230214164638.1189804-5-dionnaglaze@google.com
> ---
> Documentation/virt/coco/sev-guest.rst | 20 ++++++----
> arch/x86/include/asm/sev-common.h | 4 --
> arch/x86/include/asm/sev.h | 8 ++--
> arch/x86/kernel/sev.c | 15 ++++----
> drivers/virt/coco/sev-guest/sev-guest.c | 49 +++++++++++++------------
> include/uapi/linux/sev-guest.h | 18 ++++++++-
> 6 files changed, 68 insertions(+), 46 deletions(-)
>
> diff --git a/Documentation/virt/coco/sev-guest.rst b/Documentation/virt/coco/sev-guest.rst
> index aa3e4c6a1f90..68b0d2363af8 100644
> --- a/Documentation/virt/coco/sev-guest.rst
> +++ b/Documentation/virt/coco/sev-guest.rst
> @@ -37,11 +37,11 @@ along with a description:
> the return value. General error numbers (-ENOMEM, -EINVAL)
> are not detailed, but errors with specific meanings are.
>
> -The guest ioctl should be issued on a file descriptor of the /dev/sev-guest device.
> -The ioctl accepts struct snp_user_guest_request. The input and output structure is
> -specified through the req_data and resp_data field respectively. If the ioctl fails
> -to execute due to a firmware error, then fw_err code will be set. Otherwise, fw_err
> -will be set to 0x00000000ffffffff, i.e., the lower 32-bits are -1.
> +The guest ioctl should be issued on a file descriptor of the /dev/sev-guest
> +device. The ioctl accepts struct snp_user_guest_request. The input and
> +output structure is specified through the req_data and resp_data field
> +respectively. If the ioctl fails to execute due to a firmware error, then
> +the fw_error code will be set, otherwise fw_error will be set to -1.
>
> The firmware checks that the message sequence counter is one greater than
> the guests message sequence counter. If guest driver fails to increment message
> @@ -57,8 +57,14 @@ counter (e.g. counter overflow), then -EIO will be returned.
> __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;
> + struct {
> + __u32 fw_error;
> + __u32 vmm_error;
> + };
> + };
> };
>
> 2.1 SNP_GET_REPORT
> diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
> index b63be696b776..0759af9b1acf 100644
> --- a/arch/x86/include/asm/sev-common.h
> +++ b/arch/x86/include/asm/sev-common.h
> @@ -128,10 +128,6 @@ struct snp_psc_desc {
> struct psc_entry entries[VMGEXIT_PSC_MAX_ENTRY];
> } __packed;
>
> -/* Guest message request error codes */
> -#define SNP_GUEST_REQ_INVALID_LEN BIT_ULL(32)
> -#define SNP_GUEST_REQ_ERR_BUSY BIT_ULL(33)
> -
> #define GHCB_MSR_TERM_REQ 0x100
> #define GHCB_MSR_TERM_REASON_SET_POS 12
> #define GHCB_MSR_TERM_REASON_SET_MASK 0xf
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index ebc271bb6d8e..30567c72cbd9 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -185,6 +185,9 @@ static inline int pvalidate(unsigned long vaddr, bool rmp_psize, bool validate)
>
> return rc;
> }
> +
> +struct snp_guest_request_ioctl;
> +
> void setup_ghcb(void);
> void __init early_snp_set_memory_private(unsigned long vaddr, unsigned long paddr,
> unsigned int npages);
> @@ -196,7 +199,7 @@ void snp_set_memory_private(unsigned long vaddr, unsigned int npages);
> void snp_set_wakeup_secondary_cpu(void);
> bool snp_init(struct boot_params *bp);
> void __init __noreturn snp_abort(void);
> -int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned long *fw_err);
> +int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, struct snp_guest_request_ioctl *rio);
> #else
> static inline void sev_es_ist_enter(struct pt_regs *regs) { }
> static inline void sev_es_ist_exit(void) { }
> @@ -216,8 +219,7 @@ static inline void snp_set_memory_private(unsigned long vaddr, unsigned int npag
> static inline void snp_set_wakeup_secondary_cpu(void) { }
> static inline bool snp_init(struct boot_params *bp) { return false; }
> static inline void snp_abort(void) { }
> -static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input,
> - unsigned long *fw_err)
> +static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, struct snp_guest_request_ioctl *rio)
> {
> return -ENOTTY;
> }
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index 3f664ab277c4..b031244d6d2d 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -22,6 +22,8 @@
> #include <linux/efi.h>
> #include <linux/platform_device.h>
> #include <linux/io.h>
> +#include <linux/psp-sev.h>
> +#include <uapi/linux/sev-guest.h>
>
> #include <asm/cpu_entry_area.h>
> #include <asm/stacktrace.h>
> @@ -2175,7 +2177,7 @@ static int __init init_sev_config(char *str)
> }
> __setup("sev=", init_sev_config);
>
> -int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned long *fw_err)
> +int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, struct snp_guest_request_ioctl *rio)
> {
> struct ghcb_state state;
> struct es_em_ctxt ctxt;
> @@ -2183,8 +2185,7 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned
> struct ghcb *ghcb;
> int ret;
>
> - if (!fw_err)
> - return -EINVAL;
> + rio->exitinfo2 = SEV_RET_NO_FW_CALL;
>
> /*
> * __sev_get_ghcb() needs to run with IRQs disabled because it is using
> @@ -2209,16 +2210,16 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned
> if (ret)
> goto e_put;
>
> - *fw_err = ghcb->save.sw_exit_info_2;
> - switch (*fw_err) {
> + rio->exitinfo2 = ghcb->save.sw_exit_info_2;
> + switch (rio->exitinfo2) {
> case 0:
> break;
>
> - case SNP_GUEST_REQ_ERR_BUSY:
> + case SNP_GUEST_VMM_ERR(SNP_GUEST_VMM_ERR_BUSY):
> ret = -EAGAIN;
> break;
>
> - case SNP_GUEST_REQ_INVALID_LEN:
> + case SNP_GUEST_VMM_ERR(SNP_GUEST_VMM_ERR_INVALID_LEN):
> /* Number of expected pages are returned in RBX */
> if (exit_code == SVM_VMGEXIT_EXT_GUEST_REQUEST) {
> input->data_npages = ghcb_get_rbx(ghcb);
> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> index 6bc1390b54e8..6b3c1d308353 100644
> --- a/drivers/virt/coco/sev-guest/sev-guest.c
> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> @@ -332,11 +332,12 @@ static int enc_payload(struct snp_guest_dev *snp_dev, u64 seqno, int version, u8
> return __enc_payload(snp_dev, req, payload, sz);
> }
>
> -static int __handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, __u64 *fw_err)
> +static int __handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,
> + struct snp_guest_request_ioctl *rio)
> {
> - unsigned long err = 0xff, override_err = 0;
> unsigned long req_start = jiffies;
> unsigned int override_npages = 0;
> + u64 override_err = 0;
> int rc;
>
> retry_request:
> @@ -346,7 +347,7 @@ static int __handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,
> * sequence number must be incremented or the VMPCK must be deleted to
> * prevent reuse of the IV.
> */
> - rc = snp_issue_guest_request(exit_code, &snp_dev->input, &err);
> + rc = snp_issue_guest_request(exit_code, &snp_dev->input, rio);
> switch (rc) {
> case -ENOSPC:
> /*
> @@ -364,7 +365,7 @@ static int __handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,
> * request buffer size was too small and give the caller the
> * required buffer size.
> */
> - override_err = SNP_GUEST_REQ_INVALID_LEN;
> + override_err = SNP_GUEST_VMM_ERR_INVALID_LEN << SNP_GUEST_VMM_ERR_SHIFT;
Would it be better to do?:
override_err = SNP_GUEST_VMM_ERR(SNP_GUEST_VMM_ERR_INVALID_LEN);
>
> /*
> * If this call to the firmware succeeds, the sequence number can
> @@ -377,7 +378,7 @@ static int __handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,
> goto retry_request;
>
> /*
> - * The host may return SNP_GUEST_REQ_ERR_EBUSY if the request has been
> + * The host may return SNP_GUEST_VMM_ERR_BUSY if the request has been
> * throttled. Retry in the driver to avoid returning and reusing the
> * message sequence number on a different message.
> */
> @@ -390,8 +391,8 @@ static int __handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,
> goto retry_request;
> }
>
> - if (fw_err)
> - *fw_err = override_err ?: err;
> + if (override_err)
> + rio->exitinfo2 = override_err;
>
> if (override_npages)
> snp_dev->input.data_npages = override_npages;
> @@ -399,9 +400,10 @@ static int __handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,
> return rc;
> }
>
> -static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, int msg_ver,
> - u8 type, void *req_buf, size_t req_sz, void *resp_buf,
> - u32 resp_sz, __u64 *fw_err)
> +static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,
> + struct snp_guest_request_ioctl *rio, u8 type,
> + void *req_buf, size_t req_sz, void *resp_buf,
> + u32 resp_sz)
> {
> u64 seqno;
> int rc;
> @@ -415,7 +417,7 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, in
> memset(snp_dev->response, 0, sizeof(struct snp_guest_msg));
>
> /* Encrypt the userspace provided payload in snp_dev->secret_request. */
> - rc = enc_payload(snp_dev, seqno, msg_ver, type, req_buf, req_sz);
> + rc = enc_payload(snp_dev, seqno, rio->msg_version, type, req_buf, req_sz);
> if (rc)
> return rc;
>
> @@ -426,9 +428,11 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, in
> memcpy(snp_dev->request, &snp_dev->secret_request,
> sizeof(snp_dev->secret_request));
>
> - rc = __handle_guest_request(snp_dev, exit_code, fw_err);
> + rc = __handle_guest_request(snp_dev, exit_code, rio);
> if (rc) {
> - dev_alert(snp_dev->dev, "Detected error from ASP request. rc: %d, fw_err: %llu\n", rc, *fw_err);
> + dev_alert(snp_dev->dev,
> + "Detected error from ASP request. rc: %d, exitinfo2: %llu\n",
> + rc, rio->exitinfo2);
> snp_disable_vmpck(snp_dev);
> return rc;
> }
> @@ -471,9 +475,9 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
> if (!resp)
> return -ENOMEM;
>
> - rc = handle_guest_request(snp_dev, SVM_VMGEXIT_GUEST_REQUEST, arg->msg_version,
> + rc = handle_guest_request(snp_dev, SVM_VMGEXIT_GUEST_REQUEST, arg,
> SNP_MSG_REPORT_REQ, &req, sizeof(req), resp->data,
> - resp_len, &arg->fw_err);
> + resp_len);
> if (rc)
> goto e_free;
>
> @@ -511,9 +515,8 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque
> if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
> return -EFAULT;
>
> - rc = handle_guest_request(snp_dev, SVM_VMGEXIT_GUEST_REQUEST, arg->msg_version,
> - SNP_MSG_KEY_REQ, &req, sizeof(req), buf, resp_len,
> - &arg->fw_err);
> + rc = handle_guest_request(snp_dev, SVM_VMGEXIT_GUEST_REQUEST, arg,
> + SNP_MSG_KEY_REQ, &req, sizeof(req), buf, resp_len);
> if (rc)
> return rc;
>
> @@ -573,12 +576,12 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
> return -ENOMEM;
>
> snp_dev->input.data_npages = npages;
> - ret = handle_guest_request(snp_dev, SVM_VMGEXIT_EXT_GUEST_REQUEST, arg->msg_version,
> + ret = handle_guest_request(snp_dev, SVM_VMGEXIT_EXT_GUEST_REQUEST, arg,
> SNP_MSG_REPORT_REQ, &req.data,
> - sizeof(req.data), resp->data, resp_len, &arg->fw_err);
> + 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)))
> @@ -613,7 +616,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 = 0xff;
Should this be?
input.exitinfo2 = SEV_RET_NO_FW_CALL;
or make it part of patch #1?
>
> /* Message version must be non-zero */
> if (!input.msg_version)
> @@ -644,7 +647,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;
> diff --git a/include/uapi/linux/sev-guest.h b/include/uapi/linux/sev-guest.h
> index 256aaeff7e65..b25b728281ca 100644
> --- a/include/uapi/linux/sev-guest.h
> +++ b/include/uapi/linux/sev-guest.h
> @@ -52,8 +52,14 @@ 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;
> + struct {
> + __u32 fw_error;
> + __u32 vmm_error;
> + };
> + };
> };
>
> struct snp_ext_report_req {
> @@ -77,4 +83,12 @@ 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(x) (((u64)x) << SNP_GUEST_VMM_ERR_SHIFT)
> +
> +#define SNP_GUEST_VMM_ERR_INVALID_LEN BIT(0)
> +#define SNP_GUEST_VMM_ERR_BUSY BIT(1)
These are actually supposed to be numbers, not bits. It works out that
INVALID_LEN is 1 and BUSY is 2, but the next value (in the GHCB spec) will
be 3, so lets change these to 1 and 2.
This could be fixed in patch #9 or here as/at the final change.
Thanks,
Tom
> +
> #endif /* __UAPI_LINUX_SEV_GUEST_H_ */
Powered by blists - more mailing lists