Shadow Stack provides protection against function return address
corruption. It is active when the processor supports it, the kernel has
CONFIG_X86_CET_USER enabled, and the application is built for the feature.
This is only implemented for the 64-bit kernel. When it is enabled, legacy
non-Shadow Stack applications continue to work, but without protection.
Signed-off-by: Yu-cheng Yu <redacted>
---
arch/x86/Kconfig | 22 ++++++++++++++++++++++
arch/x86/Kconfig.assembler | 5 +++++
2 files changed, 27 insertions(+)
On Tue, Dec 29, 2020 at 01:30:29PM -0800, Yu-cheng Yu wrote:
quoted hunk
Shadow Stack provides protection against function return address
corruption. It is active when the processor supports it, the kernel has
CONFIG_X86_CET_USER enabled, and the application is built for the feature.
This is only implemented for the 64-bit kernel. When it is enabled, legacy
non-Shadow Stack applications continue to work, but without protection.
Signed-off-by: Yu-cheng Yu <redacted>
---
arch/x86/Kconfig | 22 ++++++++++++++++++++++
arch/x86/Kconfig.assembler | 5 +++++
2 files changed, 27 insertions(+)
On Tue, Dec 29, 2020 at 01:30:29PM -0800, Yu-cheng Yu wrote:
quoted
Shadow Stack provides protection against function return address
corruption. It is active when the processor supports it, the kernel has
CONFIG_X86_CET_USER enabled, and the application is built for the feature.
This is only implemented for the 64-bit kernel. When it is enabled, legacy
non-Shadow Stack applications continue to work, but without protection.
Signed-off-by: Yu-cheng Yu <redacted>
---
arch/x86/Kconfig | 22 ++++++++++++++++++++++
arch/x86/Kconfig.assembler | 5 +++++
2 files changed, 27 insertions(+)
That thing needs to be X86_CET. How many times do I need to type this
before you do it?
Yes, I totally understand that now. I was still thinking about
separately enabling user/kernel mode. Perhaps I should have
communicated that thought before the change. Sorry about that. I will
update it.
--
Yu-cheng
@@ -0,0 +1,136 @@+.. SPDX-License-Identifier: GPL-2.0++=========================================+Control-flow Enforcement Technology (CET)+=========================================++[1] Overview+============++Control-flow Enforcement Technology (CET) is an Intel processor feature+that provides protection against return/jump-oriented programming (ROP)+attacks. It can be set up to protect both applications and the kernel.+Only user-mode protection is implemented in the 64-bit kernel, including+support for running legacy 32-bit applications.++CET introduces Shadow Stack and Indirect Branch Tracking. Shadow stack is+a secondary stack allocated from memory and cannot be directly modified by+applications. When executing a CALL instruction, the processor pushes the+return address to both the normal stack and the shadow stack. Upon+function return, the processor pops the shadow stack copy and compares it+to the normal stack copy. If the two differ, the processor raises a+control-protection fault. Indirect branch tracking verifies indirect+CALL/JMP targets are intended as marked by the compiler with 'ENDBR'+opcodes.++There is a Kconfig option:++ X86_CET_USER.++To build a CET-enabled kernel, Binutils v2.31 and GCC v8.1 or LLVM v10.0.1+or later are required. To build a CET-enabled application, GLIBC v2.28 or+later is also required.++There are two command-line options for disabling CET features::++ no_user_shstk - disables user shadow stack, and+ no_user_ibt - disables user indirect branch tracking.++At run time, /proc/cpuinfo shows CET features if the processor supports+CET.++[2] Application Enabling+========================++An application's CET capability is marked in its ELF header and can be+verified from readelf/llvm-readelf output:++ readelf -n <application> | grep -a SHSTK+ properties: x86 feature: IBT, SHSTK++If an application supports CET and is statically linked, it will run with+CET protection. If the application needs any shared libraries, the loader+checks all dependencies and enables CET when all requirements are met.++[3] Backward Compatibility+==========================++GLIBC provides a few CET tunables via the GLIBC_TUNABLES environment+variable:++GLIBC_TUNABLES=glibc.tune.hwcaps=-SHSTK,-IBT+ Turn off SHSTK/IBT.++GLIBC_TUNABLES=glibc.tune.x86_shstk=<on, permissive>+ This controls how dlopen() handles SHSTK legacy libraries::++ on - continue with SHSTK enabled;+ permissive - continue with SHSTK off.++Details can be found in the GLIBC manual pages.++[4] CET arch_prctl()'s+======================++Several arch_prctl()'s have been added for CET:++arch_prctl(ARCH_X86_CET_STATUS, u64 *addr)+ Return CET feature status.++ The parameter 'addr' is a pointer to a user buffer.+ On returning to the caller, the kernel fills the following+ information::++ *addr = shadow stack/indirect branch tracking status+ *(addr + 1) = shadow stack base address+ *(addr + 2) = shadow stack size++arch_prctl(ARCH_X86_CET_DISABLE, unsigned int features)+ Disable shadow stack and/or indirect branch tracking as specified in+ 'features'. Return -EPERM if CET is locked.++arch_prctl(ARCH_X86_CET_LOCK)+ Lock in all CET features. They cannot be turned off afterwards.++Note:+ There is no CET-enabling arch_prctl function. By design, CET is enabled+ automatically if the binary and the system can support it.++[5] The implementation of the Shadow Stack+==========================================++Shadow Stack size+-----------------++A task's shadow stack is allocated from memory to a fixed size of+MIN(RLIMIT_STACK, 4 GB). In other words, the shadow stack is allocated to+the maximum size of the normal stack, but capped to 4 GB. However,+a compat-mode application's address space is smaller, each of its thread's+shadow stack size is MIN(1/4 RLIMIT_STACK, 4 GB).++Signal+------++The main program and its signal handlers use the same shadow stack.+Because the shadow stack stores only return addresses, a large shadow+stack covers the condition that both the program stack and the signal+alternate stack run out.++The kernel creates a restore token for the shadow stack restoring address+and verifies that token when restoring from the signal handler.++Fork+----++The shadow stack's vma has VM_SHSTK flag set; its PTEs are required to be+read-only and dirty. When a shadow stack PTE is not RO and dirty, a+shadow access triggers a page fault with the shadow stack access bit set+in the page fault error code.++When a task forks a child, its shadow stack PTEs are copied and both the+parent's and the child's shadow stack PTEs are cleared of the dirty bit.+Upon the next shadow stack access, the resulting shadow stack page fault+is handled by page copy/re-use.++When a pthread child is created, the kernel allocates a new shadow stack+for the new thread.
No need for that function - just add this two-liner to bsp_init_intel()
and not in get_cpu_cap().
+static void adjust_combined_cpu_features(void)
+{
+#ifdef CONFIG_X86_CET_USER
+ if (test_bit(X86_FEATURE_SHSTK, (unsigned long *)cpu_caps_cleared) &&
+ test_bit(X86_FEATURE_IBT, (unsigned long *)cpu_caps_cleared))
+ setup_clear_cpu_cap(X86_FEATURE_CET);
+#endif
There's no need for this function...
quoted hunk
+}
+
/*
* We parse cpu parameters early because fpu__init_system() is executed
* before parse_early_param().
@@ -1252,9 +1276,19 @@ static void __init cpu_parse_early_param(void) if (cmdline_find_option_bool(boot_command_line, "noxsaves")) setup_clear_cpu_cap(X86_FEATURE_XSAVES);+ /*+ * CET states are XSAVES states and options must be parsed early.+ */+#ifdef CONFIG_X86_CET_USER+ if (cmdline_find_option_bool(boot_command_line, "no_user_shstk"))+ setup_clear_cpu_cap(X86_FEATURE_SHSTK);
... when you can do
setup_clear_cpu_cap(X86_FEATURE_CET);
here and...
+ if (cmdline_find_option_bool(boot_command_line, "no_user_ibt"))
+ setup_clear_cpu_cap(X86_FEATURE_IBT);
No need for that function - just add this two-liner to bsp_init_intel()
and not in get_cpu_cap().
I will move these to bsp_init_intel(), and change to:
if (cpu_has(c, X86_FEATURE_SHSTK) || cpu_has(c, X86_FEATURE_IBT))
setup_force_cpu_cap(X86_FEATURE_CET);
quoted
+static void adjust_combined_cpu_features(void)
+{
+#ifdef CONFIG_X86_CET_USER
+ if (test_bit(X86_FEATURE_SHSTK, (unsigned long *)cpu_caps_cleared) &&
+ test_bit(X86_FEATURE_IBT, (unsigned long *)cpu_caps_cleared))
+ setup_clear_cpu_cap(X86_FEATURE_CET);
+#endif
There's no need for this function...
quoted
+}
+
/*
* We parse cpu parameters early because fpu__init_system() is executed
* before parse_early_param().
@@ -1252,9 +1276,19 @@ static void __init cpu_parse_early_param(void) if (cmdline_find_option_bool(boot_command_line, "noxsaves")) setup_clear_cpu_cap(X86_FEATURE_XSAVES);+ /*+ * CET states are XSAVES states and options must be parsed early.+ */+#ifdef CONFIG_X86_CET_USER+ if (cmdline_find_option_bool(boot_command_line, "no_user_shstk"))+ setup_clear_cpu_cap(X86_FEATURE_SHSTK);
... when you can do
setup_clear_cpu_cap(X86_FEATURE_CET);
here and...
quoted
+ if (cmdline_find_option_bool(boot_command_line, "no_user_ibt"))
+ setup_clear_cpu_cap(X86_FEATURE_IBT);
... here.
Two problems here. X86_FEATURE_CET indicates either CET features is
enabled, not both. Also, "clearcpuid" can has CET features. However,
since X86_FEATURE_CET is now set in bsp_init_intel() (after
cpu_parse_early_params()), I think, adjust_combined_cpu_features() can
be removed. I will test it.
--
Thanks,
Yu-cheng
This is a minor revision of the original [PATCH v17 04/26] patch. The
purpose of the revision is to address Boris' comment earlier by doing
setup_force_cpu_cap(X86_FEATURE_CET) in bsp_init_intel().
--
Yu-cheng
On Mon, Jan 11, 2021 at 03:09:00PM -0800, Yu-cheng Yu wrote:
quoted hunk
@@ -1252,6 +1260,16 @@ static void __init cpu_parse_early_param(void) if (cmdline_find_option_bool(boot_command_line, "noxsaves")) setup_clear_cpu_cap(X86_FEATURE_XSAVES);+ /*+ * CET states are XSAVES states and options must be parsed early.+ */
On Mon, Jan 11, 2021 at 03:09:00PM -0800, Yu-cheng Yu wrote:
quoted
@@ -1252,6 +1260,16 @@ static void __init cpu_parse_early_param(void) if (cmdline_find_option_bool(boot_command_line, "noxsaves")) setup_clear_cpu_cap(X86_FEATURE_XSAVES);+ /*+ * CET states are XSAVES states and options must be parsed early.+ */
That comment is redundant - look at the containing function's name.
Otherwise patch looks just as it should.
Thx.
Should I send an updated patch? Thanks!
--
Yu-cheng
On Tue, Jan 12, 2021 at 03:02:06PM -0800, Yu, Yu-cheng wrote:
Should I send an updated patch? Thanks!
No, this is not how review works.
Usually, you send your patchset, wait a week or two to gather feedback,
incorporate that feedback or discuss/dispute it and you send your next
version. You should know the process by now...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
There is essentially no room left in the x86 hardware PTEs on some OSes
(not Linux). That left the hardware architects looking for a way to
represent a new memory type (shadow stack) within the existing bits.
They chose to repurpose a lightly-used state: Write=0, Dirty=1.
The reason it's lightly used is that Dirty=1 is normally set by hardware
and cannot normally be set by hardware on a Write=0 PTE. Software must
normally be involved to create one of these PTEs, so software can simply
opt to not create them.
In places where Linux normally creates Write=0, Dirty=1, it can use the
software-defined _PAGE_COW in place of the hardware _PAGE_DIRTY. In other
words, whenever Linux needs to create Write=0, Dirty=1, it instead creates
Write=0, Cow=1, except for shadow stack, which is Write=0, Dirty=1. This
clearly separates shadow stack from other data, and results in the
following:
(a) A modified, copy-on-write (COW) page: (Write=0, Cow=1)
(b) A R/O page that has been COW'ed: (Write=0, Cow=1)
The user page is in a R/O VMA, and get_user_pages() needs a writable
copy. The page fault handler creates a copy of the page and sets
the new copy's PTE as Write=0 and Cow=1.
(c) A shadow stack PTE: (Write=0, Dirty=1)
(d) A shared shadow stack PTE: (Write=0, Cow=1)
When a shadow stack page is being shared among processes (this happens
at fork()), its PTE is made Dirty=0, so the next shadow stack access
causes a fault, and the page is duplicated and Dirty=1 is set again.
This is the COW equivalent for shadow stack pages, even though it's
copy-on-access rather than copy-on-write.
(e) A page where the processor observed a Write=1 PTE, started a write, set
Dirty=1, but then observed a Write=0 PTE. That's possible today, but
will not happen on processors that support shadow stack.
Define _PAGE_COW and update pte_*() helpers and apply the same changes to
pmd and pud.
After this, there are six free bits left in the 64-bit PTE, and no more
free bits in the 32-bit PTE (except for PAE) and Shadow Stack is not
implemented for the 32-bit kernel.
Signed-off-by: Yu-cheng Yu <redacted>
---
arch/x86/include/asm/pgtable.h | 117 ++++++++++++++++++++++++---
arch/x86/include/asm/pgtable_types.h | 42 +++++++++-
2 files changed, 148 insertions(+), 11 deletions(-)
@@ -23,7 +23,8 @@#define _PAGE_BIT_SOFTW2 10 /* " */#define _PAGE_BIT_SOFTW3 11 /* " */#define _PAGE_BIT_PAT_LARGE 12 /* On 2MB or 1GB pages */-#define _PAGE_BIT_SOFTW4 58 /* available for programmer */+#define _PAGE_BIT_SOFTW4 57 /* available for programmer */+#define _PAGE_BIT_SOFTW5 58 /* available for programmer */#define _PAGE_BIT_PKEY_BIT0 59 /* Protection Keys, bit 1/4 */#define _PAGE_BIT_PKEY_BIT1 60 /* Protection Keys, bit 2/4 */#define _PAGE_BIT_PKEY_BIT2 61 /* Protection Keys, bit 3/4 */
@@ -36,6 +37,15 @@#define _PAGE_BIT_SOFT_DIRTY _PAGE_BIT_SOFTW3 /* software dirty tracking */#define _PAGE_BIT_DEVMAP _PAGE_BIT_SOFTW4+/*+*Indicatesacopy-on-writepage.+*/+#ifdef CONFIG_X86_CET_USER+#define _PAGE_BIT_COW _PAGE_BIT_SOFTW5 /* copy-on-write */+#else+#define _PAGE_BIT_COW 0+#endif+/* If _PAGE_BIT_PRESENT is clear, we use these: *//* - if the user mapped it with PROT_NONE; pte_present gives true */#define _PAGE_BIT_PROTNONE _PAGE_BIT_GLOBAL
On Tue, Dec 29, 2020 at 01:30:35PM -0800, Yu-cheng Yu wrote:
quoted hunk
@@ -182,6 +182,12 @@ static inline int pud_young(pud_t pud) static inline int pte_write(pte_t pte) {+ /*+ * If _PAGE_DIRTY is set, the PTE must either have _PAGE_RW or be+ * a shadow stack PTE, which is logically writable.+ */+ if (cpu_feature_enabled(X86_FEATURE_SHSTK))+ return pte_flags(pte) & (_PAGE_RW | _PAGE_DIRTY); return pte_flags(pte) & _PAGE_RW;
if (cpu_feature_enabled(X86_FEATURE_SHSTK))
return pte_flags(pte) & (_PAGE_RW | _PAGE_DIRTY);
else
return pte_flags(pte) & _PAGE_RW;
The else makes it ballanced and easier to read.
@@ -343,6 +349,16 @@ static inline pte_t pte_mkold(pte_t pte) static inline pte_t pte_wrprotect(pte_t pte) {+ /*+ * Blindly clearing _PAGE_RW might accidentally create+ * a shadow stack PTE (RW=0, Dirty=1). Move the hardware+ * dirty value to the software bit.+ */+ if (cpu_feature_enabled(X86_FEATURE_SHSTK)) {+ pte.pte |= (pte.pte & _PAGE_DIRTY) >> _PAGE_BIT_DIRTY << _PAGE_BIT_COW;
Why the unreadable shifting when you can simply do:
if (pte.pte & _PAGE_DIRTY)
pte.pte |= _PAGE_COW;
?
quoted hunk
@@ -434,16 +469,40 @@ static inline pmd_t pmd_mkold(pmd_t pmd) static inline pmd_t pmd_mkclean(pmd_t pmd) {- return pmd_clear_flags(pmd, _PAGE_DIRTY);+ return pmd_clear_flags(pmd, _PAGE_DIRTY_BITS); } static inline pmd_t pmd_wrprotect(pmd_t pmd) {+ /*+ * Blindly clearing _PAGE_RW might accidentally create+ * a shadow stack PMD (RW=0, Dirty=1). Move the hardware+ * dirty value to the software bit.+ */+ if (cpu_feature_enabled(X86_FEATURE_SHSTK)) {+ pmdval_t v = native_pmd_val(pmd);++ v |= (v & _PAGE_DIRTY) >> _PAGE_BIT_DIRTY << _PAGE_BIT_COW;
As above.
quoted hunk
@@ -488,17 +554,35 @@ static inline pud_t pud_mkold(pud_t pud) static inline pud_t pud_mkclean(pud_t pud) {- return pud_clear_flags(pud, _PAGE_DIRTY);+ return pud_clear_flags(pud, _PAGE_DIRTY_BITS); } static inline pud_t pud_wrprotect(pud_t pud) {+ /*+ * Blindly clearing _PAGE_RW might accidentally create+ * a shadow stack PUD (RW=0, Dirty=1). Move the hardware+ * dirty value to the software bit.+ */+ if (cpu_feature_enabled(X86_FEATURE_SHSTK)) {+ pudval_t v = native_pud_val(pud);++ v |= (v & _PAGE_DIRTY) >> _PAGE_BIT_DIRTY << _PAGE_BIT_COW;
Ditto.
quoted hunk
@@ -1131,6 +1222,12 @@ extern int pmdp_clear_flush_young(struct vm_area_struct *vma, #define pmd_write pmd_write static inline int pmd_write(pmd_t pmd) {+ /*+ * If _PAGE_DIRTY is set, then the PMD must either have _PAGE_RW or+ * be a shadow stack PMD, which is logically writable.+ */+ if (cpu_feature_enabled(X86_FEATURE_SHSTK))+ return pmd_flags(pmd) & (_PAGE_RW | _PAGE_DIRTY);
On Tue, Dec 29, 2020 at 01:30:35PM -0800, Yu-cheng Yu wrote:
[...]
quoted
@@ -343,6 +349,16 @@ static inline pte_t pte_mkold(pte_t pte) static inline pte_t pte_wrprotect(pte_t pte) {+ /*+ * Blindly clearing _PAGE_RW might accidentally create+ * a shadow stack PTE (RW=0, Dirty=1). Move the hardware+ * dirty value to the software bit.+ */+ if (cpu_feature_enabled(X86_FEATURE_SHSTK)) {+ pte.pte |= (pte.pte & _PAGE_DIRTY) >> _PAGE_BIT_DIRTY << _PAGE_BIT_COW;
Why the unreadable shifting when you can simply do:
if (pte.pte & _PAGE_DIRTY)
pte.pte |= _PAGE_COW;
?
It clears _PAGE_DIRTY and sets _PAGE_COW. That is,
if (pte.pte & _PAGE_DIRTY) {
pte.pte &= ~_PAGE_DIRTY;
pte.pte |= _PAGE_COW;
}
So, shifting makes resulting code more efficient.
quoted
@@ -434,16 +469,40 @@ static inline pmd_t pmd_mkold(pmd_t pmd) static inline pmd_t pmd_mkclean(pmd_t pmd) {- return pmd_clear_flags(pmd, _PAGE_DIRTY);+ return pmd_clear_flags(pmd, _PAGE_DIRTY_BITS); } static inline pmd_t pmd_wrprotect(pmd_t pmd) {+ /*+ * Blindly clearing _PAGE_RW might accidentally create+ * a shadow stack PMD (RW=0, Dirty=1). Move the hardware+ * dirty value to the software bit.+ */+ if (cpu_feature_enabled(X86_FEATURE_SHSTK)) {+ pmdval_t v = native_pmd_val(pmd);++ v |= (v & _PAGE_DIRTY) >> _PAGE_BIT_DIRTY << _PAGE_BIT_COW;
As above.
quoted
@@ -488,17 +554,35 @@ static inline pud_t pud_mkold(pud_t pud) static inline pud_t pud_mkclean(pud_t pud) {- return pud_clear_flags(pud, _PAGE_DIRTY);+ return pud_clear_flags(pud, _PAGE_DIRTY_BITS); } static inline pud_t pud_wrprotect(pud_t pud) {+ /*+ * Blindly clearing _PAGE_RW might accidentally create+ * a shadow stack PUD (RW=0, Dirty=1). Move the hardware+ * dirty value to the software bit.+ */+ if (cpu_feature_enabled(X86_FEATURE_SHSTK)) {+ pudval_t v = native_pud_val(pud);++ v |= (v & _PAGE_DIRTY) >> _PAGE_BIT_DIRTY << _PAGE_BIT_COW;
Ditto.
quoted
@@ -1131,6 +1222,12 @@ extern int pmdp_clear_flush_young(struct vm_area_struct *vma, #define pmd_write pmd_write static inline int pmd_write(pmd_t pmd) {+ /*+ * If _PAGE_DIRTY is set, then the PMD must either have _PAGE_RW or+ * be a shadow stack PMD, which is logically writable.+ */+ if (cpu_feature_enabled(X86_FEATURE_SHSTK))+ return pmd_flags(pmd) & (_PAGE_RW | _PAGE_DIRTY);
static inline pte_t pte_wrprotect(pte_t pte)
{
+ /*
+ * Blindly clearing _PAGE_RW might accidentally create
+ * a shadow stack PTE (RW=0, Dirty=1). Move the hardware
+ * dirty value to the software bit.
+ */
+ if (cpu_feature_enabled(X86_FEATURE_SHSTK)) {
+ pte.pte |= (pte.pte & _PAGE_DIRTY) >> _PAGE_BIT_DIRTY <<
_PAGE_BIT_COW;
Why the unreadable shifting when you can simply do:
if (pte.pte & _PAGE_DIRTY)
pte.pte |= _PAGE_COW;
?
It clears _PAGE_DIRTY and sets _PAGE_COW. That is,
if (pte.pte & _PAGE_DIRTY) {
pte.pte &= ~_PAGE_DIRTY;
pte.pte |= _PAGE_COW;
}
So, shifting makes resulting code more efficient.
Are you sure?
Usually, the compiler is better at making code efficient than humans. I
find that coding it in the most human-readable way is best unless I
*know* the compiler is unable to generate god code.
From: Dave Hansen <hidden> Date: 2021-01-21 20:28:29
Usually, the compiler is better at making code efficient than humans. I
find that coding it in the most human-readable way is best unless I
*know* the compiler is unable to generate god code.
"good code", even.
I really want a "god code" compiler, though. :)
Usually, the compiler is better at making code efficient than humans. I
find that coding it in the most human-readable way is best unless I
*know* the compiler is unable to generate god code.
"good code", even.
I really want a "god code" compiler, though. :)
With my version of GCC, the shifting implementation creates five
instructions, all operate on registers only. The other implementation
also creates five instructions, but introduces one jump and one memory
access. But, you are right, being readable is also important. Maybe we
can tweak it a little or create something similar to those in bitops.
--
Yu-cheng
On Thu, Jan 21, 2021 at 12:16:23PM -0800, Yu, Yu-cheng wrote:
It clears _PAGE_DIRTY and sets _PAGE_COW. That is,
if (pte.pte & _PAGE_DIRTY) {
pte.pte &= ~_PAGE_DIRTY;
pte.pte |= _PAGE_COW;
}
So, shifting makes resulting code more efficient.
On Thu, Jan 21, 2021 at 12:16:23PM -0800, Yu, Yu-cheng wrote:
quoted
It clears _PAGE_DIRTY and sets _PAGE_COW. That is,
if (pte.pte & _PAGE_DIRTY) {
pte.pte &= ~_PAGE_DIRTY;
pte.pte |= _PAGE_COW;
}
So, shifting makes resulting code more efficient.
Efficient for what? Is this a hot path?
If not, I'd take readable code any day of the week.
Ok, I will change it to the more readable code as stated earlier.
--
Yu-cheng
From: David Laight <hidden> Date: 2021-01-21 22:18:41
From: Yu, Yu-cheng
On 1/21/2021 10:44 AM, Borislav Petkov wrote:
quoted
On Tue, Dec 29, 2020 at 01:30:35PM -0800, Yu-cheng Yu wrote:
[...]
quoted
quoted
@@ -343,6 +349,16 @@ static inline pte_t pte_mkold(pte_t pte) static inline pte_t pte_wrprotect(pte_t pte) {+ /*+ * Blindly clearing _PAGE_RW might accidentally create+ * a shadow stack PTE (RW=0, Dirty=1). Move the hardware+ * dirty value to the software bit.+ */+ if (cpu_feature_enabled(X86_FEATURE_SHSTK)) {+ pte.pte |= (pte.pte & _PAGE_DIRTY) >> _PAGE_BIT_DIRTY << _PAGE_BIT_COW;
Why the unreadable shifting when you can simply do:
if (pte.pte & _PAGE_DIRTY)
pte.pte |= _PAGE_COW;
quoted
?
It clears _PAGE_DIRTY and sets _PAGE_COW. That is,
if (pte.pte & _PAGE_DIRTY) {
pte.pte &= ~_PAGE_DIRTY;
pte.pte |= _PAGE_COW;
}
So, shifting makes resulting code more efficient.
Does the compiler manage to do one shift?
How can it clear anything?
There is only an |= against the target.
Something horrid with ^= might set and clear.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: Randy Dunlap <hidden> Date: 2021-01-21 22:23:58
On 1/21/21 2:16 PM, David Laight wrote:
From: Yu, Yu-cheng
quoted
On 1/21/2021 10:44 AM, Borislav Petkov wrote:
quoted
On Tue, Dec 29, 2020 at 01:30:35PM -0800, Yu-cheng Yu wrote:
[...]
quoted
quoted
@@ -343,6 +349,16 @@ static inline pte_t pte_mkold(pte_t pte) static inline pte_t pte_wrprotect(pte_t pte) {+ /*+ * Blindly clearing _PAGE_RW might accidentally create+ * a shadow stack PTE (RW=0, Dirty=1). Move the hardware+ * dirty value to the software bit.+ */+ if (cpu_feature_enabled(X86_FEATURE_SHSTK)) {+ pte.pte |= (pte.pte & _PAGE_DIRTY) >> _PAGE_BIT_DIRTY << _PAGE_BIT_COW;
Why the unreadable shifting when you can simply do:
if (pte.pte & _PAGE_DIRTY)
pte.pte |= _PAGE_COW;
quoted
quoted
?
It clears _PAGE_DIRTY and sets _PAGE_COW. That is,
if (pte.pte & _PAGE_DIRTY) {
pte.pte &= ~_PAGE_DIRTY;
pte.pte |= _PAGE_COW;
}
So, shifting makes resulting code more efficient.
Does the compiler manage to do one shift?
How can it clear anything?
It could shift it off either end since there are both
<< and >>.
There is only an |= against the target.
Something horrid with ^= might set and clear.
--
~Randy
"He closes his eyes and drops the goggles. You can't get hurt
by looking at a bitmap. Or can you?"
(Neal Stephenson: Snow Crash)
From: David Laight <hidden> Date: 2021-01-21 22:35:02
From: Randy Dunlap
Sent: 21 January 2021 22:19
On 1/21/21 2:16 PM, David Laight wrote:
quoted
From: Yu, Yu-cheng
quoted
On 1/21/2021 10:44 AM, Borislav Petkov wrote:
quoted
On Tue, Dec 29, 2020 at 01:30:35PM -0800, Yu-cheng Yu wrote:
[...]
quoted
quoted
@@ -343,6 +349,16 @@ static inline pte_t pte_mkold(pte_t pte) static inline pte_t pte_wrprotect(pte_t pte) {+ /*+ * Blindly clearing _PAGE_RW might accidentally create+ * a shadow stack PTE (RW=0, Dirty=1). Move the hardware+ * dirty value to the software bit.+ */+ if (cpu_feature_enabled(X86_FEATURE_SHSTK)) {+ pte.pte |= (pte.pte & _PAGE_DIRTY) >> _PAGE_BIT_DIRTY << _PAGE_BIT_COW;
Why the unreadable shifting when you can simply do:
if (pte.pte & _PAGE_DIRTY)
pte.pte |= _PAGE_COW;
quoted
quoted
?
It clears _PAGE_DIRTY and sets _PAGE_COW. That is,
if (pte.pte & _PAGE_DIRTY) {
pte.pte &= ~_PAGE_DIRTY;
pte.pte |= _PAGE_COW;
}
So, shifting makes resulting code more efficient.
Does the compiler manage to do one shift?
How can it clear anything?
It could shift it off either end since there are both << and >>.
It is still:
pte.pte |= xxxxxxx;
quoted
There is only an |= against the target.
Something horrid with ^= might set and clear.
It could be 4 instructions:
is_dirty = pte.pte & PAGE_DIRTY;
pte.pte &= ~PAGE_DIRTY; // or pte.pte ^= is_dirty
is_cow = is_dirty << (BIT_COW - BIT_DIRTY); // or equivalent >>
pte.pte |= is_cow;
provided you've a three operand form for one of the first two instructions.
Something like ARM might manage to merge the last two as well.
But the register dependency chain length may matter more than
the number of instructions.
The above is likely to be three long.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Sent: 21 January 2021 22:19
On 1/21/21 2:16 PM, David Laight wrote:
quoted
From: Yu, Yu-cheng
quoted
On 1/21/2021 10:44 AM, Borislav Petkov wrote:
quoted
On Tue, Dec 29, 2020 at 01:30:35PM -0800, Yu-cheng Yu wrote:
[...]
quoted
quoted
@@ -343,6 +349,16 @@ static inline pte_t pte_mkold(pte_t pte) static inline pte_t pte_wrprotect(pte_t pte) {+ /*+ * Blindly clearing _PAGE_RW might accidentally create+ * a shadow stack PTE (RW=0, Dirty=1). Move the hardware+ * dirty value to the software bit.+ */+ if (cpu_feature_enabled(X86_FEATURE_SHSTK)) {+ pte.pte |= (pte.pte & _PAGE_DIRTY) >> _PAGE_BIT_DIRTY << _PAGE_BIT_COW;
Why the unreadable shifting when you can simply do:
if (pte.pte & _PAGE_DIRTY)
pte.pte |= _PAGE_COW;
quoted
quoted
?
It clears _PAGE_DIRTY and sets _PAGE_COW. That is,
if (pte.pte & _PAGE_DIRTY) {
pte.pte &= ~_PAGE_DIRTY;
pte.pte |= _PAGE_COW;
}
So, shifting makes resulting code more efficient.
Does the compiler manage to do one shift?
How can it clear anything?
It could shift it off either end since there are both << and >>.
It is still:
pte.pte |= xxxxxxx;
quoted
quoted
There is only an |= against the target.
Something horrid with ^= might set and clear.
It could be 4 instructions:
is_dirty = pte.pte & PAGE_DIRTY;
pte.pte &= ~PAGE_DIRTY; // or pte.pte ^= is_dirty
is_cow = is_dirty << (BIT_COW - BIT_DIRTY); // or equivalent >>
pte.pte |= is_cow;
provided you've a three operand form for one of the first two instructions.
Something like ARM might manage to merge the last two as well.
But the register dependency chain length may matter more than
the number of instructions.
The above is likely to be three long.
I see what you are saying. The patch is like...
if (cpu_feature_enabled(X86_FEATURE_SHSTK)) {
pte.pte |= (pte.pte & _PAGE_DIRTY) >> _PAGE_BIT_DIRTY << _PAGE_BIT_COW;
pte = pte_clear_flags(pte, _PAGE_DIRTY);
}
It is not necessary to do the shifting. I will make it, simply,
if (pte.pte & _PAGE_DIRTY) {
pte.pte &= ~PAGE_DIRTY;
pte.pte |= _PAGE_COW;
}
Thanks for your comments.
--
Yu-cheng
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 <redacted>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Kees Cook <redacted>
Cc: Thomas Gleixner <redacted>
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(-)
@@ -1932,7 +1932,7 @@ int set_memory_nx(unsigned long addr, int numpages)intset_memory_ro(unsignedlongaddr,intnumpages){-returnchange_page_attr_clear(&addr,numpages,__pgprot(_PAGE_RW),0);+returnchange_page_attr_clear(&addr,numpages,__pgprot(_PAGE_RW|_PAGE_DIRTY),0);}intset_memory_rw(unsignedlongaddr,intnumpages)
After the introduction of _PAGE_COW, a modified page's PTE can have either
_PAGE_DIRTY or _PAGE_COW. Change _PAGE_DIRTY to _PAGE_DIRTY_BITS.
Signed-off-by: Yu-cheng Yu <redacted>
Reviewed-by: Kees Cook <redacted>
Cc: David Airlie <redacted>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Daniel Vetter <redacted>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Zhenyu Wang <redacted>
Cc: Zhi Wang <redacted>
---
drivers/gpu/drm/i915/gvt/gtt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
A control-protection fault is triggered when a control-flow transfer
attempt violates Shadow Stack or Indirect Branch Tracking constraints.
For example, the return address for a RET instruction differs from the copy
on the shadow stack; or an indirect JMP instruction, without the NOTRACK
prefix, arrives at a non-ENDBR opcode.
The control-protection fault handler works in a similar way as the general
protection fault handler. It provides the si_code SEGV_CPERR to the signal
handler.
Signed-off-by: Yu-cheng Yu <redacted>
---
arch/x86/include/asm/idtentry.h | 4 ++
arch/x86/kernel/idt.c | 4 ++
arch/x86/kernel/signal_compat.c | 2 +-
arch/x86/kernel/traps.c | 59 ++++++++++++++++++++++++++++++
include/uapi/asm-generic/siginfo.h | 3 +-
5 files changed, 70 insertions(+), 2 deletions(-)
@@ -574,6 +574,10 @@ DECLARE_IDTENTRY_ERRORCODE(X86_TRAP_SS, exc_stack_segment);DECLARE_IDTENTRY_ERRORCODE(X86_TRAP_GP,exc_general_protection);DECLARE_IDTENTRY_ERRORCODE(X86_TRAP_AC,exc_alignment_check);+#ifdef CONFIG_X86_CET_USER+DECLARE_IDTENTRY_ERRORCODE(X86_TRAP_CP,exc_control_protection);+#endif+/* Raw exception entries which need extra work */DECLARE_IDTENTRY_RAW(X86_TRAP_UD,exc_invalid_op);DECLARE_IDTENTRY_RAW(X86_TRAP_BP,exc_int3);
@@ -606,6 +606,65 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)cond_local_irq_disable(regs);}+#ifdef CONFIG_X86_CET_USER+staticconstchar*constcontrol_protection_err[]={+"unknown",+"near-ret",+"far-ret/iret",+"endbranch",+"rstorssp",+"setssbsy",+};++/*+*Whenacontrolprotectionexceptionoccurs,sendasignaltotheresponsible+*application.Currently,controlprotectionisonlyenabledfortheuser+*mode.Thisexceptionshouldnotcomefromthekernelmode.+*/+DEFINE_IDTENTRY_ERRORCODE(exc_control_protection)+{+structtask_struct*tsk;++if(!user_mode(regs)){+if(notify_die(DIE_TRAP,"control protection fault",regs,+error_code,X86_TRAP_CP,SIGSEGV)==NOTIFY_STOP)+return;+die("Upexpected/unsupported kernel control protection fault",regs,error_code);+}++cond_local_irq_enable(regs);++if(!boot_cpu_has(X86_FEATURE_CET))+WARN_ONCE(1,"Control protection fault with CET support disabled\n");++tsk=current;+tsk->thread.error_code=error_code;+tsk->thread.trap_nr=X86_TRAP_CP;++if(show_unhandled_signals&&unhandled_signal(tsk,SIGSEGV)&&+printk_ratelimit()){+unsignedintmax_err;+unsignedlongssp;++max_err=ARRAY_SIZE(control_protection_err)-1;+if((error_code<0)||(error_code>max_err))+error_code=0;++rdmsrl(MSR_IA32_PL3_SSP,ssp);+pr_info("%s[%d] control protection ip:%lx sp:%lx ssp:%lx error:%lx(%s)",+tsk->comm,task_pid_nr(tsk),+regs->ip,regs->sp,ssp,error_code,+control_protection_err[error_code]);+print_vma_addr(KERN_CONT" in ",regs->ip);+pr_cont("\n");+}++force_sig_fault(SIGSEGV,SEGV_CPERR,+(void__user*)uprobe_get_trap_addr(regs));+cond_local_irq_disable(regs);+}+#endif+staticbooldo_int3(structpt_regs*regs){intres;
On Tue, Dec 29, 2020 at 01:30:33PM -0800, Yu-cheng Yu wrote:
quoted hunk
@@ -606,6 +606,65 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection) cond_local_irq_disable(regs); }+#ifdef CONFIG_X86_CET_USER+static const char * const control_protection_err[] = {+ "unknown",+ "near-ret",+ "far-ret/iret",+ "endbranch",+ "rstorssp",+ "setssbsy",+};++/*+ * When a control protection exception occurs, send a signal to the responsible+ * application. Currently, control protection is only enabled for the user+ * mode. This exception should not come from the kernel mode.+ */
There's no "the user mode" or "the kernel mode" - just "user mode" or
"kernel mode".
Isn't the machine supposed to panic() here and do no further progress?
+ }
+
+ cond_local_irq_enable(regs);
+
+ if (!boot_cpu_has(X86_FEATURE_CET))
+ WARN_ONCE(1, "Control protection fault with CET support disabled\n");
+
+ tsk = current;
+ tsk->thread.error_code = error_code;
+ tsk->thread.trap_nr = X86_TRAP_CP;
+
+ if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) &&
+ printk_ratelimit()) {
WARNING: Prefer printk_ratelimited or pr_<level>_ratelimited to printk_ratelimit
#136: FILE: arch/x86/kernel/traps.c:645:
+ printk_ratelimit()) {
Still not using checkpatch?
+ unsigned int max_err;
+ unsigned long ssp;
+
+ max_err = ARRAY_SIZE(control_protection_err) - 1;
+ if ((error_code < 0) || (error_code > max_err))
+ error_code = 0;
+
+ rdmsrl(MSR_IA32_PL3_SSP, ssp);
+ pr_info("%s[%d] control protection ip:%lx sp:%lx ssp:%lx error:%lx(%s)",
Isn't the machine supposed to panic() here and do no further progress?
Ok, make it panic().
quoted
+ }
+
+ cond_local_irq_enable(regs);
+
+ if (!boot_cpu_has(X86_FEATURE_CET))
+ WARN_ONCE(1, "Control protection fault with CET support disabled\n");
+
+ tsk = current;
+ tsk->thread.error_code = error_code;
+ tsk->thread.trap_nr = X86_TRAP_CP;
+
+ if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) &&
+ printk_ratelimit()) {
WARNING: Prefer printk_ratelimited or pr_<level>_ratelimited to printk_ratelimit
#136: FILE: arch/x86/kernel/traps.c:645:
+ printk_ratelimit()) {
Still not using checkpatch?
Most places in arch/x86 still use printk_ratelimit(). I should have
trusted checkpatch. I will fix it.
quoted
+ unsigned int max_err;
+ unsigned long ssp;
+
+ max_err = ARRAY_SIZE(control_protection_err) - 1;
+ if ((error_code < 0) || (error_code > max_err))
+ error_code = 0;
+
+ rdmsrl(MSR_IA32_PL3_SSP, ssp);
+ pr_info("%s[%d] control protection ip:%lx sp:%lx ssp:%lx error:%lx(%s)",
When Shadow Stack is introduced, [R/O + _PAGE_DIRTY] PTE is reserved for
shadow stack. Copy-on-write PTEs have [R/O + _PAGE_COW].
When a PTE goes from [R/W + _PAGE_DIRTY] to [R/O + _PAGE_COW], it could
become a transient shadow stack PTE in two cases:
The first case is that some processors can start a write but end up seeing
a read-only PTE by the time they get to the Dirty bit, creating a transient
shadow stack PTE. However, this will not occur on processors supporting
Shadow Stack, therefore we don't need a TLB flush here.
The second case is that when the software, without atomic, tests & replaces
_PAGE_DIRTY with _PAGE_COW, a transient shadow stack PTE can exist.
This is prevented with cmpxchg.
Dave Hansen, Jann Horn, Andy Lutomirski, and Peter Zijlstra provided many
insights to the issue. Jann Horn provided the cmpxchg solution.
Signed-off-by: Yu-cheng Yu <redacted>
Reviewed-by: Kees Cook <redacted>
---
arch/x86/include/asm/pgtable.h | 52 ++++++++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
On Tue, Dec 29, 2020 at 01:30:38PM -0800, Yu-cheng Yu wrote:
When Shadow Stack is introduced, [R/O + _PAGE_DIRTY] PTE is reserved for
shadow stack. Copy-on-write PTEs have [R/O + _PAGE_COW].
When a PTE goes from [R/W + _PAGE_DIRTY] to [R/O + _PAGE_COW], it could
become a transient shadow stack PTE in two cases:
The first case is that some processors can start a write but end up seeing
a read-only PTE by the time they get to the Dirty bit, creating a transient
shadow stack PTE. However, this will not occur on processors supporting
Shadow Stack, therefore we don't need a TLB flush here.
Who's "we"?
The second case is that when the software, without atomic, tests & replaces
"... when _PAGE_DIRTY is replaced with _PAGE_COW non-atomically, a transient
shadow stack PTE can be created, as a result."
quoted hunk
_PAGE_DIRTY with _PAGE_COW, a transient shadow stack PTE can exist.
This is prevented with cmpxchg.
Dave Hansen, Jann Horn, Andy Lutomirski, and Peter Zijlstra provided many
insights to the issue. Jann Horn provided the cmpxchg solution.
Signed-off-by: Yu-cheng Yu <redacted>
Reviewed-by: Kees Cook <redacted>
---
arch/x86/include/asm/pgtable.h | 52 ++++++++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
and will not occur on
+ * processors supporting Shadow Stack. Without this guarantee, a
Which guarantee? That it won't happen on CPUs which support SHSTK?
+ * transition to a non-present PTE and flush the TLB would be
s/flush the TLB/TLB flush/
+ * needed.
+ *
+ * When changing a writable PTE to read-only and if the PTE has
+ * _PAGE_DIRTY set, move that bit to _PAGE_COW so that the PTE is
+ * not a shadow stack PTE.
+ */
This sentence doesn't belong here as it refers to what pte_wrprotect()
does. You could expand the comment in pte_wrprotect() with this here as
it is better.
+ if (cpu_feature_enabled(X86_FEATURE_SHSTK)) {
+ pte_t old_pte, new_pte;
+
+ do {
+ old_pte = READ_ONCE(*ptep);
+ new_pte = pte_wrprotect(old_pte);
Maybe I'm missing something but those two can happen outside of the
loop, no? Or is *ptep somehow changing concurrently while the loop is
doing the CMPXCHG and you need to recreate it each time?
IOW, you can generate upfront and do the empty loop...
quoted hunk
+
+ } while (!try_cmpxchg(&ptep->pte, &old_pte.pte, new_pte.pte));
+
+ return;
+ }
clear_bit(_PAGE_BIT_RW, (unsigned long *)&ptep->pte);
}
@@ -1282,6 +1308,32 @@ static inline pud_t pudp_huge_get_and_clear(struct mm_struct *mm, static inline void pmdp_set_wrprotect(struct mm_struct *mm, unsigned long addr, pmd_t *pmdp) {+ /*+ * Some processors can start a write, but end up seeing a read-only+ * PMD by the time they get to the Dirty bit. In this case, they+ * will set the Dirty bit, leaving a read-only, Dirty PMD which+ * looks like a Shadow Stack PMD.+ *+ * However, this behavior has been improved and will not occur on+ * processors supporting Shadow Stack. Without this guarantee, a+ * transition to a non-present PMD and flush the TLB would be+ * needed.+ *+ * When changing a writable PMD to read-only and if the PMD has+ * _PAGE_DIRTY set, move that bit to _PAGE_COW so that the PMD is+ * not a shadow stack PMD.+ */
Processors supporting Shadow Stack will not set a read-only PTE's dirty
bit. I will revise the comments.
quoted
and will not occur on
+ * processors supporting Shadow Stack. Without this guarantee, a
Which guarantee? That it won't happen on CPUs which support SHSTK?
Yes.
quoted
+ * transition to a non-present PTE and flush the TLB would be
s/flush the TLB/TLB flush/
quoted
+ * needed.
+ *
+ * When changing a writable PTE to read-only and if the PTE has
+ * _PAGE_DIRTY set, move that bit to _PAGE_COW so that the PTE is
+ * not a shadow stack PTE.
+ */
This sentence doesn't belong here as it refers to what pte_wrprotect()
does. You could expand the comment in pte_wrprotect() with this here as
it is better.
I will move this paragraph to pte_wrprotect().
quoted
+ if (cpu_feature_enabled(X86_FEATURE_SHSTK)) {
+ pte_t old_pte, new_pte;
+
+ do {
+ old_pte = READ_ONCE(*ptep);
+ new_pte = pte_wrprotect(old_pte);
Maybe I'm missing something but those two can happen outside of the
loop, no? Or is *ptep somehow changing concurrently while the loop is
doing the CMPXCHG and you need to recreate it each time?
IOW, you can generate upfront and do the empty loop...
*ptep can change concurrently.
quoted
+
+ } while (!try_cmpxchg(&ptep->pte, &old_pte.pte, new_pte.pte));
+
+ return;
+ }
clear_bit(_PAGE_BIT_RW, (unsigned long *)&ptep->pte);
}
On Mon, Jan 25, 2021 at 01:27:51PM -0800, Yu, Yu-cheng wrote:
quoted
Maybe I'm missing something but those two can happen outside of the
loop, no? Or is *ptep somehow changing concurrently while the loop is
doing the CMPXCHG and you need to recreate it each time?
IOW, you can generate upfront and do the empty loop...
On Mon, Jan 25, 2021 at 01:27:51PM -0800, Yu, Yu-cheng wrote:
quoted
quoted
Maybe I'm missing something but those two can happen outside of the
loop, no? Or is *ptep somehow changing concurrently while the loop is
doing the CMPXCHG and you need to recreate it each time?
IOW, you can generate upfront and do the empty loop...
*ptep can change concurrently.
Care to elaborate?
For example, when a thread reads a W=1, D=0 PTE and before changing it
to W=0,D=0, another thread could have written to the page and the PTE is
W=1, D=1 now. When try_cmpxchg() detects the difference, old_pte is
read again.
On Mon, Jan 25, 2021 at 02:18:37PM -0800, Yu, Yu-cheng wrote:
For example, when a thread reads a W=1, D=0 PTE and before changing it to
W=0,D=0, another thread could have written to the page and the PTE is W=1,
D=1 now. When try_cmpxchg() detects the difference, old_pte is read again.
None of that is mentioned in the comment above it and if anything,
*that* is what should be explained there - not some guarantee about some
processors which doesn't even apply here.
Also, add the fact that try_cmpxchg() will update old_pte with any
modified bits - D=1 for example - when it fails. As Peter just explained
to me on IRC.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Jan 25, 2021 at 02:18:37PM -0800, Yu, Yu-cheng wrote:
quoted
For example, when a thread reads a W=1, D=0 PTE and before changing it to
W=0,D=0, another thread could have written to the page and the PTE is W=1,
D=1 now. When try_cmpxchg() detects the difference, old_pte is read again.
None of that is mentioned in the comment above it and if anything,
*that* is what should be explained there - not some guarantee about some
processors which doesn't even apply here.
Also, add the fact that try_cmpxchg() will update old_pte with any
modified bits - D=1 for example - when it fails. As Peter just explained
to me on IRC.
Thx.
Maybe I'm missing something but those two can happen outside of the
loop, no? Or is *ptep somehow changing concurrently while the loop is
doing the CMPXCHG and you need to recreate it each time?
IOW, you can generate upfront and do the empty loop...
Empty loop would be wrong, but that wants to be written like:
old_pte = READ_ONCE(*ptep);
do {
new_pte = pte_wrprotect(old_pte);
} while (try_cmpxchg(&ptep->pte, &old_pte.pte, new_pte.pte));
Since try_cmpxchg() will update old_pte on failure.
Maybe I'm missing something but those two can happen outside of the
loop, no? Or is *ptep somehow changing concurrently while the loop is
doing the CMPXCHG and you need to recreate it each time?
IOW, you can generate upfront and do the empty loop...
Empty loop would be wrong, but that wants to be written like:
old_pte = READ_ONCE(*ptep);
do {
new_pte = pte_wrprotect(old_pte);
} while (try_cmpxchg(&ptep->pte, &old_pte.pte, new_pte.pte));
! went missing, too early, moar wake-up juice.
Since try_cmpxchg() will update old_pte on failure.
Maybe I'm missing something but those two can happen outside of the
loop, no? Or is *ptep somehow changing concurrently while the loop is
doing the CMPXCHG and you need to recreate it each time?
IOW, you can generate upfront and do the empty loop...
Empty loop would be wrong, but that wants to be written like:
old_pte = READ_ONCE(*ptep);
do {
new_pte = pte_wrprotect(old_pte);
} while (try_cmpxchg(&ptep->pte, &old_pte.pte, new_pte.pte));
! went missing, too early, moar wake-up juice.
quoted
Since try_cmpxchg() will update old_pte on failure.
A shadow stack PTE must be read-only and have _PAGE_DIRTY set. However,
read-only and Dirty PTEs also exist for copy-on-write (COW) pages. These
two cases are handled differently for page faults. Introduce VM_SHSTK to
track shadow stack VMAs.
Signed-off-by: Yu-cheng Yu <redacted>
Reviewed-by: Kees Cook <redacted>
---
arch/x86/mm/mmap.c | 2 ++
fs/proc/task_mmu.c | 3 +++
include/linux/mm.h | 8 ++++++++
3 files changed, 13 insertions(+)
Shadow stack accesses are those that are performed by the CPU where it
expects to encounter a shadow stack mapping. These accesses are performed
implicitly by CALL/RET at the site of the shadow stack pointer. These
accesses are made explicitly by shadow stack management instructions like
WRUSSQ.
Shadow stacks accesses to shadow-stack mapping can see faults in normal,
valid operation just like regular accesses to regular mappings. Shadow
stacks need some of the same features like delayed allocation, swap and
copy-on-write.
Shadow stack accesses can also result in errors, such as when a shadow
stack overflows, or if a shadow stack access occurs to a non-shadow-stack
mapping.
In handling a shadow stack page fault, verify it occurs within a shadow
stack mapping. It is always an error otherwise. For valid shadow stack
accesses, set FAULT_FLAG_WRITE to effect copy-on-write. Because clearing
_PAGE_DIRTY (vs. _PAGE_RW) is used to trigger the fault, shadow stack read
fault and shadow stack write fault are not differentiated and both are
handled as a write access.
Signed-off-by: Yu-cheng Yu <redacted>
Reviewed-by: Kees Cook <redacted>
---
arch/x86/include/asm/trap_pf.h | 2 ++
arch/x86/mm/fault.c | 19 +++++++++++++++++++
2 files changed, 21 insertions(+)
When serving a page fault, maybe_mkwrite() makes a PTE writable if its vma
has VM_WRITE.
A shadow stack vma has VM_SHSTK. Its PTEs have _PAGE_DIRTY, but not
_PAGE_WRITE. In fork(), _PAGE_DIRTY is cleared to effect copy-on-write,
and in page fault, _PAGE_DIRTY is restored and the shadow stack page is
writable again.
Update maybe_mkwrite() by introducing arch_maybe_mkwrite(), which sets
_PAGE_DIRTY for a shadow stack PTE.
Apply the same changes to maybe_pmd_mkwrite().
Signed-off-by: Yu-cheng Yu <redacted>
---
arch/x86/Kconfig | 4 ++++
arch/x86/mm/pgtable.c | 18 ++++++++++++++++++
include/linux/mm.h | 2 ++
include/linux/pgtable.h | 24 ++++++++++++++++++++++++
mm/huge_memory.c | 2 ++
5 files changed, 50 insertions(+)
When serving a page fault, maybe_mkwrite() makes a PTE writable if it is in
a writable vma. A shadow stack vma is writable, but its PTEs need
_PAGE_DIRTY to be set to become writable. For this reason, maybe_mkwrite()
has been updated.
There are a few places that call pte_mkwrite() directly, but effect the
same result as from maybe_mkwrite(). These sites need to be updated for
shadow stack as well. Thus, change them to maybe_mkwrite():
- do_anonymous_page() and migrate_vma_insert_page() check VM_WRITE directly
and call pte_mkwrite(), which is the same as maybe_mkwrite(). Change
them to maybe_mkwrite().
- In do_numa_page(), if the numa entry 'was-writable', then pte_mkwrite()
is called directly. Fix it by doing maybe_mkwrite().
- In change_pte_range(), pte_mkwrite() is called directly. Replace it with
maybe_mkwrite().
A shadow stack vma is writable but has different vma
flags, and handled accordingly in maybe_mkwrite().
Signed-off-by: Yu-cheng Yu <redacted>
---
mm/memory.c | 5 ++---
mm/migrate.c | 3 +--
mm/mprotect.c | 2 +-
3 files changed, 4 insertions(+), 6 deletions(-)
Can_follow_write_pte() ensures a read-only page is COWed by checking the
FOLL_COW flag, and uses pte_dirty() to validate the flag is still valid.
Like a writable data page, a shadow stack page is writable, and becomes
read-only during copy-on-write, but it is always dirty. Thus, in the
can_follow_write_pte() check, it belongs to the writable page case and
should be excluded from the read-only page pte_dirty() check. Apply
the same changes to can_follow_write_pmd().
Signed-off-by: Yu-cheng Yu <redacted>
---
mm/gup.c | 8 +++++---
mm/huge_memory.c | 8 +++++---
2 files changed, 10 insertions(+), 6 deletions(-)
@@ -536,6 +537,10 @@ struct thread_struct {unsignedintsig_on_uaccess_err:1;+#ifdef CONFIG_X86_CET_USER+structcet_statuscet;+#endif+/* Floating point and extended processor state */structfpufpu;/*
To deliver a signal, create a shadow stack restore token and put a restore
token and the signal restorer address on the shadow stack. For sigreturn,
verify the token and restore the shadow stack pointer.
Introduce WRUSS, which is a kernel-mode instruction but writes directly to
user shadow stack. It is used to construct the user signal stack as
described above.
Introduce a signal context extension struct 'sc_ext', which is used to save
shadow stack restore token address and WAIT_ENDBR status. WAIT_ENDBR will
be introduced later in the Indirect Branch Tracking (IBT) series, but add
that into sc_ext now to keep the struct stable in case the IBT series is
applied later.
Signed-off-by: Yu-cheng Yu <redacted>
---
arch/x86/ia32/ia32_signal.c | 17 +++
arch/x86/include/asm/cet.h | 8 ++
arch/x86/include/asm/fpu/internal.h | 10 ++
arch/x86/include/asm/special_insns.h | 32 ++++++
arch/x86/include/uapi/asm/sigcontext.h | 9 ++
arch/x86/kernel/cet.c | 152 +++++++++++++++++++++++++
arch/x86/kernel/fpu/signal.c | 100 ++++++++++++++++
arch/x86/kernel/signal.c | 10 ++
8 files changed, 338 insertions(+)
@@ -196,6 +196,15 @@ struct _xstate {/* New processor state extensions go here: */};+/*+*Locatedattheendofsigcontext->fpstate,alignedto8.+*/+structsc_ext{+unsignedlongtotal_size;+unsignedlongssp;+unsignedlongwait_endbr;+};+/**The32-bitsignalframe:*/
@@ -72,6 +74,80 @@ static unsigned long alloc_shstk(unsigned long size, int flags)returnaddr;}+#define TOKEN_MODE_MASK 3UL+#define TOKEN_MODE_64 1UL+#define IS_TOKEN_64(token) (((token) & TOKEN_MODE_MASK) == TOKEN_MODE_64)+#define IS_TOKEN_32(token) (((token) & TOKEN_MODE_MASK) == 0)++/*+*Verifytherestoretokenattheaddressof'ssp'is+*validandthensetshadowstackpointeraccordingtothe+*token.+*/+intcet_verify_rstor_token(boolia32,unsignedlongssp,+unsignedlong*new_ssp)+{+unsignedlongtoken;++*new_ssp=0;++if(!IS_ALIGNED(ssp,8))+return-EINVAL;++if(get_user(token,(unsignedlong__user*)ssp))+return-EFAULT;++/* Is 64-bit mode flag correct? */+if(!ia32&&!IS_TOKEN_64(token))+return-EINVAL;+elseif(ia32&&!IS_TOKEN_32(token))+return-EINVAL;++token&=~TOKEN_MODE_MASK;++/*+*Restoreaddressproperlyaligned?+*/+if((!ia32&&!IS_ALIGNED(token,8))||!IS_ALIGNED(token,4))+return-EINVAL;++/*+*Tokenwasplacedproperly?+*/+if(((ALIGN_DOWN(token,8)-8)!=ssp)||(token>=TASK_SIZE_MAX))+return-EINVAL;++*new_ssp=token;+return0;+}++/*+*Createarestoretokenontheshadowstack.+*Atokenisalways8-byteandalignedto8.+*/+staticintcreate_rstor_token(boolia32,unsignedlongssp,+unsignedlong*new_ssp)+{+unsignedlongaddr;++*new_ssp=0;++if((!ia32&&!IS_ALIGNED(ssp,8))||!IS_ALIGNED(ssp,4))+return-EINVAL;++addr=ALIGN_DOWN(ssp,8)-8;++/* Is the token for 64-bit? */+if(!ia32)+ssp|=TOKEN_MODE_64;++if(write_user_shstk_64(addr,ssp))+return-EFAULT;++*new_ssp=addr;+return0;+}+intcet_setup_shstk(void){unsignedlongaddr,size;