| Message ID | 20211210154332.11526-37-brijesh.singh@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Guest Support | expand |
On Fri, Dec 10, 2021 at 09:43:28AM -0600, Brijesh Singh wrote: > Version 2 of GHCB specification provides SNP_GUEST_REQUEST and > SNP_EXT_GUEST_REQUEST NAE that can be used by the SNP guest to communicate > with the PSP. > > While at it, add a snp_issue_guest_request() helper that can be used by Not "that can" but "that will". > driver or other subsystem to issue the request to PSP. > > See SEV-SNP and GHCB spec for more details. > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > arch/x86/include/asm/sev-common.h | 3 ++ > arch/x86/include/asm/sev.h | 14 +++++++++ > arch/x86/include/uapi/asm/svm.h | 4 +++ > arch/x86/kernel/sev.c | 51 +++++++++++++++++++++++++++++++ > 4 files changed, 72 insertions(+) > > diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h > index 673e6778194b..346600724b84 100644 > --- a/arch/x86/include/asm/sev-common.h > +++ b/arch/x86/include/asm/sev-common.h > @@ -128,6 +128,9 @@ struct snp_psc_desc { > struct psc_entry entries[VMGEXIT_PSC_MAX_ENTRY]; > } __packed; > > +/* Guest message request error code */ > +#define SNP_GUEST_REQ_INVALID_LEN BIT_ULL(32) SZ_4G is more descriptive, perhaps... > +int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned long *fw_err) > +{ > + struct ghcb_state state; > + unsigned long flags; > + struct ghcb *ghcb; > + int ret; > + > + if (!cc_platform_has(CC_ATTR_SEV_SNP)) > + return -ENODEV; > + > + /* __sev_get_ghcb() need to run with IRQs disabled because it using per-cpu GHCB */ needs it is using a > + local_irq_save(flags); > + > + ghcb = __sev_get_ghcb(&state); > + if (!ghcb) { > + ret = -EIO; > + goto e_restore_irq; > + } > + > + vc_ghcb_invalidate(ghcb); > + > + if (exit_code == SVM_VMGEXIT_EXT_GUEST_REQUEST) { > + ghcb_set_rax(ghcb, input->data_gpa); > + ghcb_set_rbx(ghcb, input->data_npages); > + } > + > + ret = sev_es_ghcb_hv_call(ghcb, true, NULL, exit_code, input->req_gpa, input->resp_gpa); ^^^^^ That's ctxt which is accessed without a NULL check in verify_exception_info(). Why aren't you allocating a ctxt on stack like the other callers do?
On 1/27/22 10:21 AM, Borislav Petkov wrote: > On Fri, Dec 10, 2021 at 09:43:28AM -0600, Brijesh Singh wrote: >> Version 2 of GHCB specification provides SNP_GUEST_REQUEST and >> SNP_EXT_GUEST_REQUEST NAE that can be used by the SNP guest to communicate >> with the PSP. >> >> While at it, add a snp_issue_guest_request() helper that can be used by > > Not "that can" but "that will". > Noted. >> >> +/* Guest message request error code */ >> +#define SNP_GUEST_REQ_INVALID_LEN BIT_ULL(32) > > SZ_4G is more descriptive, perhaps... > I am okay with using SZ_4G but per the spec they don't spell that its 4G size. It says bit 32 will should be set on error. >> + >> + ret = sev_es_ghcb_hv_call(ghcb, true, NULL, exit_code, input->req_gpa, input->resp_gpa); > ^^^^^ > > That's ctxt which is accessed without a NULL check in > verify_exception_info(). > > Why aren't you allocating a ctxt on stack like the other callers do? Typically the sev_es_ghcb_hv_handler() is called from #VC handler, which provides the context structure. But in this and PSC case, the caller is not a #VC handler, so we don't have a context structure. But as you pointed, we could allocate context structure on the stack and pass it down so that verify_exception_info() does not cause a panic with NULL deference (when HV violates the spec and inject exception while handling this NAE). thanks
On Thu, Jan 27, 2022 at 11:02:13AM -0600, Brijesh Singh wrote: > I am okay with using SZ_4G but per the spec they don't spell that its 4G > size. It says bit 32 will should be set on error. What does the speck call it exactly? Is it "length"? Because that's what confused me: SNP_GUEST_REQ_INVALID_LEN - that's a length and length you don't usually specify with a bit position... > Typically the sev_es_ghcb_hv_handler() is called from #VC handler, which > provides the context structure. But in this and PSC case, the caller is not > a #VC handler, so we don't have a context structure. But as you pointed, we > could allocate context structure on the stack and pass it down so that > verify_exception_info() does not cause a panic with NULL deference (when HV > violates the spec and inject exception while handling this NAE). Yap, exactly.
On 1/29/22 4:27 AM, Borislav Petkov wrote: > On Thu, Jan 27, 2022 at 11:02:13AM -0600, Brijesh Singh wrote: >> I am okay with using SZ_4G but per the spec they don't spell that its 4G >> size. It says bit 32 will should be set on error. > What does the speck call it exactly? Is it "length"? Because that's what > confused me: SNP_GUEST_REQ_INVALID_LEN - that's a length and length you > don't usually specify with a bit position... Here is the text from the spec: ---------- The hypervisor must validate that the guest has supplied enough pages to hold the certificates that will be returned before performing the SNP guest request. If there are not enough guest pages to hold the certificate table and certificate data, the hypervisor will return the required number of pages needed to hold the certificate table and certificate data in the RBX register and set the SW_EXITINFO2 field to 0x0000000100000000. --------- It does not spell it as invalid length. However, for *similar* failure, the SEV-SNP spec spells out it as INVALID_LENGTH, so, I choose macro name as INVALID_LENGTH. thanks
On Sat, Jan 29, 2022 at 05:49:06AM -0600, Brijesh Singh wrote: > The hypervisor must validate that the guest has supplied enough pages to > hold the certificates that will be returned before performing the SNP > guest request. If there are not enough guest pages to hold the > certificate table and certificate data, the hypervisor will return the > required number of pages needed to hold the certificate table and > certificate data in the RBX register and set the SW_EXITINFO2 field to > 0x0000000100000000. Then you could call that one: #define SNP_GUEST_REQ_ERR_NUM_PAGES BIT_ULL(32) or so, to mean what exactly that error is. Because when you read the code, it is more "self-descriptive" this way: ... ghcb->save.sw_exit_info_2 == SNP_GUEST_REQ_ERR_NUM_PAGES) input->data_npages = ghcb_get_rbx(ghcb); > It does not spell it as invalid length. However, for *similar* failure, > the SEV-SNP spec spells out it as INVALID_LENGTH, so, I choose macro > name as INVALID_LENGTH. You can simply define a separate one here called ...INVALID_LENGTH. Thx.
diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h index 673e6778194b..346600724b84 100644 --- a/arch/x86/include/asm/sev-common.h +++ b/arch/x86/include/asm/sev-common.h @@ -128,6 +128,9 @@ struct snp_psc_desc { struct psc_entry entries[VMGEXIT_PSC_MAX_ENTRY]; } __packed; +/* Guest message request error code */ +#define SNP_GUEST_REQ_INVALID_LEN BIT_ULL(32) + #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 76a208fd451b..a47fa0f2547e 100644 --- a/arch/x86/include/asm/sev.h +++ b/arch/x86/include/asm/sev.h @@ -81,6 +81,14 @@ extern bool handle_vc_boot_ghcb(struct pt_regs *regs); #define RMPADJUST_VMSA_PAGE_BIT BIT(16) +/* SNP Guest message request */ +struct snp_req_data { + unsigned long req_gpa; + unsigned long resp_gpa; + unsigned long data_gpa; + unsigned int data_npages; +}; + #ifdef CONFIG_AMD_MEM_ENCRYPT extern struct static_key_false sev_es_enable_key; extern void __sev_es_ist_enter(struct pt_regs *regs); @@ -148,6 +156,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 snp_abort(void); +int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned long *fw_err); #else static inline void sev_es_ist_enter(struct pt_regs *regs) { } static inline void sev_es_ist_exit(void) { } @@ -167,6 +176,11 @@ 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) +{ + return -ENOTTY; +} #endif #endif diff --git a/arch/x86/include/uapi/asm/svm.h b/arch/x86/include/uapi/asm/svm.h index 8b4c57baec52..5b8bc2b65a5e 100644 --- a/arch/x86/include/uapi/asm/svm.h +++ b/arch/x86/include/uapi/asm/svm.h @@ -109,6 +109,8 @@ #define SVM_VMGEXIT_SET_AP_JUMP_TABLE 0 #define SVM_VMGEXIT_GET_AP_JUMP_TABLE 1 #define SVM_VMGEXIT_PSC 0x80000010 +#define SVM_VMGEXIT_GUEST_REQUEST 0x80000011 +#define SVM_VMGEXIT_EXT_GUEST_REQUEST 0x80000012 #define SVM_VMGEXIT_AP_CREATION 0x80000013 #define SVM_VMGEXIT_AP_CREATE_ON_INIT 0 #define SVM_VMGEXIT_AP_CREATE 1 @@ -225,6 +227,8 @@ { SVM_VMGEXIT_AP_HLT_LOOP, "vmgexit_ap_hlt_loop" }, \ { SVM_VMGEXIT_AP_JUMP_TABLE, "vmgexit_ap_jump_table" }, \ { SVM_VMGEXIT_PSC, "vmgexit_page_state_change" }, \ + { SVM_VMGEXIT_GUEST_REQUEST, "vmgexit_guest_request" }, \ + { SVM_VMGEXIT_EXT_GUEST_REQUEST, "vmgexit_ext_guest_request" }, \ { SVM_VMGEXIT_AP_CREATION, "vmgexit_ap_creation" }, \ { SVM_VMGEXIT_HV_FEATURES, "vmgexit_hypervisor_feature" }, \ { SVM_EXIT_ERR, "invalid_guest_state" } diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c index 70e18b98bb68..289f93e1ab80 100644 --- a/arch/x86/kernel/sev.c +++ b/arch/x86/kernel/sev.c @@ -2051,3 +2051,54 @@ static int __init snp_cpuid_check_status(void) } arch_initcall(snp_cpuid_check_status); + +int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned long *fw_err) +{ + struct ghcb_state state; + unsigned long flags; + struct ghcb *ghcb; + int ret; + + if (!cc_platform_has(CC_ATTR_SEV_SNP)) + return -ENODEV; + + /* __sev_get_ghcb() need to run with IRQs disabled because it using per-cpu GHCB */ + local_irq_save(flags); + + ghcb = __sev_get_ghcb(&state); + if (!ghcb) { + ret = -EIO; + goto e_restore_irq; + } + + vc_ghcb_invalidate(ghcb); + + if (exit_code == SVM_VMGEXIT_EXT_GUEST_REQUEST) { + ghcb_set_rax(ghcb, input->data_gpa); + ghcb_set_rbx(ghcb, input->data_npages); + } + + ret = sev_es_ghcb_hv_call(ghcb, true, NULL, exit_code, input->req_gpa, input->resp_gpa); + if (ret) + goto e_put; + + if (ghcb->save.sw_exit_info_2) { + /* Number of expected pages are returned in RBX */ + if (exit_code == SVM_VMGEXIT_EXT_GUEST_REQUEST && + ghcb->save.sw_exit_info_2 == SNP_GUEST_REQ_INVALID_LEN) + input->data_npages = ghcb_get_rbx(ghcb); + + if (fw_err) + *fw_err = ghcb->save.sw_exit_info_2; + + ret = -EIO; + } + +e_put: + __sev_put_ghcb(&state); +e_restore_irq: + local_irq_restore(flags); + + return ret; +} +EXPORT_SYMBOL_GPL(snp_issue_guest_request);
Version 2 of GHCB specification provides SNP_GUEST_REQUEST and SNP_EXT_GUEST_REQUEST NAE that can be used by the SNP guest to communicate with the PSP. While at it, add a snp_issue_guest_request() helper that can be used by driver or other subsystem to issue the request to PSP. See SEV-SNP and GHCB spec for more details. Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- arch/x86/include/asm/sev-common.h | 3 ++ arch/x86/include/asm/sev.h | 14 +++++++++ arch/x86/include/uapi/asm/svm.h | 4 +++ arch/x86/kernel/sev.c | 51 +++++++++++++++++++++++++++++++ 4 files changed, 72 insertions(+)