Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

RFC.  I don't know if this is something we'd want to keep or not.

Getting extable handling working for test_nx_data is proving tricky, and while
I can't spot anything that should stop the extable from working with NX
faults, from a security hardening perspective, there really ought to
be.

(Spurious faults aside), there are no circumstances where an NX fault is
legitimate, and restricting extable's ability to interfere with the fatality
of an NX fault provides a better security posture.
---
 xen/arch/x86/setup.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 3bbc46f244b9..7cb530a7528f 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -668,6 +668,45 @@ static void noreturn init_done(void)
                         (unsigned long)&__2M_rodata_end,
                         PAGE_HYPERVISOR_RO);
 
+    if ( IS_ENABLED(CONFIG_DEBUG) )
+    {
+        static const char test_rodata = 1;
+        static char __ro_after_init test_ro_after_init = 1;
+
+#define PROBE(insn, c, p)                       \
+    ({                                          \
+        bool fault = 0;                         \
+        asm ( "1:" insn "[ptr]\n\t"             \
+              "2:\n\t"                          \
+              ".section .fixup,\"ax\"\n\t"      \
+              "3: movb $1, %[fault]\n\t"        \
+              "jmp 2b\n\t"                      \
+              ".previous"                       \
+              _ASM_EXTABLE(1b, 3b)              \
+              : [fault] "+r" (fault)            \
+              : [ptr] c (p)                     \
+            );                                  \
+        fault;                                  \
+    })
+
+        if ( !PROBE("notb %", "m", test_rodata) )
+            panic("No fault from test_rodata\n");
+
+        if ( !PROBE("notb %", "m", test_ro_after_init) )
+            panic("No fault from test_ro_after_init\n");
+
+        if ( !PROBE("notb %", "m", init_done) )
+            panic("No fault from modifying init_done\n");
+
+        if ( 0 /* RFC */ && cpu_has_nx )
+        {
+            static char test_nx_data[1] = { 0xc3 };
+
+            if ( !PROBE("call %c", "i", test_nx_data) )
+                panic("No fault from test_nx_data\n");
+        }
+    }
+
     startup_cpu_idle_loop();
 }
 
-- 
2.11.0

On 30.11.2021 11:04, Andrew Cooper wrote:
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> 
> RFC.  I don't know if this is something we'd want to keep or not.
> 
> Getting extable handling working for test_nx_data is proving tricky, and while
> I can't spot anything that should stop the extable from working with NX
> faults, from a security hardening perspective, there really ought to
> be.
> 
> (Spurious faults aside), there are no circumstances where an NX fault is
> legitimate, and restricting extable's ability to interfere with the fatality
> of an NX fault provides a better security posture.

Gating the extable_fixup() invocation accordingly should be possible.
A respective check could live even earlier, but the window between
the !guest_mode() check and the function's invocation isn't very large
anyway.

Since we can't have both testability and such faults being uniformly
fatal, but since otoh we use pre_extable quite sparingly, how about
forcing the fixup to take that path by disabling interrupts around
the test?

In any event this touches the insufficient selectiveness of the fixup
machinery again: Any kind of fault will be recovered from whenever a
fixup record is attached to an insn.

Jan

On 02/12/2021 13:33, Jan Beulich wrote:
> On 30.11.2021 11:04, Andrew Cooper wrote:
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>>
>> RFC.  I don't know if this is something we'd want to keep or not.
>>
>> Getting extable handling working for test_nx_data is proving tricky, and while
>> I can't spot anything that should stop the extable from working with NX
>> faults, from a security hardening perspective, there really ought to
>> be.
>>
>> (Spurious faults aside), there are no circumstances where an NX fault is
>> legitimate, and restricting extable's ability to interfere with the fatality
>> of an NX fault provides a better security posture.
> Gating the extable_fixup() invocation accordingly should be possible.
> A respective check could live even earlier, but the window between
> the !guest_mode() check and the function's invocation isn't very large
> anyway.
>
> Since we can't have both testability and such faults being uniformly
> fatal, but since otoh we use pre_extable quite sparingly, how about
> forcing the fixup to take that path by disabling interrupts around
> the test?

That feels like an abuse of an unrelated mechanism, not to mention fragile.

> In any event this touches the insufficient selectiveness of the fixup
> machinery again: Any kind of fault will be recovered from whenever a
> fixup record is attached to an insn.

There are multiple things here.  Yes, I agree that fixing up all faults
is suboptimal.

But even within #PF alone, things such as SMAP/SMEP faults are
programmer error with no hope of extable being able to adequately
resolve, and should be fatal.

I actually like the approach that Linux has recently taken, by
describing a fixup type rather than arbitrary logic, in an attempt to
keep the number of special cases from getting out of hand.  This
approach is quite easy to filter into specific exceptions.

~Andrew