On Tue, May 10, 2022 at 07:23:46PM +1000, Nicholas Piggin wrote:
Excerpts from Xiu Jianfeng's message of May 5, 2022 9:19 pm:Agreed! :)
Add support for adding a random offset to the stack while handlingHey, very nice.
syscalls. This patch uses mftb() instead of get_random_int() for better
performance.
I welcome it being used for any userspace controllable entry to the[...]This looks like the right place. I wonder why other interrupts don't
@@ -82,6 +83,7 @@ notrace long system_call_exception(long r3, long r4, long r5,
kuap_lock();
+ add_random_kstack_offset();
regs->orig_gpr3 = r3;
if (IS_ENABLED(CONFIG_PPC_IRQ_SOFT_MASK_DEBUG))
get the same treatment. Userspace can induce the kernel to take a
synchronous interrupt, or wait for async ones. Smaller surface area
maybe but certain instruction emulation for example could result in
significant logic that depends on user state. Anyway that's for
hardening gurus to ponder.
kernel! :)
Also, related, have you validated the result using the LKDTM test?
See tools/testing/selftests/lkdtm/stack-entropy.sh
How does this read? I can send a "real" patch if it looks good:@@ -405,6 +407,7 @@ interrupt_exit_user_prepare_main(unsigned long ret, struct pt_regs *regs)So this seems to be what x86 and s390 do, but why are we choosing a
/* Restore user access locks last */
kuap_user_restore(regs);
+ choose_random_kstack_offset(mftb() & 0xFF);
return ret;
}
new offset for every interrupt when it's only used on a syscall?
I would rather you do what arm64 does and just choose the offset
at the end of system_call_exception.
I wonder why the choose is separated from the add? I guess it's to
avoid a data dependency for stack access on an expensive random
function, so that makes sense (a comment would be nice in the
generic code).
diff --git a/include/linux/randomize_kstack.h b/include/linux/randomize_kstack.h
index 1468caf001c0..ad3e80275c74 100644
--- a/include/linux/randomize_kstack.h
+++ b/include/linux/randomize_kstack.h
@@ -40,8 +40,11 @@ DECLARE_PER_CPU(u32, kstack_offset);
*/
#define KSTACK_OFFSET_MAX(x) ((x) & 0x3FF)
-/*
- * These macros must be used during syscall entry when interrupts and
+/**
+ * add_random_kstack_offset - Increase stack utilization by previously
+ * chosen random offset
+ *
+ * This should be used in the syscall entry path when interrupts and
* preempt are disabled, and after user registers have been stored to
* the stack.
*/
@@ -55,6 +58,24 @@ DECLARE_PER_CPU(u32, kstack_offset);
} \
} while (0)
+/**
+ * choose_random_kstack_offset - Choose the random offsset for the next
+ * add_random_kstack_offset()
+ *
+ * This should only be used during syscall exit when interrupts and
+ * preempt are disabled, and before user registers have been restored
+ * from the stack. This is done to frustrate attack attempts from
+ * userspace to learn the offset:
+ * - Maximize the timing uncertainty visible from userspace: if the
+ * the offset is chosen at syscall entry, userspace has much more
+ * control over the timing between chosen offsets. "How long will we
+ * be in kernel mode?" tends to be more difficult to know than "how
+ * long will be be in user mode?"
+ * - Reduce the lifetime of the new offset sitting in memory during
+ * kernel mode execution. Exposures of "thread-local" (e.g. current,
+ * percpu, etc) memory contents tends to be easier than arbitrary
+ * location memory exposures.
+ */
#define choose_random_kstack_offset(rand) do { \
if (static_branch_maybe(CONFIG_RANDOMIZE_KSTACK_OFFSET_DEFAULT, \
&randomize_kstack_offset)) { \