| Message ID | bb10f0a4c5eb13a5338f77ef34f08f1190d4ae30.1655761627.git.ashish.kalra@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) | expand |
* Ashish Kalra (Ashish.Kalra@amd.com) wrote: > From: Brijesh Singh <brijesh.singh@amd.com> > > When SEV-SNP is enabled, the guest private pages are added in the RMP > table; while adding the pages, the rmp_make_private() unmaps the pages > from the direct map. If KSM attempts to access those unmapped pages then > it will trigger #PF (page-not-present). > > Encrypted guest pages cannot be shared between the process, so an > userspace should not mark the region mergeable but to be safe, mark the > process vma unmerable before adding the pages in the RMP table. ^ Typo 'unmergable' (also in title) > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > arch/x86/kvm/svm/sev.c | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index b5f0707d7ed6..a9461d352eda 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -19,11 +19,13 @@ > #include <linux/trace_events.h> > #include <linux/hugetlb.h> > #include <linux/sev.h> > +#include <linux/ksm.h> > > #include <asm/pkru.h> > #include <asm/trapnr.h> > #include <asm/fpu/xcr.h> > #include <asm/sev.h> > +#include <asm/mman.h> > > #include "x86.h" > #include "svm.h" > @@ -1965,6 +1967,30 @@ static bool is_hva_registered(struct kvm *kvm, hva_t hva, size_t len) > return false; > } > > +static int snp_mark_unmergable(struct kvm *kvm, u64 start, u64 size) > +{ > + struct vm_area_struct *vma; > + u64 end = start + size; > + int ret; > + > + do { > + vma = find_vma_intersection(kvm->mm, start, end); > + if (!vma) { > + ret = -EINVAL; > + break; > + } > + > + ret = ksm_madvise(vma, vma->vm_start, vma->vm_end, > + MADV_UNMERGEABLE, &vma->vm_flags); > + if (ret) > + break; > + > + start = vma->vm_end; > + } while (end > vma->vm_end); > + > + return ret; > +} > + > static int snp_launch_update(struct kvm *kvm, struct kvm_sev_cmd *argp) > { > struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; > @@ -1989,6 +2015,12 @@ static int snp_launch_update(struct kvm *kvm, struct kvm_sev_cmd *argp) > if (!is_hva_registered(kvm, params.uaddr, params.len)) > return -EINVAL; > > + mmap_write_lock(kvm->mm); > + ret = snp_mark_unmergable(kvm, params.uaddr, params.len); > + mmap_write_unlock(kvm->mm); > + if (ret) > + return -EFAULT; > + > /* > * The userspace memory is already locked so technically we don't > * need to lock it again. Later part of the function needs to know > -- > 2.25.1 >
On 6/21/22 01:08, Ashish Kalra wrote: > From: Brijesh Singh <brijesh.singh@amd.com> > > When SEV-SNP is enabled, the guest private pages are added in the RMP > table; while adding the pages, the rmp_make_private() unmaps the pages > from the direct map. If KSM attempts to access those unmapped pages then > it will trigger #PF (page-not-present). > > Encrypted guest pages cannot be shared between the process, so an > userspace should not mark the region mergeable but to be safe, mark the > process vma unmerable before adding the pages in the RMP table. > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> Note this doesn't really mark the vma unmergeable, rather it unmarks it as mergeable, and unmerges any already merged pages. Which seems like a good idea. Is snp_launch_update() the only place that needs it or can private pages be added elsewhere too? However, AFAICS nothing stops userspace to do another madvise(MADV_MERGEABLE) afterwards, so we should make somehow sure that ksm will still be prevented, as we should protect the kernel even from a buggy userspace. So either we stop it with a flag at vma level (see ksm_madvise() for which flags currently stop it), or page level - currently only PageAnon() pages are handled. The vma level is probably easier/cheaper. It's also possible that this will solve itself with the switch to UPM as those vma's or pages might be incompatible with ksm naturally (didn't check closely), and then this patch can be just dropped. But we should double-check.
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index b5f0707d7ed6..a9461d352eda 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -19,11 +19,13 @@ #include <linux/trace_events.h> #include <linux/hugetlb.h> #include <linux/sev.h> +#include <linux/ksm.h> #include <asm/pkru.h> #include <asm/trapnr.h> #include <asm/fpu/xcr.h> #include <asm/sev.h> +#include <asm/mman.h> #include "x86.h" #include "svm.h" @@ -1965,6 +1967,30 @@ static bool is_hva_registered(struct kvm *kvm, hva_t hva, size_t len) return false; } +static int snp_mark_unmergable(struct kvm *kvm, u64 start, u64 size) +{ + struct vm_area_struct *vma; + u64 end = start + size; + int ret; + + do { + vma = find_vma_intersection(kvm->mm, start, end); + if (!vma) { + ret = -EINVAL; + break; + } + + ret = ksm_madvise(vma, vma->vm_start, vma->vm_end, + MADV_UNMERGEABLE, &vma->vm_flags); + if (ret) + break; + + start = vma->vm_end; + } while (end > vma->vm_end); + + return ret; +} + static int snp_launch_update(struct kvm *kvm, struct kvm_sev_cmd *argp) { struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; @@ -1989,6 +2015,12 @@ static int snp_launch_update(struct kvm *kvm, struct kvm_sev_cmd *argp) if (!is_hva_registered(kvm, params.uaddr, params.len)) return -EINVAL; + mmap_write_lock(kvm->mm); + ret = snp_mark_unmergable(kvm, params.uaddr, params.len); + mmap_write_unlock(kvm->mm); + if (ret) + return -EFAULT; + /* * The userspace memory is already locked so technically we don't * need to lock it again. Later part of the function needs to know