| Message ID | 20230220183847.59159-16-michael.roth@amd.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support | expand |
On Mon, Feb 20, 2023 at 12:38:06PM -0600, Michael Roth wrote: > From: Brijesh Singh <brijesh.singh@amd.com> > > The integrity guarantee of SEV-SNP is enforced through the RMP table. > The RMP is used with standard x86 and IOMMU page tables to enforce > memory restrictions and page access rights. The RMP check is enforced as > soon as SEV-SNP is enabled globally in the system. When hardware > encounters an RMP-check failure, it raises a page-fault exception. > > The rmp_make_private() and rmp_make_shared() helpers are used to add > or remove the pages from the RMP table. Improve the rmp_make_private() > to invalidate state so that pages cannot be used in the direct-map after > they are added the RMP table, and restored to their default valid > permission after the pages are removed from the RMP table. > > Co-developed-by: Ashish Kalra <ashish.kalra@amd.com> > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > Signed-off-by: Michael Roth <michael.roth@amd.com> > --- > arch/x86/kernel/sev.c | 57 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 57 insertions(+) > > diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c > index a49f30c10dc1..3e5ff5934e83 100644 > --- a/arch/x86/kernel/sev.c > +++ b/arch/x86/kernel/sev.c > @@ -2595,6 +2595,37 @@ int psmash(u64 pfn) > } > EXPORT_SYMBOL_GPL(psmash); > > +static int restore_direct_map(u64 pfn, int npages) > +{ > + int i, ret = 0; > + > + for (i = 0; i < npages; i++) { > + ret = set_direct_map_default_noflush(pfn_to_page(pfn + i)); > + if (ret) > + goto cleanup; > + } > + > +cleanup: > + WARN(ret > 0, "Failed to restore direct map for pfn 0x%llx\n", pfn + i); > + return ret; > +} > + > +static int invalidate_direct_map(u64 pfn, int npages) > +{ > + int i, ret = 0; > + > + for (i = 0; i < npages; i++) { > + ret = set_direct_map_invalid_noflush(pfn_to_page(pfn + i)); > + if (ret) > + goto cleanup; > + } > + > +cleanup: > + WARN(ret > 0, "Failed to invalidate direct map for pfn 0x%llx\n", pfn + i); > + restore_direct_map(pfn, i); This immediately restores the direct map after invalidating it. It probably needs to put behind if(ret). Regards, Tom > + return ret; > +} > + > static int rmpupdate(u64 pfn, struct rmp_state *val) > { > int max_attempts = 4 * num_present_cpus(); > @@ -2605,6 +2636,21 @@ static int rmpupdate(u64 pfn, struct rmp_state *val) > if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP)) > return -ENXIO; > > + level = RMP_TO_X86_PG_LEVEL(val->pagesize); > + npages = page_level_size(level) / PAGE_SIZE; > + > + /* > + * If page is getting assigned in the RMP table then unmap it from the > + * direct map. > + */ > + if (val->assigned) { > + if (invalidate_direct_map(pfn, npages)) { > + pr_err("Failed to unmap %d pages at pfn 0x%llx from the direct_map\n", > + npages, pfn); > + return -EFAULT; > + } > + } > + > do { > /* Binutils version 2.36 supports the RMPUPDATE mnemonic. */ > asm volatile(".byte 0xF2, 0x0F, 0x01, 0xFE" > @@ -2630,6 +2676,17 @@ static int rmpupdate(u64 pfn, struct rmp_state *val) > attempts, val->asid, ret, pfn, npages); > } > > + /* > + * Restore the direct map after the page is removed from the RMP table. > + */ > + if (!val->assigned) { > + if (restore_direct_map(pfn, npages)) { > + pr_err("Failed to map %d pages at pfn 0x%llx into the direct_map\n", > + npages, pfn); > + return -EFAULT; > + } > + } > + > return 0; > } > > -- > 2.25.1 >
On 2/20/23 10:38, Michael Roth wrote: > From: Brijesh Singh <brijesh.singh@amd.com> > > The integrity guarantee of SEV-SNP is enforced through the RMP table. > The RMP is used with standard x86 and IOMMU page tables to enforce > memory restrictions and page access rights. The RMP check is enforced as > soon as SEV-SNP is enabled globally in the system. When hardware > encounters an RMP-check failure, it raises a page-fault exception. > > The rmp_make_private() and rmp_make_shared() helpers are used to add > or remove the pages from the RMP table. Improve the rmp_make_private() > to invalidate state so that pages cannot be used in the direct-map after > they are added the RMP table, and restored to their default valid > permission after the pages are removed from the RMP table. This is a purely "what" changelog. It doesn't explain the "why" at all. Could you please elaborate on why this unmapping operation is necessary?
On Wed, Mar 01, 2023 at 08:15:46AM -0800, Dave Hansen wrote: > On 2/20/23 10:38, Michael Roth wrote: > > From: Brijesh Singh <brijesh.singh@amd.com> > > > > The integrity guarantee of SEV-SNP is enforced through the RMP table. > > The RMP is used with standard x86 and IOMMU page tables to enforce > > memory restrictions and page access rights. The RMP check is enforced as > > soon as SEV-SNP is enabled globally in the system. When hardware > > encounters an RMP-check failure, it raises a page-fault exception. > > > > The rmp_make_private() and rmp_make_shared() helpers are used to add > > or remove the pages from the RMP table. Improve the rmp_make_private() > > to invalidate state so that pages cannot be used in the direct-map after > > they are added the RMP table, and restored to their default valid > > permission after the pages are removed from the RMP table. > > This is a purely "what" changelog. It doesn't explain the "why" at all. > > Could you please elaborate on why this unmapping operation is necessary? > Here's my attempt at an updated changelog that explains why this handling is needed: With SEV-SNP, if a host attempts to write to a page that is owned by a guest (according to the SEV-SNP RMP table), the host will get a #PF with a bit set to indicate that an RMP violation occurred. This shouldn't normally occur, since guest-owned pages are encrypted, so any attempt to write to them would just result in garbage being written. However, if a host writes to a page via a 2M/1G mapping in the host process' page table, the above #PF condition will trigger if *any* 4K sub-pages mapped by that PTE are guest-owned, even if the write is only to 4K pages that are owned by the host. This becomes an issue with the kernel directmap, which provides a static/linear mapping of all kernel memory and defaults to using 2M pages. In cases where a page from one of these 2M ranges gets assigned to a guest, the kernel can inadvertantly trigger #PF by writing to some other page in that 2M region via the virtual addresses provided by the directmap. Address this by removing directmap mappings for these PFNs before marking them as guest-owned in the RMP table, which will result in the original 2M mapping being split and ensure that guest-owned pages can't be written to via the kernel directmap. -Mike
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c index a49f30c10dc1..3e5ff5934e83 100644 --- a/arch/x86/kernel/sev.c +++ b/arch/x86/kernel/sev.c @@ -2595,6 +2595,37 @@ int psmash(u64 pfn) } EXPORT_SYMBOL_GPL(psmash); +static int restore_direct_map(u64 pfn, int npages) +{ + int i, ret = 0; + + for (i = 0; i < npages; i++) { + ret = set_direct_map_default_noflush(pfn_to_page(pfn + i)); + if (ret) + goto cleanup; + } + +cleanup: + WARN(ret > 0, "Failed to restore direct map for pfn 0x%llx\n", pfn + i); + return ret; +} + +static int invalidate_direct_map(u64 pfn, int npages) +{ + int i, ret = 0; + + for (i = 0; i < npages; i++) { + ret = set_direct_map_invalid_noflush(pfn_to_page(pfn + i)); + if (ret) + goto cleanup; + } + +cleanup: + WARN(ret > 0, "Failed to invalidate direct map for pfn 0x%llx\n", pfn + i); + restore_direct_map(pfn, i); + return ret; +} + static int rmpupdate(u64 pfn, struct rmp_state *val) { int max_attempts = 4 * num_present_cpus(); @@ -2605,6 +2636,21 @@ static int rmpupdate(u64 pfn, struct rmp_state *val) if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP)) return -ENXIO; + level = RMP_TO_X86_PG_LEVEL(val->pagesize); + npages = page_level_size(level) / PAGE_SIZE; + + /* + * If page is getting assigned in the RMP table then unmap it from the + * direct map. + */ + if (val->assigned) { + if (invalidate_direct_map(pfn, npages)) { + pr_err("Failed to unmap %d pages at pfn 0x%llx from the direct_map\n", + npages, pfn); + return -EFAULT; + } + } + do { /* Binutils version 2.36 supports the RMPUPDATE mnemonic. */ asm volatile(".byte 0xF2, 0x0F, 0x01, 0xFE" @@ -2630,6 +2676,17 @@ static int rmpupdate(u64 pfn, struct rmp_state *val) attempts, val->asid, ret, pfn, npages); } + /* + * Restore the direct map after the page is removed from the RMP table. + */ + if (!val->assigned) { + if (restore_direct_map(pfn, npages)) { + pr_err("Failed to map %d pages at pfn 0x%llx into the direct_map\n", + npages, pfn); + return -EFAULT; + } + } + return 0; }