[v7,24/41] mm: Don't allow write GUPs to shadow stack memory

Commit Message

Edgecombe, Rick P Feb. 27, 2023, 10:29 p.m. UTC

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.

Tested-by: Pengfei Xu <[e-mail]>
Tested-by: John Allen <[e-mail]>
Tested-by: Kees Cook <[e-mail]>
Acked-by: Mike Rapoport (IBM) <[e-mail]>
Acked-by: David Hildenbrand <[e-mail]>
Reviewed-by: Kees Cook <[e-mail]>
Signed-off-by: Rick Edgecombe <[e-mail]>

---
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(-)

Comments

Borislav Petkov March 6, 2023, 1:10 p.m. UTC | #1

On Mon, Feb 27, 2023 at 02:29:40PM -0800, 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

"stacks"

or "to write to a shadow stack via..."

> little less exposed, block writable GUPs for shadow stack VMAs.

GUPs?

I supposed this means "prevent get_user_pages() from pinning pages to
which the corresponding VMA is a shadow stack one."?

Or something like that which is less mm-internal speak...

Andy Lutomirski March 6, 2023, 6:15 p.m. UTC | #2

On Mon, Mar 6, 2023 at 5:10 AM Borislav Petkov <[e-mail]> wrote:
>
> On Mon, Feb 27, 2023 at 02:29:40PM -0800, 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

Is there an alternate mechanism, or do we still want to allow
FOLL_FORCE so that debuggers can write it?

--Andy

Edgecombe, Rick P March 6, 2023, 6:33 p.m. UTC | #3

On Mon, 2023-03-06 at 10:15 -0800, Andy Lutomirski wrote:
> On Mon, Mar 6, 2023 at 5:10 AM Borislav Petkov <[e-mail]> wrote:
> > 
> > On Mon, Feb 27, 2023 at 02:29:40PM -0800, 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
> 
> Is there an alternate mechanism, or do we still want to allow
> FOLL_FORCE so that debuggers can write it?

Yes, GDB shadow stack support uses it via both ptrace poke and
/proc/pid/mem apparently. So some ability to write through is needed
for debuggers. But not CRIU actually. It uses WRSS.

There was also some discussion[0] previously about how apps might
prefer to block /proc/self/mem for general security reasons. Blocking
shadow stack writes while you allow text writes is probably not that
impactful security-wise. So I thought it would be better to leave the
logic simpler. Then when /proc/self/mem could be locked down per the
discussion, shadow stack can be locked down the same way.

[0] 
https://lore.kernel.org/lkml/[e-mail]/

Andy Lutomirski March 6, 2023, 6:57 p.m. UTC | #4

On Mon, Mar 6, 2023, at 10:33 AM, Edgecombe, Rick P wrote:
> On Mon, 2023-03-06 at 10:15 -0800, Andy Lutomirski wrote:
>> On Mon, Mar 6, 2023 at 5:10 AM Borislav Petkov <[e-mail]> wrote:
>> > 
>> > On Mon, Feb 27, 2023 at 02:29:40PM -0800, 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
>> 
>> Is there an alternate mechanism, or do we still want to allow
>> FOLL_FORCE so that debuggers can write it?
>
> Yes, GDB shadow stack support uses it via both ptrace poke and
> /proc/pid/mem apparently. So some ability to write through is needed
> for debuggers. But not CRIU actually. It uses WRSS.
>
> There was also some discussion[0] previously about how apps might
> prefer to block /proc/self/mem for general security reasons. Blocking
> shadow stack writes while you allow text writes is probably not that
> impactful security-wise. So I thought it would be better to leave the
> logic simpler. Then when /proc/self/mem could be locked down per the
> discussion, shadow stack can be locked down the same way.

Ah, I am guilty of reading your changelog but not the code.

You said:

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.

I read that as *denying* FOLL_FORCE.  Maybe clarify the changelog?

>
> [0] 
> https://lore.kernel.org/lkml/[e-mail]/

Edgecombe, Rick P March 7, 2023, 1:47 a.m. UTC | #5

On Mon, 2023-03-06 at 10:57 -0800, Andy Lutomirski wrote:
> On Mon, Mar 6, 2023, at 10:33 AM, Edgecombe, Rick P wrote:
> > On Mon, 2023-03-06 at 10:15 -0800, Andy Lutomirski wrote:
> > > On Mon, Mar 6, 2023 at 5:10 AM Borislav Petkov <[e-mail]>
> > > wrote:
> > > > 
> > > > On Mon, Feb 27, 2023 at 02:29:40PM -0800, 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
> > > 
> > > Is there an alternate mechanism, or do we still want to allow
> > > FOLL_FORCE so that debuggers can write it?
> > 
> > Yes, GDB shadow stack support uses it via both ptrace poke and
> > /proc/pid/mem apparently. So some ability to write through is
> > needed
> > for debuggers. But not CRIU actually. It uses WRSS.
> > 
> > There was also some discussion[0] previously about how apps might
> > prefer to block /proc/self/mem for general security reasons.
> > Blocking
> > shadow stack writes while you allow text writes is probably not
> > that
> > impactful security-wise. So I thought it would be better to leave
> > the
> > logic simpler. Then when /proc/self/mem could be locked down per
> > the
> > discussion, shadow stack can be locked down the same way.
> 
> Ah, I am guilty of reading your changelog but not the code.
> 
> You said:
> 
> 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.
> 
> I read that as *denying* FOLL_FORCE.  Maybe clarify the changelog?

I think maybe some helpful text missed the quote in Boris comment about
other issues: "Still allow FOLL_FORCE to write through shadow stack
protections, as it does for read-only protections."

But, yea, the tenses are hard to parse. Maybe something like this:
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.

Deepak Gupta March 17, 2023, 5:05 p.m. UTC | #6

On Mon, Feb 27, 2023 at 2:31 PM Rick Edgecombe
<[e-mail]> wrote:
> diff --git a/mm/gup.c b/mm/gup.c
> index eab18ba045db..e7c7bcc0e268 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -978,7 +978,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)) {

I think I missed this in the review.
`VM_SHADOW_STACK` is an x86 specific vmaflag to represent a shadow stack VMA.
Since this is arch agnostic code. Can we instead have
`is_arch_shadow_stack_vma` which consumes vma flags and returns true.
This allows different architectures to choose whatever encoding of the
vma flag to represent a shadow stack.


>                         if (!(gup_flags & FOLL_FORCE))
>                                 return -EFAULT;
>                         /* hugetlb does not support FOLL_FORCE|FOLL_WRITE. */
> --
> 2.17.1
>

Patch

diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index d81e7ec27507..2e3d8cca1195 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -1638,6 +1638,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 eab18ba045db..e7c7bcc0e268 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -978,7 +978,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. */