| Message ID | 20230218211433.26859-25-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Shadow stacks for userspace | expand |
On 18.02.23 22:14, Rick Edgecombe wrote: > The x86 Control-flow Enforcement Technology (CET) feature includes a new > type of memory called shadow stack. This shadow stack memory has some > unusual properties, which requires some core mm changes to function > properly. > > Shadow stack memory is writable only in very specific, controlled ways. > However, since it is writable, the kernel treats it as such. As a result > there remain many ways for userspace to trigger the kernel to write to > shadow stack's via get_user_pages(, FOLL_WRITE) operations. To make this a > little less exposed, block writable GUPs for shadow stack VMAs. > > Still allow FOLL_FORCE to write through shadow stack protections, as it > does for read-only protections. > > Reviewed-by: Kees Cook <keescook@chromium.org> > Tested-by: Pengfei Xu <pengfei.xu@intel.com> > Tested-by: John Allen <john.allen@amd.com> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > > --- > v3: > - Add comment in __pte_access_permitted() (Dave) > - Remove unneeded shadow stack specific check in > __pte_access_permitted() (Jann) > --- > arch/x86/include/asm/pgtable.h | 5 +++++ > mm/gup.c | 2 +- > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h > index 6b7106457bfb..20d0df494269 100644 > --- a/arch/x86/include/asm/pgtable.h > +++ b/arch/x86/include/asm/pgtable.h > @@ -1641,6 +1641,11 @@ static inline bool __pte_access_permitted(unsigned long pteval, bool write) > { > unsigned long need_pte_bits = _PAGE_PRESENT|_PAGE_USER; > > + /* > + * Write=0,Dirty=1 PTEs are shadow stack, which the kernel > + * shouldn't generally allow access to, but since they > + * are already Write=0, the below logic covers both cases. > + */ > if (write) > need_pte_bits |= _PAGE_RW; So, GUP fast will always fail when writing ... > > diff --git a/mm/gup.c b/mm/gup.c > index f45a3a5be53a..bfd33d9edb89 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -982,7 +982,7 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags) > return -EFAULT; > > if (write) { > - if (!(vm_flags & VM_WRITE)) { > + if (!(vm_flags & VM_WRITE) || (vm_flags & VM_SHADOW_STACK)) { > if (!(gup_flags & FOLL_FORCE)) > return -EFAULT; > /* hugetlb does not support FOLL_FORCE|FOLL_WRITE. */ and ordinary GUP without FOLL_FORCE. Acked-by: David Hildenbrand <david@redhat.com>
On Tue, 2023-02-21 at 09:42 +0100, David Hildenbrand wrote: > and ordinary GUP without FOLL_FORCE. > > Acked-by: David Hildenbrand <david@redhat.com> Thanks! And for the other acks.
diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h index 6b7106457bfb..20d0df494269 100644 --- a/arch/x86/include/asm/pgtable.h +++ b/arch/x86/include/asm/pgtable.h @@ -1641,6 +1641,11 @@ static inline bool __pte_access_permitted(unsigned long pteval, bool write) { unsigned long need_pte_bits = _PAGE_PRESENT|_PAGE_USER; + /* + * Write=0,Dirty=1 PTEs are shadow stack, which the kernel + * shouldn't generally allow access to, but since they + * are already Write=0, the below logic covers both cases. + */ if (write) need_pte_bits |= _PAGE_RW; diff --git a/mm/gup.c b/mm/gup.c index f45a3a5be53a..bfd33d9edb89 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -982,7 +982,7 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags) return -EFAULT; if (write) { - if (!(vm_flags & VM_WRITE)) { + if (!(vm_flags & VM_WRITE) || (vm_flags & VM_SHADOW_STACK)) { if (!(gup_flags & FOLL_FORCE)) return -EFAULT; /* hugetlb does not support FOLL_FORCE|FOLL_WRITE. */