| Message ID | 20211008180453.462291-20-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Guest Support | expand |
On Fri, Oct 08, 2021 at 01:04:30PM -0500, Brijesh Singh wrote: > +static int vmgexit_psc(struct snp_psc_desc *desc) > +{ > + int cur_entry, end_entry, ret; > + struct snp_psc_desc *data; > + struct ghcb_state state; > + struct ghcb *ghcb; > + struct psc_hdr *hdr; > + unsigned long flags; int cur_entry, end_entry, ret; struct snp_psc_desc *data; struct ghcb_state state; struct psc_hdr *hdr; unsigned long flags; struct ghcb *ghcb; that's properly sorted. > + > + local_irq_save(flags); What is that protecting against? Comment about it? Aha, __sev_get_ghcb() needs to run with IRQs disabled because it is using the per-CPU GHCB. > + > + ghcb = __sev_get_ghcb(&state); > + if (unlikely(!ghcb)) > + panic("SEV-SNP: Failed to get GHCB\n"); > + > + /* Copy the input desc into GHCB shared buffer */ > + data = (struct snp_psc_desc *)ghcb->shared_buffer; > + memcpy(ghcb->shared_buffer, desc, sizeof(*desc)); That shared buffer has a size - check it vs the size of the desc thing. > + > + hdr = &data->hdr; Why do you need this and why can't you use data->hdr simply? /me continues reading and realizes why Oh no, this is tricky. The HV call will modify what @data points to and thus @hdr will point to new contents. Only then your backwards processing check below makes sense. So then you *absoulutely* want to use data->hdr everywhere and then also write why in the comment above the check that data gets updated by the HV call. > + cur_entry = hdr->cur_entry; > + end_entry = hdr->end_entry; > + > + /* > + * As per the GHCB specification, the hypervisor can resume the guest > + * before processing all the entries. Checks whether all the entries Check > + * are processed. If not, then keep retrying. > + * > + * The stragtegy here is to wait for the hypervisor to change the page > + * state in the RMP table before guest access the memory pages. If the accesses > + * page state was not successful, then later memory access will result "If the page state *change* was not ..." > + * in the crash. "in a crash." > + */ > + while (hdr->cur_entry <= hdr->end_entry) { > + ghcb_set_sw_scratch(ghcb, (u64)__pa(data)); > + > + ret = sev_es_ghcb_hv_call(ghcb, NULL, SVM_VMGEXIT_PSC, 0, 0); This should be ret = sev_es_ghcb_hv_call(ghcb, true, NULL, SVM_VMGEXIT_PSC, 0, 0); as we changed it in the meantime to accomodate HyperV isolation VMs. > + > + /* > + * Page State Change VMGEXIT can pass error code through > + * exit_info_2. > + */ > + if (WARN(ret || ghcb->save.sw_exit_info_2, > + "SEV-SNP: PSC failed ret=%d exit_info_2=%llx\n", > + ret, ghcb->save.sw_exit_info_2)) { > + ret = 1; That ret = 1 goes unused with that "return 0" at the end. It should be "return ret" at the end.. Ditto for the others. Audit all your exit paths in this function. > + goto out; > + } > + > + /* > + * Sanity check that entry processing is not going backward. > + * This will happen only if hypervisor is tricking us. > + */ > + if (WARN(hdr->end_entry > end_entry || cur_entry > hdr->cur_entry, > +"SEV-SNP: PSC processing going backward, end_entry %d (got %d) cur_entry %d (got %d)\n", > + end_entry, hdr->end_entry, cur_entry, hdr->cur_entry)) { > + ret = 1; > + goto out; > + } > + > + /* Verify that reserved bit is not set */ > + if (WARN(hdr->reserved, "Reserved bit is set in the PSC header\n")) { Shouldn't that thing happen first after the HV call? > + ret = 1; > + goto out; > + } > + } > + > +out: > + __sev_put_ghcb(&state); > + local_irq_restore(flags); > + > + return 0; > +} > + > +static void __set_page_state(struct snp_psc_desc *data, unsigned long vaddr, > + unsigned long vaddr_end, int op) > +{ > + struct psc_hdr *hdr; > + struct psc_entry *e; > + unsigned long pfn; > + int i; > + > + hdr = &data->hdr; > + e = data->entries; > + > + memset(data, 0, sizeof(*data)); > + i = 0; > + > + while (vaddr < vaddr_end) { > + if (is_vmalloc_addr((void *)vaddr)) > + pfn = vmalloc_to_pfn((void *)vaddr); > + else > + pfn = __pa(vaddr) >> PAGE_SHIFT; > + > + e->gfn = pfn; > + e->operation = op; > + hdr->end_entry = i; > + > + /* > + * The GHCB specification provides the flexibility to > + * use either 4K or 2MB page size in the RMP table. > + * The current SNP support does not keep track of the > + * page size used in the RMP table. To avoid the > + * overlap request, "avoid overlap request"? No clue what that means. In general, that comment is talking about something in the future and is more confusing than explaining stuff. > use the 4K page size in the RMP > + * table. > + */ > + e->pagesize = RMP_PG_SIZE_4K; > + > + vaddr = vaddr + PAGE_SIZE; > + e++; > + i++; > + } > + > + if (vmgexit_psc(data)) > + sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_PSC); > +} > + > +static void set_page_state(unsigned long vaddr, unsigned int npages, int op) Yeah, so this should be named set_pages_state - notice the plural "pages" because it works on multiple pages, @npages exactly. > +{ > + unsigned long vaddr_end, next_vaddr; > + struct snp_psc_desc *desc; > + > + vaddr = vaddr & PAGE_MASK; > + vaddr_end = vaddr + (npages << PAGE_SHIFT); Take those two... > + > + desc = kmalloc(sizeof(*desc), GFP_KERNEL_ACCOUNT); > + if (!desc) > + panic("SEV-SNP: failed to allocate memory for PSC descriptor\n"); ... and put them here. <--- > + > + while (vaddr < vaddr_end) { > + /* > + * Calculate the last vaddr that can be fit in one > + * struct snp_psc_desc. > + */ > + next_vaddr = min_t(unsigned long, vaddr_end, > + (VMGEXIT_PSC_MAX_ENTRY * PAGE_SIZE) + vaddr); > + > + __set_page_state(desc, vaddr, next_vaddr, op); > + > + vaddr = next_vaddr; > + } > + > + kfree(desc); > +} > + > +void snp_set_memory_shared(unsigned long vaddr, unsigned int npages) > +{ > + if (!cc_platform_has(CC_ATTR_SEV_SNP)) > + return; > + > + pvalidate_pages(vaddr, npages, 0); > + > + set_page_state(vaddr, npages, SNP_PAGE_STATE_SHARED); > +} > + > +void snp_set_memory_private(unsigned long vaddr, unsigned int npages) > +{ > + if (!cc_platform_has(CC_ATTR_SEV_SNP)) > + return; > + > + set_page_state(vaddr, npages, SNP_PAGE_STATE_PRIVATE); > + > + pvalidate_pages(vaddr, npages, 1); > +} > + > int sev_es_setup_ap_jump_table(struct real_mode_header *rmh) > { > u16 startup_cs, startup_ip; > diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c > index 527957586f3c..ffe51944606a 100644 > --- a/arch/x86/mm/pat/set_memory.c > +++ b/arch/x86/mm/pat/set_memory.c > @@ -30,6 +30,7 @@ > #include <asm/proto.h> > #include <asm/memtype.h> > #include <asm/set_memory.h> > +#include <asm/sev.h> > > #include "../mm_internal.h" > > @@ -2010,8 +2011,22 @@ static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc) > */ > cpa_flush(&cpa, !this_cpu_has(X86_FEATURE_SME_COHERENT)); > > + /* > + * To maintain the security gurantees of SEV-SNP guest invalidate the memory "guarantees" Your spellchecker broke again. > + * before clearing the encryption attribute. > + */ > + if (!enc) > + snp_set_memory_shared(addr, numpages); > + > ret = __change_page_attr_set_clr(&cpa, 1); > > + /* > + * Now that memory is mapped encrypted in the page table, validate it > + * so that is consistent with the above page state. > + */ > + if (!ret && enc) > + snp_set_memory_private(addr, numpages); > + > /* > * After changing the encryption attribute, we need to flush TLBs again > * in case any speculative TLB caching occurred (but no need to flush > -- > 2.25.1 >
On 11/9/21 1:34 PM, Borislav Petkov wrote: > On Fri, Oct 08, 2021 at 01:04:30PM -0500, Brijesh Singh wrote: >> +static int vmgexit_psc(struct snp_psc_desc *desc) >> +{ >> + int cur_entry, end_entry, ret; >> + struct snp_psc_desc *data; >> + struct ghcb_state state; >> + struct ghcb *ghcb; >> + struct psc_hdr *hdr; >> + unsigned long flags; > > int cur_entry, end_entry, ret; > struct snp_psc_desc *data; > struct ghcb_state state; > struct psc_hdr *hdr; > unsigned long flags; > struct ghcb *ghcb; > > that's properly sorted. > Noted. >> + >> + local_irq_save(flags); > > What is that protecting against? Comment about it? > > Aha, __sev_get_ghcb() needs to run with IRQs disabled because it is > using the per-CPU GHCB. > I will add a comment to clarify it. >> + >> + ghcb = __sev_get_ghcb(&state); >> + if (unlikely(!ghcb)) >> + panic("SEV-SNP: Failed to get GHCB\n"); >> + >> + /* Copy the input desc into GHCB shared buffer */ >> + data = (struct snp_psc_desc *)ghcb->shared_buffer; >> + memcpy(ghcb->shared_buffer, desc, sizeof(*desc)); > > That shared buffer has a size - check it vs the size of the desc thing. > I am assuming you mean add some compile time check to ensure that desc will fit in the shared buffer ? ... >> + if (WARN(ret || ghcb->save.sw_exit_info_2, >> + "SEV-SNP: PSC failed ret=%d exit_info_2=%llx\n", >> + ret, ghcb->save.sw_exit_info_2)) { >> + ret = 1; > > That ret = 1 goes unused with that "return 0" at the end. It should be > "return ret" at the end.. Ditto for the others. Audit all your exit > paths in this function. Noted. >> + >> + /* Verify that reserved bit is not set */ >> + if (WARN(hdr->reserved, "Reserved bit is set in the PSC header\n")) { > > Shouldn't that thing happen first after the HV call? > I am okay to move this check before the going backward check. >> + >> + /* >> + * The GHCB specification provides the flexibility to >> + * use either 4K or 2MB page size in the RMP table. >> + * The current SNP support does not keep track of the >> + * page size used in the RMP table. To avoid the >> + * overlap request, > > "avoid overlap request"? > > No clue what that means. In general, that comment is talking about > something in the future and is more confusing than explaining stuff. > I can drop the overlap comment to avoid the confusion, as you pointed it more of the future thing. Basically overlap is the below condition set_memory_private(gfn=0, page_size=2m) set_memory_private(gfn=10, page_size=4k) The RMPUPDATE instruction will detect overlap on the second call and return an error to the guest. After we add the support to track the page validation state (either in bitmap or page flag), the second call will not be issued and thus avoid an overlap errors. For now, we use the page_size=4k for all the page state changes from the kernel. >> use the 4K page size in the RMP >> + * table. >> + */ >> + e->pagesize = RMP_PG_SIZE_4K; >> + >> + vaddr = vaddr + PAGE_SIZE; >> + e++; >> + i++; >> + } >> + >> + if (vmgexit_psc(data)) >> + sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_PSC); >> +} >> + >> +static void set_page_state(unsigned long vaddr, unsigned int npages, int op) > > Yeah, so this should be named > > set_pages_state - notice the plural "pages" > > because it works on multiple pages, @npages exactly. > Ah, I thought I had it pages but maybe it got renamed sometime back in the series. >> +{ >> + unsigned long vaddr_end, next_vaddr; >> + struct snp_psc_desc *desc; >> + >> + vaddr = vaddr & PAGE_MASK; >> + vaddr_end = vaddr + (npages << PAGE_SHIFT); > > Take those two... > >> + >> + desc = kmalloc(sizeof(*desc), GFP_KERNEL_ACCOUNT); >> + if (!desc) >> + panic("SEV-SNP: failed to allocate memory for PSC descriptor\n"); > > > ... and put them here. > Noted. thanks
On Wed, Nov 10, 2021 at 08:21:21AM -0600, Brijesh Singh wrote: > I am assuming you mean add some compile time check to ensure that desc will > fit in the shared buffer ? No: struct ghcb { ... u8 shared_buffer[2032]; so that memcpy needs to do: memcpy(ghcb->shared_buffer, desc, min_t(int, 2032, sizeof(*desc))); with that 2032 behind a proper define, ofc. > I can drop the overlap comment to avoid the confusion, as you pointed it > more of the future thing. Basically overlap is the below condition > > set_memory_private(gfn=0, page_size=2m) > set_memory_private(gfn=10, page_size=4k) > > The RMPUPDATE instruction will detect overlap on the second call and return > an error to the guest. After we add the support to track the page validation > state (either in bitmap or page flag), the second call will not be issued > and thus avoid an overlap errors. For now, we use the page_size=4k for all > the page state changes from the kernel. Yah, sounds like the comment is not needed now. You could put this in the commit message, though. Thx.
On 11/10/21 12:43 PM, Borislav Petkov wrote: > On Wed, Nov 10, 2021 at 08:21:21AM -0600, Brijesh Singh wrote: >> I am assuming you mean add some compile time check to ensure that desc will >> fit in the shared buffer ? > > No: > > struct ghcb { > > ... > > u8 shared_buffer[2032]; > > so that memcpy needs to do: > > memcpy(ghcb->shared_buffer, desc, min_t(int, 2032, sizeof(*desc))); > > with that 2032 behind a proper define, ofc. 2032 => sizeof(ghcb->shared_buffer) ? The idea is that a full snp_psc_desc structure is meant to fit completely in the shared_buffer area. So if there are no compile time checks, then the code on the HV side will need to ensure that the input doesn't cause the HV to access the structure outside of the shared_buffer area - which, IIRC, it does (think protect against a malicious guest), so the min_t() on the memcpy should be safe on the guest side. But given the snp_psc_desc is sized/meant to fit completely in the shared_buffer, a compile time check would be a good idea, too, right? Thanks, Tom > >> I can drop the overlap comment to avoid the confusion, as you pointed it >> more of the future thing. Basically overlap is the below condition >> >> set_memory_private(gfn=0, page_size=2m) >> set_memory_private(gfn=10, page_size=4k) >> >> The RMPUPDATE instruction will detect overlap on the second call and return >> an error to the guest. After we add the support to track the page validation >> state (either in bitmap or page flag), the second call will not be issued >> and thus avoid an overlap errors. For now, we use the page_size=4k for all >> the page state changes from the kernel. > > Yah, sounds like the comment is not needed now. You could put this in > the commit message, though. > > Thx. >
On Thu, Nov 11, 2021 at 08:49:49AM -0600, Tom Lendacky wrote: > 2032 => sizeof(ghcb->shared_buffer) ? Or that. > The idea is that a full snp_psc_desc structure is meant to fit completely in > the shared_buffer area. So if there are no compile time checks, then the > code on the HV side will need to ensure that the input doesn't cause the HV > to access the structure outside of the shared_buffer area - which, IIRC, it > does (think protect against a malicious guest), so the min_t() on the memcpy > should be safe on the guest side. > > But given the snp_psc_desc is sized/meant to fit completely in the > shared_buffer, a compile time check would be a good idea, too, right? If the desc thing is meant to fit, then a compile-time check is also a good way to express that intention. So yeah. Thx.
diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h index b82fff9d607b..c2c5d60f0da0 100644 --- a/arch/x86/include/asm/sev-common.h +++ b/arch/x86/include/asm/sev-common.h @@ -105,6 +105,28 @@ enum psc_op { #define GHCB_HV_FT_SNP BIT_ULL(0) +/* SNP Page State Change NAE event */ +#define VMGEXIT_PSC_MAX_ENTRY 253 + +struct psc_hdr { + u16 cur_entry; + u16 end_entry; + u32 reserved; +} __packed; + +struct psc_entry { + u64 cur_page : 12, + gfn : 40, + operation : 4, + pagesize : 1, + reserved : 7; +} __packed; + +struct snp_psc_desc { + struct psc_hdr hdr; + struct psc_entry entries[VMGEXIT_PSC_MAX_ENTRY]; +} __packed; + #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 ecd8cd8c5908..005f230d0406 100644 --- a/arch/x86/include/asm/sev.h +++ b/arch/x86/include/asm/sev.h @@ -109,6 +109,8 @@ void __init early_snp_set_memory_private(unsigned long vaddr, unsigned long padd void __init early_snp_set_memory_shared(unsigned long vaddr, unsigned long paddr, unsigned int npages); void __init snp_prep_memory(unsigned long paddr, unsigned int sz, enum psc_op op); +void snp_set_memory_shared(unsigned long vaddr, unsigned int npages); +void snp_set_memory_private(unsigned long vaddr, unsigned int npages); #else static inline void sev_es_ist_enter(struct pt_regs *regs) { } static inline void sev_es_ist_exit(void) { } @@ -121,6 +123,8 @@ early_snp_set_memory_private(unsigned long vaddr, unsigned long paddr, unsigned static inline void __init early_snp_set_memory_shared(unsigned long vaddr, unsigned long paddr, unsigned int npages) { } static inline void __init snp_prep_memory(unsigned long paddr, unsigned int sz, enum psc_op op) { } +static inline void snp_set_memory_shared(unsigned long vaddr, unsigned int npages) { } +static inline void snp_set_memory_private(unsigned long vaddr, unsigned int npages) { } #endif #endif diff --git a/arch/x86/include/uapi/asm/svm.h b/arch/x86/include/uapi/asm/svm.h index b0ad00f4c1e1..0dcdb6e0c913 100644 --- a/arch/x86/include/uapi/asm/svm.h +++ b/arch/x86/include/uapi/asm/svm.h @@ -108,6 +108,7 @@ #define SVM_VMGEXIT_AP_JUMP_TABLE 0x80000005 #define SVM_VMGEXIT_SET_AP_JUMP_TABLE 0 #define SVM_VMGEXIT_GET_AP_JUMP_TABLE 1 +#define SVM_VMGEXIT_PSC 0x80000010 #define SVM_VMGEXIT_HV_FEATURES 0x8000fffd #define SVM_VMGEXIT_UNSUPPORTED_EVENT 0x8000ffff @@ -219,6 +220,7 @@ { SVM_VMGEXIT_NMI_COMPLETE, "vmgexit_nmi_complete" }, \ { 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_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 488011479678..80fdfd83770a 100644 --- a/arch/x86/kernel/sev.c +++ b/arch/x86/kernel/sev.c @@ -655,6 +655,171 @@ void __init snp_prep_memory(unsigned long paddr, unsigned int sz, enum psc_op op WARN(1, "invalid memory op %d\n", op); } +static int vmgexit_psc(struct snp_psc_desc *desc) +{ + int cur_entry, end_entry, ret; + struct snp_psc_desc *data; + struct ghcb_state state; + struct ghcb *ghcb; + struct psc_hdr *hdr; + unsigned long flags; + + local_irq_save(flags); + + ghcb = __sev_get_ghcb(&state); + if (unlikely(!ghcb)) + panic("SEV-SNP: Failed to get GHCB\n"); + + /* Copy the input desc into GHCB shared buffer */ + data = (struct snp_psc_desc *)ghcb->shared_buffer; + memcpy(ghcb->shared_buffer, desc, sizeof(*desc)); + + hdr = &data->hdr; + cur_entry = hdr->cur_entry; + end_entry = hdr->end_entry; + + /* + * As per the GHCB specification, the hypervisor can resume the guest + * before processing all the entries. Checks whether all the entries + * are processed. If not, then keep retrying. + * + * The stragtegy here is to wait for the hypervisor to change the page + * state in the RMP table before guest access the memory pages. If the + * page state was not successful, then later memory access will result + * in the crash. + */ + while (hdr->cur_entry <= hdr->end_entry) { + ghcb_set_sw_scratch(ghcb, (u64)__pa(data)); + + ret = sev_es_ghcb_hv_call(ghcb, NULL, SVM_VMGEXIT_PSC, 0, 0); + + /* + * Page State Change VMGEXIT can pass error code through + * exit_info_2. + */ + if (WARN(ret || ghcb->save.sw_exit_info_2, + "SEV-SNP: PSC failed ret=%d exit_info_2=%llx\n", + ret, ghcb->save.sw_exit_info_2)) { + ret = 1; + goto out; + } + + /* + * Sanity check that entry processing is not going backward. + * This will happen only if hypervisor is tricking us. + */ + if (WARN(hdr->end_entry > end_entry || cur_entry > hdr->cur_entry, +"SEV-SNP: PSC processing going backward, end_entry %d (got %d) cur_entry %d (got %d)\n", + end_entry, hdr->end_entry, cur_entry, hdr->cur_entry)) { + ret = 1; + goto out; + } + + /* Verify that reserved bit is not set */ + if (WARN(hdr->reserved, "Reserved bit is set in the PSC header\n")) { + ret = 1; + goto out; + } + } + +out: + __sev_put_ghcb(&state); + local_irq_restore(flags); + + return 0; +} + +static void __set_page_state(struct snp_psc_desc *data, unsigned long vaddr, + unsigned long vaddr_end, int op) +{ + struct psc_hdr *hdr; + struct psc_entry *e; + unsigned long pfn; + int i; + + hdr = &data->hdr; + e = data->entries; + + memset(data, 0, sizeof(*data)); + i = 0; + + while (vaddr < vaddr_end) { + if (is_vmalloc_addr((void *)vaddr)) + pfn = vmalloc_to_pfn((void *)vaddr); + else + pfn = __pa(vaddr) >> PAGE_SHIFT; + + e->gfn = pfn; + e->operation = op; + hdr->end_entry = i; + + /* + * The GHCB specification provides the flexibility to + * use either 4K or 2MB page size in the RMP table. + * The current SNP support does not keep track of the + * page size used in the RMP table. To avoid the + * overlap request, use the 4K page size in the RMP + * table. + */ + e->pagesize = RMP_PG_SIZE_4K; + + vaddr = vaddr + PAGE_SIZE; + e++; + i++; + } + + if (vmgexit_psc(data)) + sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_PSC); +} + +static void set_page_state(unsigned long vaddr, unsigned int npages, int op) +{ + unsigned long vaddr_end, next_vaddr; + struct snp_psc_desc *desc; + + vaddr = vaddr & PAGE_MASK; + vaddr_end = vaddr + (npages << PAGE_SHIFT); + + desc = kmalloc(sizeof(*desc), GFP_KERNEL_ACCOUNT); + if (!desc) + panic("SEV-SNP: failed to allocate memory for PSC descriptor\n"); + + while (vaddr < vaddr_end) { + /* + * Calculate the last vaddr that can be fit in one + * struct snp_psc_desc. + */ + next_vaddr = min_t(unsigned long, vaddr_end, + (VMGEXIT_PSC_MAX_ENTRY * PAGE_SIZE) + vaddr); + + __set_page_state(desc, vaddr, next_vaddr, op); + + vaddr = next_vaddr; + } + + kfree(desc); +} + +void snp_set_memory_shared(unsigned long vaddr, unsigned int npages) +{ + if (!cc_platform_has(CC_ATTR_SEV_SNP)) + return; + + pvalidate_pages(vaddr, npages, 0); + + set_page_state(vaddr, npages, SNP_PAGE_STATE_SHARED); +} + +void snp_set_memory_private(unsigned long vaddr, unsigned int npages) +{ + if (!cc_platform_has(CC_ATTR_SEV_SNP)) + return; + + set_page_state(vaddr, npages, SNP_PAGE_STATE_PRIVATE); + + pvalidate_pages(vaddr, npages, 1); +} + int sev_es_setup_ap_jump_table(struct real_mode_header *rmh) { u16 startup_cs, startup_ip; diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c index 527957586f3c..ffe51944606a 100644 --- a/arch/x86/mm/pat/set_memory.c +++ b/arch/x86/mm/pat/set_memory.c @@ -30,6 +30,7 @@ #include <asm/proto.h> #include <asm/memtype.h> #include <asm/set_memory.h> +#include <asm/sev.h> #include "../mm_internal.h" @@ -2010,8 +2011,22 @@ static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc) */ cpa_flush(&cpa, !this_cpu_has(X86_FEATURE_SME_COHERENT)); + /* + * To maintain the security gurantees of SEV-SNP guest invalidate the memory + * before clearing the encryption attribute. + */ + if (!enc) + snp_set_memory_shared(addr, numpages); + ret = __change_page_attr_set_clr(&cpa, 1); + /* + * Now that memory is mapped encrypted in the page table, validate it + * so that is consistent with the above page state. + */ + if (!ret && enc) + snp_set_memory_private(addr, numpages); + /* * After changing the encryption attribute, we need to flush TLBs again * in case any speculative TLB caching occurred (but no need to flush
The set_memory_{encrypt,decrypt}() are used for changing the pages from decrypted (shared) to encrypted (private) and vice versa. When SEV-SNP is active, the page state transition needs to go through additional steps. If the page is transitioned from shared to private, then perform the following after the encryption attribute is set in the page table: 1. Issue the page state change VMGEXIT to add the memory region in the RMP table. 2. Validate the memory region after the RMP entry is added. To maintain the security guarantees, if the page is transitioned from private to shared, then perform the following before encryption attribute is removed from the page table: 1. Invalidate the page. 2. Issue the page state change VMGEXIT to remove the page from RMP table. To change the page state in the RMP table, use the Page State Change VMGEXIT defined in the GHCB specification. Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- arch/x86/include/asm/sev-common.h | 22 ++++ arch/x86/include/asm/sev.h | 4 + arch/x86/include/uapi/asm/svm.h | 2 + arch/x86/kernel/sev.c | 165 ++++++++++++++++++++++++++++++ arch/x86/mm/pat/set_memory.c | 15 +++ 5 files changed, 208 insertions(+)