From: Yu-cheng Yu <yu-cheng.yu@intel.com>

The x86 family of processors do not directly create read-only and Dirty
PTEs.  These PTEs are created by software.  One such case is that kernel
read-only pages are historically setup as Dirty.

New processors that support Shadow Stack regard read-only and Dirty PTEs as
shadow stack pages.  This results in ambiguity between shadow stack and
kernel read-only pages.  To resolve this, removed Dirty from kernel read-
only pages.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 arch/x86/include/asm/pgtable_types.h | 6 +++---
 arch/x86/mm/pat/set_memory.c         | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index 40497a9020c6..3781a79b6388 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -190,10 +190,10 @@ enum page_cache_mode {
 #define _KERNPG_TABLE		 (__PP|__RW|   0|___A|   0|___D|   0|   0| _ENC)
 #define _PAGE_TABLE_NOENC	 (__PP|__RW|_USR|___A|   0|___D|   0|   0)
 #define _PAGE_TABLE		 (__PP|__RW|_USR|___A|   0|___D|   0|   0| _ENC)
-#define __PAGE_KERNEL_RO	 (__PP|   0|   0|___A|__NX|___D|   0|___G)
-#define __PAGE_KERNEL_ROX	 (__PP|   0|   0|___A|   0|___D|   0|___G)
+#define __PAGE_KERNEL_RO	 (__PP|   0|   0|___A|__NX|   0|   0|___G)
+#define __PAGE_KERNEL_ROX	 (__PP|   0|   0|___A|   0|   0|   0|___G)
 #define __PAGE_KERNEL_NOCACHE	 (__PP|__RW|   0|___A|__NX|___D|   0|___G| __NC)
-#define __PAGE_KERNEL_VVAR	 (__PP|   0|_USR|___A|__NX|___D|   0|___G)
+#define __PAGE_KERNEL_VVAR	 (__PP|   0|_USR|___A|__NX|   0|   0|___G)
 #define __PAGE_KERNEL_LARGE	 (__PP|__RW|   0|___A|__NX|___D|_PSE|___G)
 #define __PAGE_KERNEL_LARGE_EXEC (__PP|__RW|   0|___A|   0|___D|_PSE|___G)
 #define __PAGE_KERNEL_WP	 (__PP|__RW|   0|___A|__NX|___D|   0|___G| __WP)
diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
index b4072115c8ef..844bb30280b7 100644
--- a/arch/x86/mm/pat/set_memory.c
+++ b/arch/x86/mm/pat/set_memory.c
@@ -1943,7 +1943,7 @@ int set_memory_nx(unsigned long addr, int numpages)
 
 int set_memory_ro(unsigned long addr, int numpages)
 {
-	return change_page_attr_clear(&addr, numpages, __pgprot(_PAGE_RW), 0);
+	return change_page_attr_clear(&addr, numpages, __pgprot(_PAGE_RW | _PAGE_DIRTY), 0);
 }
 
 int set_memory_rw(unsigned long addr, int numpages)
-- 
2.17.1

On 1/30/22 13:18, Rick Edgecombe wrote:
> The x86 family of processors do not directly create read-only and Dirty
> PTEs.  These PTEs are created by software.

That's not strictly correct.

There's nothing in the architecture today to prevent the CPU from
creating Write=0,Dirty=1 PTEs.  In fact, some CPUs do this in weird
situations.  It wouldn't be wrong to say:

	Processors sometimes directly create read-only and Dirty PTEs.

which is the opposite of what is written above.  This is why the CET
spec has the blurb about shadow-stack-supporting CPUs promise not to do
this any more.

> One such case is that kernel
> read-only pages are historically setup as Dirty.

				   ^ set up

> New processors that support Shadow Stack regard read-only and Dirty PTEs as
> shadow stack pages.

This also isn't *quite* correct.  It's not just having a new processor,
it includes enabling shadow stacks.

> This results in ambiguity between shadow stack and kernel read-only
> pages.  To resolve this, removed Dirty from kernel read- only pages.
One thing that's not clear from the spec: does this cause an *actual*
problem?  For instance, does setting:

	IA32_U_CET.SH_STK_EN=1
but
	IA32_S_CET.SH_STK_EN=0

means that shadow stacks are enforced in user *MODE* or on
user-paging-permission (U=0) PTEs?

I think it's modes, but it would be nice to be clear.  *BUT*, if this is
accurate, doesn't it also mean that this patch is not strictly necessary?

Don't get me wrong, the patch is probably still a good idea, but let's
make sure we get the exact reasoning clear.

On Mon, 2022-02-07 at 16:13 -0800, Dave Hansen wrote:
> On 1/30/22 13:18, Rick Edgecombe wrote:
> > The x86 family of processors do not directly create read-only and
> > Dirty
> > PTEs.  These PTEs are created by software.
> 
> That's not strictly correct.
> 
> There's nothing in the architecture today to prevent the CPU from
> creating Write=0,Dirty=1 PTEs.  In fact, some CPUs do this in weird
> situations.  It wouldn't be wrong to say:
> 
> 	Processors sometimes directly create read-only and Dirty PTEs.
> 
> which is the opposite of what is written above.  This is why the CET
> spec has the blurb about shadow-stack-supporting CPUs promise not to
> do
> this any more.

Yea, it's wrong. The whole point of the new assurance is that it could
before as you say.

> 
> > One such case is that kernel
> > read-only pages are historically setup as Dirty.
> 
> 				   ^ set up
> 
> > New processors that support Shadow Stack regard read-only and Dirty
> > PTEs as
> > shadow stack pages.
> 
> This also isn't *quite* correct.  It's not just having a new
> processor,
> it includes enabling shadow stacks.

Right.

> 
> > This results in ambiguity between shadow stack and kernel read-only
> > pages.  To resolve this, removed Dirty from kernel read- only
> > pages.
> 
> One thing that's not clear from the spec: does this cause an *actual*
> problem?  For instance, does setting:
> 
> 	IA32_U_CET.SH_STK_EN=1
> but
> 	IA32_S_CET.SH_STK_EN=0
> 
> means that shadow stacks are enforced in user *MODE* or on
> user-paging-permission (U=0) PTEs?
> 
> I think it's modes, but it would be nice to be clear.  *BUT*, if this
> is
> accurate, doesn't it also mean that this patch is not strictly
> necessary?
> 
> Don't get me wrong, the patch is probably still a good idea, but
> let's
> make sure we get the exact reasoning clear.

Yea, I think this is just a tying up loose ends thing. It is not
functionally needed until there would be shadow stack mode for kernel.
I'll update the patch to make this clear.