lkrg-org / lkrg

Linux Kernel Runtime Guard
https://lkrg.org
Other
410 stars 72 forks source link

Heavy pCFI unnecessarily skipped on systems with ORC #210

Closed solardiz closed 2 years ago

solardiz commented 2 years ago

Per discussion with @Adam-pi3 and testing on AlmaLinux 8.5 (see https://github.com/lkrg-org/lkrg/pull/202#issuecomment-1190588572), we need this:

+++ b/src/modules/exploit_detection/p_exploit_detection.c
@@ -1630,7 +1630,9 @@ int p_ed_enforce_pcfi(struct task_struct *p_task, struct p_ed_process *p_orig, s
    unsigned int i = 0;
 //   unsigned long p_flags;
    struct stack_trace p_trace;
+#ifndef CONFIG_UNWINDER_ORC
    const void *p_fp = (const void *)p_regs_get_fp(p_regs);
+#endif
 #ifdef CONFIG_X86
 #if defined(CONFIG_UNWINDER_ORC)
    struct unwind_state p_state;
@@ -1672,10 +1674,12 @@ int p_ed_enforce_pcfi(struct task_struct *p_task, struct p_ed_process *p_orig, s
       }
    }

+#ifndef CONFIG_UNWINDER_ORC
    if (!p_is_obj_on_stack(p_task, p_fp)) {
       p_debug_log(P_LOG_DEBUG, "Frame pointer is not on the stack, so CFI is not enforced");
       goto p_ed_enforce_pcfi_out;
    }
+#endif

    while ( (p_trace.entries = p_ed_pcfi_alloc()) == NULL); // Should never be NULL

The issue had been overlooked because of a bug in the logging. Prior to recent changes, we had:

      p_debug_log(P_LKRG_WARN,
                  "Frame pointer is NOT on the stack - CFI is not enforced :(\n");

where p_debug_log and P_LKRG_WARN were inconsistent, so this message was always suppressed.

When reworking the log messages, I temporarily set this message to DEBUG, even though a direct translation of WARN to new conventions would have been ISSUE. I did that to avoid log flood. Once we fix this issue, we should probably set this message's severity to ISSUE (and not forget to change p_debug_log to p_print_log).

solardiz commented 2 years ago

@Adam-pi3 Is this "Frame pointer is not on the stack" condition possibly unexpected and problematic enough in the non-ORC builds that we should treat it as a security violation? I wonder if it's right to treat it as an ISSUE (literal translation of the former WARN that you had chosen for it) or maybe treat it as WATCH or DEBUG if it's expected to sometimes occur without an attack or maybe upgrade it to ALERT if it's only expected to be seen when there's an attack. What do you think?

solardiz commented 2 years ago

I temporarily upgraded the message to FAULT for testing by CI in my fork - so far, 16 out of 18 checks successful, 2 pending - so it looks like this message doesn't normally appear. (Incidentally, somehow the 19th check - CodeQL - does not run in my fork.)

solardiz commented 2 years ago

All 18 have passed. So, what do we set the severity of this message to? If it's ALERT, I assume we'll need to trigger pCFI enforcement on it. Let's not have alerts without possibility of enforcement.