| Message ID | 20211130100445.31156-9-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Support for __ro_after_init | expand |
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
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(); }
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(+)