diff mbox series

[tip:,x86/shstk] mm: Don't allow write GUPs to shadow stack memory

Message ID 168980685456.28540.8149705038742119504.tip-bot2@tip-bot2
State New
Headers
Series [tip:,x86/shstk] mm: Don't allow write GUPs to shadow stack memory |

Commit Message

tip-bot2 for Thomas Gleixner July 19, 2023, 10:47 p.m. UTC 
  The following commit has been merged into the x86/shstk branch of tip:

Commit-ID:     7c8f82db8acb9f0935703c0d96cda552bbf91ff2
Gitweb:        https://git.kernel.org/tip/7c8f82db8acb9f0935703c0d96cda552bbf91ff2
Author:        Rick Edgecombe <rick.p.edgecombe@intel.com>
AuthorDate:    Mon, 12 Jun 2023 17:10:48 -07:00
Committer:     Rick Edgecombe <rick.p.edgecombe@intel.com>
CommitterDate: Tue, 11 Jul 2023 14:12:47 -07:00

mm: Don't allow write GUPs to shadow stack memory

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.

In userspace, shadow stack memory is writable only in very specific,
controlled ways. However, since userspace can, even in the limited
ways, modify shadow stack contents, the kernel treats it as writable
memory. As a result, without additional work there would remain many
ways for userspace to trigger the kernel to write arbitrary data to
shadow stacks via get_user_pages(, FOLL_WRITE) based operations. To
help userspace protect their shadow stacks, make this a little less
exposed by blocking writable get_user_pages() operations for shadow
stack VMAs.

Still allow FOLL_FORCE to write through shadow stack protections, as it
does for read-only protections. This is required for debugging use
cases.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Reviewed-by: Borislav Petkov (AMD) <bp@alien8.de>
Reviewed-by: Kees Cook <keescook@chromium.org>
Acked-by: Mike Rapoport (IBM) <rppt@kernel.org>
Acked-by: David Hildenbrand <david@redhat.com>
Tested-by: Pengfei Xu <pengfei.xu@intel.com>
Tested-by: John Allen <john.allen@amd.com>
Tested-by: Kees Cook <keescook@chromium.org>
Link: https://lore.kernel.org/all/20230613001108.3040476-23-rick.p.edgecombe%40intel.com
---
 arch/x86/include/asm/pgtable.h | 5 +++++
 mm/gup.c                       | 6 +-----
 2 files changed, 6 insertions(+), 5 deletions(-)

Comments

Ingo Molnar July 21, 2023, 8:34 a.m. UTC | #1 
* tip-bot2 for Rick Edgecombe <tip-bot2@linutronix.de> wrote:

> The following commit has been merged into the x86/shstk branch of tip:
> 
> Commit-ID:     7c8f82db8acb9f0935703c0d96cda552bbf91ff2
> Gitweb:        https://git.kernel.org/tip/7c8f82db8acb9f0935703c0d96cda552bbf91ff2
> Author:        Rick Edgecombe <rick.p.edgecombe@intel.com>
> AuthorDate:    Mon, 12 Jun 2023 17:10:48 -07:00
> Committer:     Rick Edgecombe <rick.p.edgecombe@intel.com>
> CommitterDate: Tue, 11 Jul 2023 14:12:47 -07:00
> 
> mm: Don't allow write GUPs to shadow stack memory

JFYI, this commit was the last user of writable_file_mapping_allowed(), 
which breaks the x86-defconfig build:

  mm/gup.c:1015:13: error: ‘writable_file_mapping_allowed’ defined but not used [-Werror=unused-function]

There's also a now orphan reference to writable_file_mapping_allowed() in 
the comment of folio_fast_pin_allowed().

Thanks,

	Ingo
Edgecombe, Rick P July 21, 2023, 4:24 p.m. UTC | #2 
On Fri, 2023-07-21 at 10:34 +0200, Ingo Molnar wrote:
> 
> * tip-bot2 for Rick Edgecombe <tip-bot2@linutronix.de> wrote:
> 
> > The following commit has been merged into the x86/shstk branch of
> > tip:
> > 
> > Commit-ID:     7c8f82db8acb9f0935703c0d96cda552bbf91ff2
> > Gitweb:       
> > https://git.kernel.org/tip/7c8f82db8acb9f0935703c0d96cda552bbf91ff2
> > Author:        Rick Edgecombe <rick.p.edgecombe@intel.com>
> > AuthorDate:    Mon, 12 Jun 2023 17:10:48 -07:00
> > Committer:     Rick Edgecombe <rick.p.edgecombe@intel.com>
> > CommitterDate: Tue, 11 Jul 2023 14:12:47 -07:00
> > 
> > mm: Don't allow write GUPs to shadow stack memory
> 
> JFYI, this commit was the last user of
> writable_file_mapping_allowed(), 
> which breaks the x86-defconfig build:
> 
>   mm/gup.c:1015:13: error: ‘writable_file_mapping_allowed’ defined
> but not used [-Werror=unused-function]
> 
> There's also a now orphan reference to
> writable_file_mapping_allowed() in 
> the comment of folio_fast_pin_allowed().

Oh, thanks for letting me know. It looks like a rebase error on my
part. This hunk should not have been dropped in that commit:
-		if (!vma_anon &&
-		    !writable_file_mapping_allowed(vma, gup_flags))
-			return -EFAULT;
Then writable_file_mapping_allowed() would still have a reference.

As for the build breakage, I'm not sure how this slipped through. My
runtime test environment was missing CONFIG_WERROR, but I got 0day
build successes for the x86_64 and i386 defconfigs specifically. I'll
have to follow up with the 0day people.

Rick
Edgecombe, Rick P July 26, 2023, 6:11 p.m. UTC | #3 
On Fri, 2023-07-21 at 09:24 -0700, Rick Edgecombe wrote:
> As for the build breakage, I'm not sure how this slipped through. My
> runtime test environment was missing CONFIG_WERROR, but I got 0day
> build successes for the x86_64 and i386 defconfigs specifically. I'll
> have to follow up with the 0day people.

Yea, there was a 0-day bug that caused it to ignore Wunused-function
warnings. Apparently 0-day also forces CONFIG_WERROR=n, so the build
erroneously succeeded.

The Wunused-function bug is fixed now on 0-day, and the fixed shadow
stack branch re-tested by it. Dave had pushed the shadow stack fix to
his repo as well, so I'm assuming he will handle getting the fix into
tip. But please let me know if you need anything from me, and sorry for
the hassle.
diff mbox series

Patch

diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index 61b5244..e95cfd3 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -1631,6 +1631,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 76d222c..86ccdb7 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1050,11 +1050,7 @@  static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags)
 		return -EFAULT;
 
 	if (write) {
-		if (!vma_anon &&
-		    !writable_file_mapping_allowed(vma, gup_flags))
-			return -EFAULT;
-
-		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. */