kernel: introduce uaccess logging

In an upcoming change we are going to add uaccess instrumentation that uses inline access to struct task_struct from the instrumentation routines. Because instrumentation.h is included from many places including (recursively) from sched.h this would otherwise lead to a circular dependency. Break the dependency by moving uaccess instrumentation routines into a separate header, instrumentation-uaccess.h.

Link https://linux-review.googlesource.com/id/I625728db0c8db374e13e4ebc54985ac5c79ace7d

Signed-off-by Peter Collingbourne <[email protected]>

Acked-by Dmitry Vyukov <[email protected]>

---
 include/linux/instrumented-uaccess.h | 49 ++++++++++++++++++++++++++++
 include/linux/instrumented.h         | 34 -------------------
 include/linux/uaccess.h              |  2 +-
 lib/iov_iter.c                       |  2 +-
 lib/usercopy.c                       |  2 +-
 5 files changed, 52 insertions(+), 37 deletions(-)
 create mode 100644 include/linux/instrumented-uaccess.h

include/linux/instrumented-uaccess.h new file +49 -0

new file mode 100644
index 000000000000..ece549088e50
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * This header provides generic wrappers for memory access instrumentation for
+ * uaccess routines that the compiler cannot emit for: KASAN, KCSAN.
+ */
+#ifndef _LINUX_INSTRUMENTED_UACCESS_H
+#define _LINUX_INSTRUMENTED_UACCESS_H
+
+#include <linux/compiler.h>
+#include <linux/kasan-checks.h>
+#include <linux/kcsan-checks.h>
+#include <linux/types.h>
+
+/**
+ * instrument_copy_to_user - instrument reads of copy_to_user
+ *
+ * Instrument reads from kernel memory, that are due to copy_to_user (and
+ * variants). The instrumentation must be inserted before the accesses.
+ *
+ * @to destination address
+ * @from source address
+ * @n number of bytes to copy
+ */
+static __always_inline void
+instrument_copy_to_user(void __user *to, const void *from, unsigned long n)
+{
+	kasan_check_read(from, n);
+	kcsan_check_read(from, n);
+}
+
+/**
+ * instrument_copy_from_user - instrument writes of copy_from_user
+ *
+ * Instrument writes to kernel memory, that are due to copy_from_user (and
+ * variants). The instrumentation should be inserted before the accesses.
+ *
+ * @to destination address
+ * @from source address
+ * @n number of bytes to copy
+ */
+static __always_inline void
+instrument_copy_from_user(const void *to, const void __user *from, unsigned long n)
+{
+	kasan_check_write(to, n);
+	kcsan_check_write(to, n);
+}
+
+#endif /* _LINUX_INSTRUMENTED_UACCESS_H */

include/linux/instrumented.h +0 -34

index 42faebbaa202..b68f415510c7 100644
@@ -102,38 +102,4 @@ static __always_inline void instrument_atomic_read_write(const volatile void *v,
 	kcsan_check_atomic_read_write(v, size);
 }
 
-/**
- * instrument_copy_to_user - instrument reads of copy_to_user
- *
- * Instrument reads from kernel memory, that are due to copy_to_user (and
- * variants). The instrumentation must be inserted before the accesses.
- *
- * @to destination address
- * @from source address
- * @n number of bytes to copy
- */
-static __always_inline void
-instrument_copy_to_user(void __user *to, const void *from, unsigned long n)
-{
-	kasan_check_read(from, n);
-	kcsan_check_read(from, n);
-}
-
-/**
- * instrument_copy_from_user - instrument writes of copy_from_user
- *
- * Instrument writes to kernel memory, that are due to copy_from_user (and
- * variants). The instrumentation should be inserted before the accesses.
- *
- * @to destination address
- * @from source address
- * @n number of bytes to copy
- */
-static __always_inline void
-instrument_copy_from_user(const void *to, const void __user *from, unsigned long n)
-{
-	kasan_check_write(to, n);
-	kcsan_check_write(to, n);
-}
-
 #endif /* _LINUX_INSTRUMENTED_H */

include/linux/uaccess.h +1 -1

index ac0394087f7d..c0c467e39657 100644
@@ -3,7 +3,7 @@
 #define __LINUX_UACCESS_H__
 
 #include <linux/fault-inject-usercopy.h>
-#include <linux/instrumented.h>
+#include <linux/instrumented-uaccess.h>
 #include <linux/minmax.h>
 #include <linux/sched.h>
 #include <linux/thread_info.h>

lib/iov_iter.c +1 -1

index 66a740e6e153..3f9dc6df7102 100644
@@ -12,7 +12,7 @@
 #include <linux/compat.h>
 #include <net/checksum.h>
 #include <linux/scatterlist.h>
-#include <linux/instrumented.h>
+#include <linux/instrumented-uaccess.h>
 
 #define PIPE_PARANOIA /* for now */
 

lib/usercopy.c +1 -1

index 7413dd300516..1cd188e62d06 100644
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/bitops.h>
 #include <linux/fault-inject-usercopy.h>
-#include <linux/instrumented.h>
+#include <linux/instrumented-uaccess.h>
 #include <linux/uaccess.h>
 
 /* out-of-line parts */

Signature · 1 line

2.34.1.173.g76aa8bc2d0-goog

Add the core code to support uaccess logging. Subsequent patches will hook this up to the arch-specific kernel entry and exit code for certain architectures.

Link https://linux-review.googlesource.com/id/I6581765646501a5631b281d670903945ebadc57d

Signed-off-by Peter Collingbourne <[email protected]>

---
v4:
- add CONFIG_UACCESS_BUFFER
- add kernel doc comments to uaccess-buffer.h
- outline uaccess_buffer_set_descriptor_addr_addr
- switch to using spin_lock_irqsave/spin_unlock_irqrestore during pre/post-exit-loop code because preemption is disabled at that point
- set kend to NULL if krealloc failed
- size_t -> unsigned long in copy_from_user_nolog signature

v3:
- performance optimizations for entry/exit code
- don't use kcur == NULL to mean overflow
- fix potential double free in clone()
- don't allocate a new kernel-side uaccess buffer for each syscall
- fix uaccess buffer leak on exit
- fix some sparse warnings

v2:
- New interface that avoids multiple syscalls per real syscall and is arch-generic
- Avoid logging uaccesses done by BPF programs
- Add documentation
- Split up into multiple patches
- Various code moves, renames etc as requested by Marco

 arch/Kconfig                         |  13 +++
 fs/exec.c                            |   3 +
 include/linux/instrumented-uaccess.h |   6 +-
 include/linux/sched.h                |   5 +
 include/linux/uaccess-buffer-info.h  |  46 ++++++++
 include/linux/uaccess-buffer.h       | 152 +++++++++++++++++++++++++++
 include/uapi/linux/prctl.h           |   3 +
 include/uapi/linux/uaccess-buffer.h  |  27 +++++
 kernel/Makefile                      |   1 +
 kernel/bpf/helpers.c                 |   7 +-
 kernel/fork.c                        |   4 +
 kernel/signal.c                      |   9 +-
 kernel/sys.c                         |   6 ++
 kernel/uaccess-buffer.c              | 145 +++++++++++++++++++++++++
 14 files changed, 422 insertions(+), 5 deletions(-)
 create mode 100644 include/linux/uaccess-buffer-info.h
 create mode 100644 include/linux/uaccess-buffer.h
 create mode 100644 include/uapi/linux/uaccess-buffer.h
 create mode 100644 kernel/uaccess-buffer.c

arch/Kconfig +13 -0

index d3c4ab249e9c..17819f53ea80 100644
@@ -1312,6 +1312,19 @@ config ARCH_HAS_PARANOID_L1D_FLUSH
 config DYNAMIC_SIGFRAME
        bool
 
+config HAVE_ARCH_UACCESS_BUFFER
+       bool
+       help
+         Select if the architecture's syscall entry/exit code supports uaccess buffers.
+
+config UACCESS_BUFFER
+       bool "Uaccess logging" if EXPERT
+       default y
+       depends on HAVE_ARCH_UACCESS_BUFFER
+       help
+         Select to enable support for uaccess logging
+         (see Documentation/admin-guide/uaccess-logging.rst).
+
 source "kernel/gcov/Kconfig"
 
 source "scripts/gcc-plugins/Kconfig"

fs/exec.c +3 -0

index 537d92c41105..c9975e790f30 100644
@@ -65,6 +65,7 @@
 #include <linux/vmalloc.h>
 #include <linux/io_uring.h>
 #include <linux/syscall_user_dispatch.h>
+#include <linux/uaccess-buffer.h>
 
 #include <linux/uaccess.h>
 #include <asm/mmu_context.h>

@@ -1313,6 +1314,8 @@ int begin_new_exec(struct linux_binprm * bprm)
        me->personality &= ~bprm->per_clear;
 
        clear_syscall_work_syscall_user_dispatch(me);
+       uaccess_buffer_set_descriptor_addr_addr(0);
+       uaccess_buffer_free(current);
 
        /*
         * We have to apply CLOEXEC before we change whether the process is

include/linux/instrumented-uaccess.h +5 -1

index ece549088e50..b967f4436d15 100644
@@ -2,7 +2,8 @@
 
 /*
  * This header provides generic wrappers for memory access instrumentation for
- * uaccess routines that the compiler cannot emit for: KASAN, KCSAN.
+ * uaccess routines that the compiler cannot emit for: KASAN, KCSAN,
+ * uaccess buffers.
  */
 #ifndef _LINUX_INSTRUMENTED_UACCESS_H
 #define _LINUX_INSTRUMENTED_UACCESS_H

@@ -11,6 +12,7 @@
 #include <linux/kasan-checks.h>
 #include <linux/kcsan-checks.h>
 #include <linux/types.h>
+#include <linux/uaccess-buffer.h>
 
 /**
  * instrument_copy_to_user - instrument reads of copy_to_user

@@ -27,6 +29,7 @@ instrument_copy_to_user(void __user *to, const void *from, unsigned long n)
 {
        kasan_check_read(from, n);
        kcsan_check_read(from, n);
+       uaccess_buffer_log_write(to, n);
 }
 
 /**

@@ -44,6 +47,7 @@ instrument_copy_from_user(const void *to, const void __user *from, unsigned long
 {
        kasan_check_write(to, n);
        kcsan_check_write(to, n);
+       uaccess_buffer_log_read(from, n);
 }
 
 #endif /* _LINUX_INSTRUMENTED_UACCESS_H */

include/linux/sched.h +5 -0

index 78c351e35fec..96014dd2702e 100644
@@ -34,6 +34,7 @@
 #include <linux/rseq.h>
 #include <linux/seqlock.h>
 #include <linux/kcsan.h>
+#include <linux/uaccess-buffer-info.h>
 #include <asm/kmap_size.h>
 
 /* task_struct member predeclarations (sorted alphabetically): */

@@ -1484,6 +1485,10 @@ struct task_struct {
        struct callback_head            l1d_flush_kill;
 #endif
 
+#ifdef CONFIG_UACCESS_BUFFER
+       struct uaccess_buffer_info      uaccess_buffer;
+#endif
+
        /*
         * New fields for task_struct should be added above here, so that
         * they are included in the randomized portion of task_struct.

include/linux/uaccess-buffer-info.h new file +46 -0

new file mode 100644
index 000000000000..46e2b1a4a20f
@@ -0,0 +1,46 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_UACCESS_BUFFER_INFO_H
+#define _LINUX_UACCESS_BUFFER_INFO_H
+
+#ifdef CONFIG_UACCESS_BUFFER
+
+struct uaccess_buffer_info {
+       /*
+        * The pointer to pointer to struct uaccess_descriptor. This is the
+        * value controlled by prctl(PR_SET_UACCESS_DESCRIPTOR_ADDR_ADDR).
+        */
+       struct uaccess_descriptor __user *__user *desc_ptr_ptr;
+
+       /*
+        * The pointer to struct uaccess_descriptor read at syscall entry time.
+        */
+       struct uaccess_descriptor __user *desc_ptr;
+
+       /*
+        * A pointer to the kernel's temporary copy of the uaccess log for the
+        * current syscall. We log to a kernel buffer in order to avoid leaking
+        * timing information to userspace.
+        */
+       struct uaccess_buffer_entry *kbegin;
+
+       /*
+        * The position of the next uaccess buffer entry for the current
+        * syscall, or NULL if we are not logging the current syscall.
+        */
+       struct uaccess_buffer_entry *kcur;
+
+       /*
+        * A pointer to the end of the kernel's uaccess log.
+        */
+       struct uaccess_buffer_entry *kend;
+
+       /*
+        * The pointer to the userspace uaccess log, as read from the
+        * struct uaccess_descriptor.
+        */
+       struct uaccess_buffer_entry __user *ubegin;
+};
+
+#endif
+
+#endif  /* _LINUX_UACCESS_BUFFER_INFO_H */

include/linux/uaccess-buffer.h new file +152 -0

new file mode 100644
index 000000000000..2e9b4010fb59
@@ -0,0 +1,152 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_UACCESS_BUFFER_H
+#define _LINUX_UACCESS_BUFFER_H
+
+#include <linux/sched.h>
+#include <uapi/linux/uaccess-buffer.h>
+
+#include <asm-generic/errno-base.h>
+
+#ifdef CONFIG_UACCESS_BUFFER
+
+/*
+ * uaccess_buffer_maybe_blocked - returns whether a task potentially has signals
+ * blocked due to uaccess logging
+ * @tsk: the task.
+ */
+static inline bool uaccess_buffer_maybe_blocked(struct task_struct *tsk)
+{
+       return test_task_syscall_work(tsk, UACCESS_BUFFER_ENTRY);
+}
+
+void __uaccess_buffer_syscall_entry(void);
+/*
+ * uaccess_buffer_syscall_entry - hook to be run before syscall entry
+ */
+static inline void uaccess_buffer_syscall_entry(void)
+{
+       __uaccess_buffer_syscall_entry();
+}
+
+void __uaccess_buffer_syscall_exit(void);
+/*
+ * uaccess_buffer_syscall_exit - hook to be run after syscall exit
+ */
+static inline void uaccess_buffer_syscall_exit(void)
+{
+       __uaccess_buffer_syscall_exit();
+}
+
+bool __uaccess_buffer_pre_exit_loop(void);
+/*
+ * uaccess_buffer_pre_exit_loop - hook to be run immediately before the
+ * pre-kernel-exit loop that handles signals, tracing etc. Returns a bool to
+ * be passed to uaccess_buffer_post_exit_loop.
+ */
+static inline bool uaccess_buffer_pre_exit_loop(void)
+{
+       if (!test_syscall_work(UACCESS_BUFFER_ENTRY))
+               return false;
+       return __uaccess_buffer_pre_exit_loop();
+}
+
+void __uaccess_buffer_post_exit_loop(void);
+/*
+ * uaccess_buffer_post_exit_loop - hook to be run immediately after the
+ * pre-kernel-exit loop that handles signals, tracing etc.
+ * @pending: the bool returned from uaccess_buffer_pre_exit_loop.
+ */
+static inline void uaccess_buffer_post_exit_loop(bool pending)
+{
+       if (pending)
+               __uaccess_buffer_post_exit_loop();
+}
+
+/*
+ * uaccess_buffer_set_descriptor_addr_addr - implements
+ * prctl(PR_SET_UACCESS_DESCRIPTOR_ADDR_ADDR).
+ */
+int uaccess_buffer_set_descriptor_addr_addr(unsigned long addr);
+
+/*
+ * copy_from_user_nolog - a variant of copy_from_user that avoids uaccess
+ * logging. This is useful in special cases, such as when the kernel overreads a
+ * buffer.
+ * @to: the pointer to kernel memory.
+ * @from: the pointer to user memory.
+ * @len: the number of bytes to copy.
+ */
+unsigned long copy_from_user_nolog(void *to, const void __user *from,
+                                  unsigned long len);
+
+/*
+ * uaccess_buffer_free - free the task's kernel-side uaccess buffer and arrange
+ * for uaccess logging to be cancelled for the current syscall
+ * @tsk: the task.
+ */
+void uaccess_buffer_free(struct task_struct *tsk);
+
+void __uaccess_buffer_log_read(const void __user *from, unsigned long n);
+/*
+ * uaccess_buffer_log_read - log a read access
+ * @from: the address of the access.
+ * @n: the number of bytes.
+ */
+static inline void uaccess_buffer_log_read(const void __user *from, unsigned long n)
+{
+       if (unlikely(test_syscall_work(UACCESS_BUFFER_EXIT)))
+               __uaccess_buffer_log_read(from, n);
+}
+
+void __uaccess_buffer_log_write(void __user *to, unsigned long n);
+/*
+ * uaccess_buffer_log_write - log a write access
+ * @to: the address of the access.
+ * @n: the number of bytes.
+ */
+static inline void uaccess_buffer_log_write(void __user *to, unsigned long n)
+{
+       if (unlikely(test_syscall_work(UACCESS_BUFFER_EXIT)))
+               __uaccess_buffer_log_write(to, n);
+}
+
+#else
+
+static inline bool uaccess_buffer_maybe_blocked(struct task_struct *tsk)
+{
+       return false;
+}
+static inline void uaccess_buffer_syscall_entry(void)
+{
+}
+static inline void uaccess_buffer_syscall_exit(void)
+{
+}
+static inline bool uaccess_buffer_pre_exit_loop(void)
+{
+       return false;
+}
+static inline void uaccess_buffer_post_exit_loop(bool pending)
+{
+}
+static inline int uaccess_buffer_set_descriptor_addr_addr(unsigned long addr)
+{
+       return -EINVAL;
+}
+static inline void uaccess_buffer_free(struct task_struct *tsk)
+{
+}
+
+#define copy_from_user_nolog copy_from_user
+
+static inline void uaccess_buffer_log_read(const void __user *from,
+                                          unsigned long n)
+{
+}
+static inline void uaccess_buffer_log_write(void __user *to, unsigned long n)
+{
+}
+
+#endif
+
+#endif  /* _LINUX_UACCESS_BUFFER_H */

include/uapi/linux/prctl.h +3 -0

index bb73e9a0b24f..74b37469c7b3 100644
@@ -272,4 +272,7 @@ struct prctl_mm_map {
 # define PR_SCHED_CORE_SCOPE_THREAD_GROUP      1
 # define PR_SCHED_CORE_SCOPE_PROCESS_GROUP     2
 
+/* Configure uaccess logging feature */
+#define PR_SET_UACCESS_DESCRIPTOR_ADDR_ADDR    63
+
 #endif /* _LINUX_PRCTL_H */

include/uapi/linux/uaccess-buffer.h new file +27 -0

new file mode 100644
index 000000000000..bf10f7c78857
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_LINUX_UACCESS_BUFFER_H
+#define _UAPI_LINUX_UACCESS_BUFFER_H
+
+#include <linux/types.h>
+
+/* Location of the uaccess log. */
+struct uaccess_descriptor {
+       /* Address of the uaccess_buffer_entry array. */
+       __u64 addr;
+       /* Size of the uaccess_buffer_entry array in number of elements. */
+       __u64 size;
+};
+
+/* Format of the entries in the uaccess log. */
+struct uaccess_buffer_entry {
+       /* Address being accessed. */
+       __u64 addr;
+       /* Number of bytes that were accessed. */
+       __u64 size;
+       /* UACCESS_BUFFER_* flags. */
+       __u64 flags;
+};
+
+#define UACCESS_BUFFER_FLAG_WRITE      1 /* access was a write */
+
+#endif /* _UAPI_LINUX_UACCESS_BUFFER_H */

kernel/Makefile +1 -0

index 186c49582f45..e5f6c56696a2 100644
@@ -114,6 +114,7 @@ obj-$(CONFIG_KCSAN) += kcsan/
 obj-$(CONFIG_SHADOW_CALL_STACK) += scs.o
 obj-$(CONFIG_HAVE_STATIC_CALL_INLINE) += static_call.o
 obj-$(CONFIG_CFI_CLANG) += cfi.o
+obj-$(CONFIG_UACCESS_BUFFER) += uaccess-buffer.o
 
 obj-$(CONFIG_PERF_EVENTS) += events/
 

kernel/bpf/helpers.c +6 -1

index 649f07623df6..ab6520a633ef 100644
@@ -15,6 +15,7 @@
 #include <linux/pid_namespace.h>
 #include <linux/proc_ns.h>
 #include <linux/security.h>
+#include <linux/uaccess-buffer.h>
 
 #include "../../lib/kstrtox.h"
 

@@ -637,7 +638,11 @@ const struct bpf_func_proto bpf_event_output_data_proto =  {
 BPF_CALL_3(bpf_copy_from_user, void *, dst, u32, size,
           const void __user *, user_ptr)
 {
-       int ret = copy_from_user(dst, user_ptr, size);
+       /*
+        * Avoid logging uaccesses here as the BPF program may not be following
+        * the uaccess log rules.
+        */
+       int ret = copy_from_user_nolog(dst, user_ptr, size);
 
        if (unlikely(ret)) {
                memset(dst, 0, size);

kernel/fork.c +4 -0

index 3244cc56b697..8be2ca528a65 100644
@@ -96,6 +96,7 @@
 #include <linux/scs.h>
 #include <linux/io_uring.h>
 #include <linux/bpf.h>
+#include <linux/uaccess-buffer.h>
 
 #include <asm/pgalloc.h>
 #include <linux/uaccess.h>

@@ -754,6 +755,7 @@ void __put_task_struct(struct task_struct *tsk)
        delayacct_tsk_free(tsk);
        put_signal_struct(tsk->signal);
        sched_core_free(tsk);
+       uaccess_buffer_free(tsk);
 
        if (!profile_handoff_task(tsk))
                free_task(tsk);

@@ -890,6 +892,8 @@ static struct task_struct *dup_task_struct(struct task_struct *orig, int node)
        if (memcg_charge_kernel_stack(tsk))
                goto free_stack;
 
+       uaccess_buffer_free(orig);
+
        stack_vm_area = task_stack_vm_area(tsk);
 
        err = arch_dup_task_struct(tsk, orig);

kernel/signal.c +6 -3

index a629b11bf3e0..b85d7d4844f6 100644
@@ -45,6 +45,7 @@
 #include <linux/posix-timers.h>
 #include <linux/cgroup.h>
 #include <linux/audit.h>
+#include <linux/uaccess-buffer.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/signal.h>

@@ -1031,7 +1032,8 @@ static void complete_signal(int sig, struct task_struct *p, enum pid_type type)
        if (sig_fatal(p, sig) &&
            !(signal->flags & SIGNAL_GROUP_EXIT) &&
            !sigismember(&t->real_blocked, sig) &&
-           (sig == SIGKILL || !p->ptrace)) {
+           (sig == SIGKILL ||
+            !(p->ptrace || uaccess_buffer_maybe_blocked(p)))) {
                /*
                 * This signal will be fatal to the whole group.
                 */

@@ -3027,6 +3029,7 @@ void set_current_blocked(sigset_t *newset)
 void __set_current_blocked(const sigset_t *newset)
 {
        struct task_struct *tsk = current;
+       unsigned long flags;
 
        /*
         * In case the signal mask hasn't changed, there is nothing we need

@@ -3035,9 +3038,9 @@ void __set_current_blocked(const sigset_t *newset)
        if (sigequalsets(&tsk->blocked, newset))
                return;
 
-       spin_lock_irq(&tsk->sighand->siglock);
+       spin_lock_irqsave(&tsk->sighand->siglock, flags);
        __set_task_blocked(tsk, newset);
-       spin_unlock_irq(&tsk->sighand->siglock);
+       spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
 }
 
 /*

kernel/sys.c +6 -0

index 8fdac0d90504..c71a9a9c0f68 100644
@@ -42,6 +42,7 @@
 #include <linux/version.h>
 #include <linux/ctype.h>
 #include <linux/syscall_user_dispatch.h>
+#include <linux/uaccess-buffer.h>
 
 #include <linux/compat.h>
 #include <linux/syscalls.h>

@@ -2530,6 +2531,11 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
                error = sched_core_share_pid(arg2, arg3, arg4, arg5);
                break;
 #endif
+       case PR_SET_UACCESS_DESCRIPTOR_ADDR_ADDR:
+               if (arg3 || arg4 || arg5)
+                       return -EINVAL;
+               error = uaccess_buffer_set_descriptor_addr_addr(arg2);
+               break;
        default:
                error = -EINVAL;
                break;

kernel/uaccess-buffer.c new file +145 -1

new file mode 100644
index 000000000000..d3129244b7d9
@@ -0,0 +1,145 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Support for uaccess logging via uaccess buffers.
+ *
+ * Copyright (C) 2021, Google LLC.
+ */
+
+#include <linux/compat.h>
+#include <linux/mm.h>
+#include <linux/prctl.h>
+#include <linux/ptrace.h>
+#include <linux/sched.h>
+#include <linux/signal.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/uaccess-buffer.h>
+
+int uaccess_buffer_set_descriptor_addr_addr(unsigned long addr)
+{
+       current->uaccess_buffer.desc_ptr_ptr =
+               (struct uaccess_descriptor __user * __user *)addr;
+       if (addr)
+               set_syscall_work(UACCESS_BUFFER_ENTRY);
+       else
+               clear_syscall_work(UACCESS_BUFFER_ENTRY);
+       return 0;
+}
+
+static void uaccess_buffer_log(unsigned long addr, unsigned long size,
+                             unsigned long flags)
+{
+       struct uaccess_buffer_info *buf = &current->uaccess_buffer;
+       struct uaccess_buffer_entry *entry = buf->kcur;
+
+       if (entry == buf->kend || unlikely(uaccess_kernel()))
+               return;
+       entry->addr = addr;
+       entry->size = size;
+       entry->flags = flags;
+
+       ++buf->kcur;
+}
+
+void __uaccess_buffer_log_read(const void __user *from, unsigned long n)
+{
+       uaccess_buffer_log((unsigned long)from, n, 0);
+}
+EXPORT_SYMBOL(__uaccess_buffer_log_read);
+
+void __uaccess_buffer_log_write(void __user *to, unsigned long n)
+{
+       uaccess_buffer_log((unsigned long)to, n, UACCESS_BUFFER_FLAG_WRITE);
+}
+EXPORT_SYMBOL(__uaccess_buffer_log_write);
+
+bool __uaccess_buffer_pre_exit_loop(void)
+{
+       struct uaccess_buffer_info *buf = &current->uaccess_buffer;
+       struct uaccess_descriptor __user *desc_ptr;
+       sigset_t tmp_mask;
+
+       if (get_user(desc_ptr, buf->desc_ptr_ptr) || !desc_ptr)
+               return false;
+
+       current->real_blocked = current->blocked;
+       sigfillset(&tmp_mask);
+       set_current_blocked(&tmp_mask);
+       return true;
+}
+
+void __uaccess_buffer_post_exit_loop(void)
+{
+       unsigned long flags;
+
+       spin_lock_irqsave(&current->sighand->siglock, flags);
+       current->blocked = current->real_blocked;
+       recalc_sigpending();
+       spin_unlock_irqrestore(&current->sighand->siglock, flags);
+}
+
+void uaccess_buffer_free(struct task_struct *tsk)
+{
+       struct uaccess_buffer_info *buf = &tsk->uaccess_buffer;
+
+       kfree(buf->kbegin);
+       clear_syscall_work(UACCESS_BUFFER_EXIT);
+       buf->kbegin = buf->kcur = buf->kend = NULL;
+}
+
+void __uaccess_buffer_syscall_entry(void)
+{
+       struct uaccess_buffer_info *buf = &current->uaccess_buffer;
+       struct uaccess_descriptor desc;
+
+       if (get_user(buf->desc_ptr, buf->desc_ptr_ptr) || !buf->desc_ptr ||
+           put_user(0, buf->desc_ptr_ptr) ||
+           copy_from_user(&desc, buf->desc_ptr, sizeof(desc)))
+               return;
+
+       if (desc.size > 1024)
+               desc.size = 1024;
+
+       if (buf->kend - buf->kbegin != desc.size)
+               buf->kbegin =
+                       krealloc_array(buf->kbegin, desc.size,
+                                      sizeof(struct uaccess_buffer_entry),
+                                      GFP_KERNEL);
+       if (!buf->kbegin) {
+               buf->kend = NULL;
+               return;
+       }
+
+       set_syscall_work(UACCESS_BUFFER_EXIT);
+       buf->kcur = buf->kbegin;
+       buf->kend = buf->kbegin + desc.size;
+       buf->ubegin =
+               (struct uaccess_buffer_entry __user *)(unsigned long)desc.addr;
+}
+
+void __uaccess_buffer_syscall_exit(void)
+{
+       struct uaccess_buffer_info *buf = &current->uaccess_buffer;
+       u64 num_entries = buf->kcur - buf->kbegin;
+       struct uaccess_descriptor desc;
+
+       clear_syscall_work(UACCESS_BUFFER_EXIT);
+       desc.addr = (u64)(unsigned long)(buf->ubegin + num_entries);
+       desc.size = buf->kend - buf->kcur;
+       buf->kcur = NULL;
+       if (copy_to_user(buf->ubegin, buf->kbegin,
+                        num_entries * sizeof(struct uaccess_buffer_entry)) == 0)
+               (void)copy_to_user(buf->desc_ptr, &desc, sizeof(desc));
+}
+
+unsigned long copy_from_user_nolog(void *to, const void __user *from,
+                                  unsigned long len)
+{
+       size_t retval;
+
+       clear_syscall_work(UACCESS_BUFFER_EXIT);
+       retval = copy_from_user(to, from, len);
+       if (current->uaccess_buffer.kcur)
+               set_syscall_work(UACCESS_BUFFER_EXIT);
+       return retval;
+}
--

2.34.1.173.g76aa8bc2d0-goog

Add the core code to support uaccess logging. Subsequent patches will hook this up to the arch-specific kernel entry and exit code for certain architectures.

Link https://linux-review.googlesource.com/id/I6581765646501a5631b281d670903945ebadc57d

Signed-off-by Peter Collingbourne <[email protected]>

---
v4:
- add CONFIG_UACCESS_BUFFER
- add kernel doc comments to uaccess-buffer.h
- outline uaccess_buffer_set_descriptor_addr_addr
- switch to using spin_lock_irqsave/spin_unlock_irqrestore during pre/post-exit-loop code because preemption is disabled at that point
- set kend to NULL if krealloc failed
- size_t -> unsigned long in copy_from_user_nolog signature

v3:
- performance optimizations for entry/exit code
- don't use kcur == NULL to mean overflow
- fix potential double free in clone()
- don't allocate a new kernel-side uaccess buffer for each syscall
- fix uaccess buffer leak on exit
- fix some sparse warnings

v2:
- New interface that avoids multiple syscalls per real syscall and is arch-generic
- Avoid logging uaccesses done by BPF programs
- Add documentation
- Split up into multiple patches
- Various code moves, renames etc as requested by Marco

 arch/Kconfig                         |  13 +++
 fs/exec.c                            |   3 +
 include/linux/instrumented-uaccess.h |   6 +-
 include/linux/sched.h                |   5 +
 include/linux/uaccess-buffer-info.h  |  46 ++++++++
 include/linux/uaccess-buffer.h       | 152 +++++++++++++++++++++++++++
 include/uapi/linux/prctl.h           |   3 +
 include/uapi/linux/uaccess-buffer.h  |  27 +++++
 kernel/Makefile                      |   1 +
 kernel/bpf/helpers.c                 |   7 +-
 kernel/fork.c                        |   4 +
 kernel/signal.c                      |   9 +-
 kernel/sys.c                         |   6 ++
 kernel/uaccess-buffer.c              | 145 +++++++++++++++++++++++++
 14 files changed, 422 insertions(+), 5 deletions(-)
 create mode 100644 include/linux/uaccess-buffer-info.h
 create mode 100644 include/linux/uaccess-buffer.h
 create mode 100644 include/uapi/linux/uaccess-buffer.h
 create mode 100644 kernel/uaccess-buffer.c

arch/Kconfig +0 -0

index d3c4ab249e9c..17819f53ea80 100644

arch/Kconfig +6 -0

+	default y
+	depends on HAVE_ARCH_UACCESS_BUFFER
+	help
+	  Select to enable support for uaccess logging
+	  (see Documentation/admin-guide/uaccess-logging.rst).
+
 source "kernel/gcov/Kconfig"
 
 source "scripts/gcc-plugins/Kconfig"

fs/exec.c +3 -0

index 537d92c41105..c9975e790f30 100644
@@ -65,6 +65,7 @@
 #include <linux/vmalloc.h>
 #include <linux/io_uring.h>
 #include <linux/syscall_user_dispatch.h>
+#include <linux/uaccess-buffer.h>
 
 #include <linux/uaccess.h>
 #include <asm/mmu_context.h>

@@ -1313,6 +1314,8 @@ int begin_new_exec(struct linux_binprm * bprm)
 	me->personality &= ~bprm->per_clear;
 
 	clear_syscall_work_syscall_user_dispatch(me);
+	uaccess_buffer_set_descriptor_addr_addr(0);
+	uaccess_buffer_free(current);
 
 	/*
 	 * We have to apply CLOEXEC before we change whether the process is

include/linux/instrumented-uaccess.h +5 -1

index ece549088e50..b967f4436d15 100644
@@ -2,7 +2,8 @@
 
 /*
  * This header provides generic wrappers for memory access instrumentation for
- * uaccess routines that the compiler cannot emit for: KASAN, KCSAN.
+ * uaccess routines that the compiler cannot emit for: KASAN, KCSAN,
+ * uaccess buffers.
  */
 #ifndef _LINUX_INSTRUMENTED_UACCESS_H
 #define _LINUX_INSTRUMENTED_UACCESS_H

@@ -11,6 +12,7 @@
 #include <linux/kasan-checks.h>
 #include <linux/kcsan-checks.h>
 #include <linux/types.h>
+#include <linux/uaccess-buffer.h>
 
 /**
  * instrument_copy_to_user - instrument reads of copy_to_user

@@ -27,6 +29,7 @@ instrument_copy_to_user(void __user *to, const void *from, unsigned long n)
 {
 	kasan_check_read(from, n);
 	kcsan_check_read(from, n);
+	uaccess_buffer_log_write(to, n);
 }
 
 /**

@@ -44,6 +47,7 @@ instrument_copy_from_user(const void *to, const void __user *from, unsigned long
 {
 	kasan_check_write(to, n);
 	kcsan_check_write(to, n);
+	uaccess_buffer_log_read(from, n);
 }
 
 #endif /* _LINUX_INSTRUMENTED_UACCESS_H */

include/linux/sched.h +5 -0

index 78c351e35fec..96014dd2702e 100644
@@ -34,6 +34,7 @@
 #include <linux/rseq.h>
 #include <linux/seqlock.h>
 #include <linux/kcsan.h>
+#include <linux/uaccess-buffer-info.h>
 #include <asm/kmap_size.h>
 
 /* task_struct member predeclarations (sorted alphabetically): */

@@ -1484,6 +1485,10 @@ struct task_struct {
 	struct callback_head		l1d_flush_kill;
 #endif
 
+#ifdef CONFIG_UACCESS_BUFFER
+	struct uaccess_buffer_info	uaccess_buffer;
+#endif
+
 	/*
 	 * New fields for task_struct should be added above here, so that
 	 * they are included in the randomized portion of task_struct.

include/linux/uaccess-buffer-info.h new file +46 -0

new file mode 100644
index 000000000000..46e2b1a4a20f
@@ -0,0 +1,46 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_UACCESS_BUFFER_INFO_H
+#define _LINUX_UACCESS_BUFFER_INFO_H
+
+#ifdef CONFIG_UACCESS_BUFFER
+
+struct uaccess_buffer_info {
+	/*
+	 * The pointer to pointer to struct uaccess_descriptor. This is the
+	 * value controlled by prctl(PR_SET_UACCESS_DESCRIPTOR_ADDR_ADDR).
+	 */
+	struct uaccess_descriptor __user *__user *desc_ptr_ptr;
+
+	/*
+	 * The pointer to struct uaccess_descriptor read at syscall entry time.
+	 */
+	struct uaccess_descriptor __user *desc_ptr;
+
+	/*
+	 * A pointer to the kernel's temporary copy of the uaccess log for the
+	 * current syscall. We log to a kernel buffer in order to avoid leaking
+	 * timing information to userspace.
+	 */
+	struct uaccess_buffer_entry *kbegin;
+
+	/*
+	 * The position of the next uaccess buffer entry for the current
+	 * syscall, or NULL if we are not logging the current syscall.
+	 */
+	struct uaccess_buffer_entry *kcur;
+
+	/*
+	 * A pointer to the end of the kernel's uaccess log.
+	 */
+	struct uaccess_buffer_entry *kend;
+
+	/*
+	 * The pointer to the userspace uaccess log, as read from the
+	 * struct uaccess_descriptor.
+	 */
+	struct uaccess_buffer_entry __user *ubegin;
+};
+
+#endif
+
+#endif  /* _LINUX_UACCESS_BUFFER_INFO_H */

include/linux/uaccess-buffer.h new file +6 -0

new file mode 100644
index 000000000000..2e9b4010fb59
@@ -0,0 +1,152 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_UACCESS_BUFFER_H
+#define _LINUX_UACCESS_BUFFER_H
+
+#include <linux/sched.h>
+#include <uapi/linux/uaccess-buffer.h>

include/linux/uaccess-buffer.h new file +136 -0

+static inline bool uaccess_buffer_maybe_blocked(struct task_struct *tsk)
+{
+	return test_task_syscall_work(tsk, UACCESS_BUFFER_ENTRY);
+}
+
+void __uaccess_buffer_syscall_entry(void);
+/*
+ * uaccess_buffer_syscall_entry - hook to be run before syscall entry
+ */
+static inline void uaccess_buffer_syscall_entry(void)
+{
+	__uaccess_buffer_syscall_entry();
+}
+
+void __uaccess_buffer_syscall_exit(void);
+/*
+ * uaccess_buffer_syscall_exit - hook to be run after syscall exit
+ */
+static inline void uaccess_buffer_syscall_exit(void)
+{
+	__uaccess_buffer_syscall_exit();
+}
+
+bool __uaccess_buffer_pre_exit_loop(void);
+/*
+ * uaccess_buffer_pre_exit_loop - hook to be run immediately before the
+ * pre-kernel-exit loop that handles signals, tracing etc. Returns a bool to
+ * be passed to uaccess_buffer_post_exit_loop.
+ */
+static inline bool uaccess_buffer_pre_exit_loop(void)
+{
+	if (!test_syscall_work(UACCESS_BUFFER_ENTRY))
+		return false;
+	return __uaccess_buffer_pre_exit_loop();
+}
+
+void __uaccess_buffer_post_exit_loop(void);
+/*
+ * uaccess_buffer_post_exit_loop - hook to be run immediately after the
+ * pre-kernel-exit loop that handles signals, tracing etc.
+ * @pending: the bool returned from uaccess_buffer_pre_exit_loop.
+ */
+static inline void uaccess_buffer_post_exit_loop(bool pending)
+{
+	if (pending)
+		__uaccess_buffer_post_exit_loop();
+}
+
+/*
+ * uaccess_buffer_set_descriptor_addr_addr - implements
+ * prctl(PR_SET_UACCESS_DESCRIPTOR_ADDR_ADDR).
+ */
+int uaccess_buffer_set_descriptor_addr_addr(unsigned long addr);
+
+/*
+ * copy_from_user_nolog - a variant of copy_from_user that avoids uaccess
+ * logging. This is useful in special cases, such as when the kernel overreads a
+ * buffer.
+ * @to: the pointer to kernel memory.
+ * @from: the pointer to user memory.
+ * @len: the number of bytes to copy.
+ */
+unsigned long copy_from_user_nolog(void *to, const void __user *from,
+				   unsigned long len);
+
+/*
+ * uaccess_buffer_free - free the task's kernel-side uaccess buffer and arrange
+ * for uaccess logging to be cancelled for the current syscall
+ * @tsk: the task.
+ */
+void uaccess_buffer_free(struct task_struct *tsk);
+
+void __uaccess_buffer_log_read(const void __user *from, unsigned long n);
+/*
+ * uaccess_buffer_log_read - log a read access
+ * @from: the address of the access.
+ * @n: the number of bytes.
+ */
+static inline void uaccess_buffer_log_read(const void __user *from, unsigned long n)
+{
+	if (unlikely(test_syscall_work(UACCESS_BUFFER_EXIT)))
+		__uaccess_buffer_log_read(from, n);
+}
+
+void __uaccess_buffer_log_write(void __user *to, unsigned long n);
+/*
+ * uaccess_buffer_log_write - log a write access
+ * @to: the address of the access.
+ * @n: the number of bytes.
+ */
+static inline void uaccess_buffer_log_write(void __user *to, unsigned long n)
+{
+	if (unlikely(test_syscall_work(UACCESS_BUFFER_EXIT)))
+		__uaccess_buffer_log_write(to, n);
+}
+
+#else
+
+static inline bool uaccess_buffer_maybe_blocked(struct task_struct *tsk)
+{
+	return false;
+}
+static inline void uaccess_buffer_syscall_entry(void)
+{
+}
+static inline void uaccess_buffer_syscall_exit(void)
+{
+}
+static inline bool uaccess_buffer_pre_exit_loop(void)
+{
+	return false;
+}
+static inline void uaccess_buffer_post_exit_loop(bool pending)
+{
+}
+static inline int uaccess_buffer_set_descriptor_addr_addr(unsigned long addr)
+{
+	return -EINVAL;
+}
+static inline void uaccess_buffer_free(struct task_struct *tsk)
+{
+}
+
+#define copy_from_user_nolog copy_from_user
+
+static inline void uaccess_buffer_log_read(const void __user *from,
+					   unsigned long n)
+{
+}
+static inline void uaccess_buffer_log_write(void __user *to, unsigned long n)
+{
+}
+
+#endif
+
+#endif  /* _LINUX_UACCESS_BUFFER_H */

include/uapi/linux/prctl.h +3 -0

index bb73e9a0b24f..74b37469c7b3 100644
@@ -272,4 +272,7 @@ struct prctl_mm_map {
 # define PR_SCHED_CORE_SCOPE_THREAD_GROUP	1
 # define PR_SCHED_CORE_SCOPE_PROCESS_GROUP	2
 
+/* Configure uaccess logging feature */
+#define PR_SET_UACCESS_DESCRIPTOR_ADDR_ADDR	63
+
 #endif /* _LINUX_PRCTL_H */

include/uapi/linux/uaccess-buffer.h new file +27 -0

new file mode 100644
index 000000000000..bf10f7c78857
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_LINUX_UACCESS_BUFFER_H
+#define _UAPI_LINUX_UACCESS_BUFFER_H
+
+#include <linux/types.h>
+
+/* Location of the uaccess log. */
+struct uaccess_descriptor {
+	/* Address of the uaccess_buffer_entry array. */
+	__u64 addr;
+	/* Size of the uaccess_buffer_entry array in number of elements. */
+	__u64 size;
+};
+
+/* Format of the entries in the uaccess log. */
+struct uaccess_buffer_entry {
+	/* Address being accessed. */
+	__u64 addr;
+	/* Number of bytes that were accessed. */
+	__u64 size;
+	/* UACCESS_BUFFER_* flags. */
+	__u64 flags;
+};
+
+#define UACCESS_BUFFER_FLAG_WRITE	1 /* access was a write */
+
+#endif /* _UAPI_LINUX_UACCESS_BUFFER_H */

kernel/Makefile +1 -0

index 186c49582f45..e5f6c56696a2 100644
@@ -114,6 +114,7 @@ obj-$(CONFIG_KCSAN) += kcsan/
 obj-$(CONFIG_SHADOW_CALL_STACK) += scs.o
 obj-$(CONFIG_HAVE_STATIC_CALL_INLINE) += static_call.o
 obj-$(CONFIG_CFI_CLANG) += cfi.o
+obj-$(CONFIG_UACCESS_BUFFER) += uaccess-buffer.o
 
 obj-$(CONFIG_PERF_EVENTS) += events/
 

kernel/bpf/helpers.c +1 -0

index 649f07623df6..ab6520a633ef 100644
@@ -15,6 +15,7 @@
 #include <linux/pid_namespace.h>
 #include <linux/proc_ns.h>
 #include <linux/security.h>
+#include <linux/uaccess-buffer.h>
 
 #include "../../lib/kstrtox.h"
 

kernel/bpf/helpers.c +0 -0

 	if (unlikely(ret)) {
 		memset(dst, 0, size);

kernel/fork.c +4 -0

index 3244cc56b697..8be2ca528a65 100644
@@ -96,6 +96,7 @@
 #include <linux/scs.h>
 #include <linux/io_uring.h>
 #include <linux/bpf.h>
+#include <linux/uaccess-buffer.h>
 
 #include <asm/pgalloc.h>
 #include <linux/uaccess.h>

@@ -754,6 +755,7 @@ void __put_task_struct(struct task_struct *tsk)
 	delayacct_tsk_free(tsk);
 	put_signal_struct(tsk->signal);
 	sched_core_free(tsk);
+	uaccess_buffer_free(tsk);
 
 	if (!profile_handoff_task(tsk))
 		free_task(tsk);

@@ -890,6 +892,8 @@ static struct task_struct *dup_task_struct(struct task_struct *orig, int node)
 	if (memcg_charge_kernel_stack(tsk))
 		goto free_stack;
 
+	uaccess_buffer_free(orig);
+
 	stack_vm_area = task_stack_vm_area(tsk);
 
 	err = arch_dup_task_struct(tsk, orig);

kernel/signal.c +4 -1

index a629b11bf3e0..b85d7d4844f6 100644
@@ -45,6 +45,7 @@
 #include <linux/posix-timers.h>
 #include <linux/cgroup.h>
 #include <linux/audit.h>
+#include <linux/uaccess-buffer.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/signal.h>

@@ -1031,7 +1032,8 @@ static void complete_signal(int sig, struct task_struct *p, enum pid_type type)
 	if (sig_fatal(p, sig) &&
 	    !(signal->flags & SIGNAL_GROUP_EXIT) &&
 	    !sigismember(&t->real_blocked, sig) &&
-	    (sig == SIGKILL || !p->ptrace)) {
+	    (sig == SIGKILL ||
+	     !(p->ptrace || uaccess_buffer_maybe_blocked(p)))) {
 		/*
 		 * This signal will be fatal to the whole group.
 		 */

@@ -3027,6 +3029,7 @@ void set_current_blocked(sigset_t *newset)
 void __set_current_blocked(const sigset_t *newset)
 {
 	struct task_struct *tsk = current;
+	unsigned long flags;
 
 	/*
 	 * In case the signal mask hasn't changed, there is nothing we need

kernel/signal.c +0 -0

 /*

kernel/sys.c +6 -0

index 8fdac0d90504..c71a9a9c0f68 100644
@@ -42,6 +42,7 @@
 #include <linux/version.h>
 #include <linux/ctype.h>
 #include <linux/syscall_user_dispatch.h>
+#include <linux/uaccess-buffer.h>
 
 #include <linux/compat.h>
 #include <linux/syscalls.h>

@@ -2530,6 +2531,11 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 		error = sched_core_share_pid(arg2, arg3, arg4, arg5);
 		break;
 #endif
+	case PR_SET_UACCESS_DESCRIPTOR_ADDR_ADDR:
+		if (arg3 || arg4 || arg5)
+			return -EINVAL;
+		error = uaccess_buffer_set_descriptor_addr_addr(arg2);
+		break;
 	default:
 		error = -EINVAL;
 		break;

kernel/uaccess-buffer.c new file +97 -0

new file mode 100644
index 000000000000..d3129244b7d9
@@ -0,0 +1,145 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Support for uaccess logging via uaccess buffers.
+ *
+ * Copyright (C) 2021, Google LLC.
+ */
+
+#include <linux/compat.h>
+#include <linux/mm.h>
+#include <linux/prctl.h>
+#include <linux/ptrace.h>
+#include <linux/sched.h>
+#include <linux/signal.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/uaccess-buffer.h>
+
+int uaccess_buffer_set_descriptor_addr_addr(unsigned long addr)
+{
+	current->uaccess_buffer.desc_ptr_ptr =
+		(struct uaccess_descriptor __user * __user *)addr;
+	if (addr)
+		set_syscall_work(UACCESS_BUFFER_ENTRY);
+	else
+		clear_syscall_work(UACCESS_BUFFER_ENTRY);
+	return 0;
+}
+
+static void uaccess_buffer_log(unsigned long addr, unsigned long size,
+			      unsigned long flags)
+{
+	struct uaccess_buffer_info *buf = &current->uaccess_buffer;
+	struct uaccess_buffer_entry *entry = buf->kcur;
+
+	if (entry == buf->kend || unlikely(uaccess_kernel()))
+		return;
+	entry->addr = addr;
+	entry->size = size;
+	entry->flags = flags;
+
+	++buf->kcur;
+}
+
+void __uaccess_buffer_log_read(const void __user *from, unsigned long n)
+{
+	uaccess_buffer_log((unsigned long)from, n, 0);
+}
+EXPORT_SYMBOL(__uaccess_buffer_log_read);
+
+void __uaccess_buffer_log_write(void __user *to, unsigned long n)
+{
+	uaccess_buffer_log((unsigned long)to, n, UACCESS_BUFFER_FLAG_WRITE);
+}
+EXPORT_SYMBOL(__uaccess_buffer_log_write);
+
+bool __uaccess_buffer_pre_exit_loop(void)
+{
+	struct uaccess_buffer_info *buf = &current->uaccess_buffer;
+	struct uaccess_descriptor __user *desc_ptr;
+	sigset_t tmp_mask;
+
+	if (get_user(desc_ptr, buf->desc_ptr_ptr) || !desc_ptr)
+		return false;
+
+	current->real_blocked = current->blocked;
+	sigfillset(&tmp_mask);
+	set_current_blocked(&tmp_mask);
+	return true;
+}
+
+void __uaccess_buffer_post_exit_loop(void)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&current->sighand->siglock, flags);
+	current->blocked = current->real_blocked;
+	recalc_sigpending();
+	spin_unlock_irqrestore(&current->sighand->siglock, flags);
+}
+
+void uaccess_buffer_free(struct task_struct *tsk)
+{
+	struct uaccess_buffer_info *buf = &tsk->uaccess_buffer;
+
+	kfree(buf->kbegin);
+	clear_syscall_work(UACCESS_BUFFER_EXIT);
+	buf->kbegin = buf->kcur = buf->kend = NULL;
+}
+
+void __uaccess_buffer_syscall_entry(void)
+{
+	struct uaccess_buffer_info *buf = &current->uaccess_buffer;
+	struct uaccess_descriptor desc;
+
+	if (get_user(buf->desc_ptr, buf->desc_ptr_ptr) || !buf->desc_ptr ||
+	    put_user(0, buf->desc_ptr_ptr) ||
+	    copy_from_user(&desc, buf->desc_ptr, sizeof(desc)))

kernel/uaccess-buffer.c new file +38 -0

+	if (!buf->kbegin) {
+		buf->kend = NULL;
+		return;
+	}
+
+	set_syscall_work(UACCESS_BUFFER_EXIT);
+	buf->kcur = buf->kbegin;
+	buf->kend = buf->kbegin + desc.size;
+	buf->ubegin =
+		(struct uaccess_buffer_entry __user *)(unsigned long)desc.addr;
+}
+
+void __uaccess_buffer_syscall_exit(void)
+{
+	struct uaccess_buffer_info *buf = &current->uaccess_buffer;
+	u64 num_entries = buf->kcur - buf->kbegin;
+	struct uaccess_descriptor desc;
+
+	clear_syscall_work(UACCESS_BUFFER_EXIT);
+	desc.addr = (u64)(unsigned long)(buf->ubegin + num_entries);
+	desc.size = buf->kend - buf->kcur;
+	buf->kcur = NULL;
+	if (copy_to_user(buf->ubegin, buf->kbegin,
+			 num_entries * sizeof(struct uaccess_buffer_entry)) == 0)
+		(void)copy_to_user(buf->desc_ptr, &desc, sizeof(desc));
+}
+
+unsigned long copy_from_user_nolog(void *to, const void __user *from,
+				   unsigned long len)
+{
+	size_t retval;
+
+	clear_syscall_work(UACCESS_BUFFER_EXIT);
+	retval = copy_from_user(to, from, len);
+	if (current->uaccess_buffer.kcur)
+		set_syscall_work(UACCESS_BUFFER_EXIT);
+	return retval;
+}

Signature · 1 line

2.34.1.173.g76aa8bc2d0-goog

kernel/entry/common.c +1 -0

@@ -197,14 +201,19 @@ static unsigned long exit_to_user_mode_loop(struct pt_regs *regs,
 static void exit_to_user_mode_prepare(struct pt_regs *regs)
 {
 	unsigned long ti_work = READ_ONCE(current_thread_info()->flags);
+	bool uaccess_buffer_pending;
 
 	lockdep_assert_irqs_disabled();

(quoted diff) +7 -0

+/*
+ * uaccess_buffer_syscall_entry - hook to be run before syscall entry
+ */
 
+/*
+ * uaccess_buffer_syscall_exit - hook to be run after syscall exit
+ */
 
+/*

Peter,

Quoted 7 lines · Peter Collingbourne

kernel/entry/common.c +1 -0

@@ -197,14 +201,19 @@ static unsigned long exit_to_user_mode_loop(struct pt_regs *regs,
 static void exit_to_user_mode_prepare(struct pt_regs *regs)
 {
      unsigned long ti_work = READ_ONCE(current_thread_info()->flags);
+     bool uaccess_buffer_pending;
 
      lockdep_assert_irqs_disabled();

kernel/entry/common.c static unsigned long exit_to_user_mode_loop(struct pt_regs *regs, · line 215

Quoted 10 lines

kernel/entry/common.c +4 -1

 
      /* Flush pending rcuog wakeup before the last need_resched() check */
      tick_nohz_user_enter_prepare();
 
-     if (unlikely(ti_work & EXIT_TO_USER_MODE_WORK))
+     if (unlikely(ti_work & EXIT_TO_USER_MODE_WORK)) {
+             bool uaccess_buffer_pending = uaccess_buffer_pre_exit_loop();
+
              ti_work = exit_to_user_mode_loop(regs, ti_work);
+             uaccess_buffer_post_exit_loop(uaccess_buffer_pending);

What? Let me look at the these two functions, which are so full of useful comments:

Review static unsigned long exit_to_user_mode_loop(struct pt_regs *regs,

Quoted 12 lines

(quoted diff) +12 -0

+bool __uaccess_buffer_pre_exit_loop(void)
+{
+     struct uaccess_buffer_info *buf = &current->uaccess_buffer;
+     struct uaccess_descriptor __user *desc_ptr;
+     sigset_t tmp_mask;
+
+     if (get_user(desc_ptr, buf->desc_ptr_ptr) || !desc_ptr)
+             return false;
+
+     current->real_blocked = current->blocked;
+     sigfillset(&tmp_mask);
+     set_current_blocked(&tmp_mask);

This prevents signal delivery in exit_to_user_mode_loop(), right?

Review

Quoted 10 lines

(quoted diff) +10 -0

+     return true;
+}
+
+void __uaccess_buffer_post_exit_loop(void)
+{
+     unsigned long flags;
+
+     spin_lock_irqsave(&current->sighand->siglock, flags);
+     current->blocked = current->real_blocked;
+     recalc_sigpending();

This restores the signal blocked mask _after_ exit_to_user_mode_loop() has completed, recalculates pending signals and goes out to user space with eventually pending signals.

How is this supposed to be even remotely correct?

But that aside, let me look at the whole picture as I understand it from reverse engineering it. Yes, reverse engineering, because there are neither comments in the code nor any useful information in the changelogs of 2/7 and 4/7. Also the cover letter and the "documentation" are not explaining any of this and just blurb about sanitizers and how wonderful this all is.

kernel/entry/common.c static long syscall_trace_enter(struct pt_regs *regs, long syscall, · line 75

Quoted 6 lines

kernel/entry/common.c +2 -0

@@ -70,6 +71,9 @@ static long syscall_trace_enter(struct pt_regs *regs, long syscall,
                      return ret;
      }
 
+     if (work & SYSCALL_WORK_UACCESS_BUFFER_ENTRY)
+             uaccess_buffer_syscall_entry();

This conditionally sets SYSCALL_WORK_UACCESS_BUFFER_EXIT.

kernel/entry/common.c static void syscall_exit_work(struct pt_regs *regs, unsigned long work) · line 260

Quoted 6 lines

kernel/entry/common.c +2 -0

@@ -247,6 +256,9 @@ static void syscall_exit_work(struct pt_regs *regs, unsigned long work)
 
      audit_syscall_exit(regs);
 
+     if (work & SYSCALL_WORK_UACCESS_BUFFER_EXIT)
+             uaccess_buffer_syscall_exit();

When returning from the syscall and SYSCALL_WORK_UACCESS_BUFFER_EXIT is set, then uaccess_buffer_syscall_exit() clears SYSCALL_WORK_UACCESS_BUFFER_EXIT, right?

This is called _before_ exit_to_user_mode_prepare(). So why is this __uaccess_buffer_pre/post_exit_loop() required at all?

It's not required at all. Why?

Simply because there are only two ways how exit_to_user_mode_prepare() can be reached:

1) When returning from a syscall

2) When returning from an interrupt which hit user mode execution

#1 SYSCALL_WORK_UACCESS_BUFFER_EXIT is cleared _before_
exit_to_user_mode_prepare() is reached as documented above.

#2 SYSCALL_WORK_UACCESS_BUFFER_EXIT cannot be set because the entry
to the kernel does not go through syscall_trace_enter().

So what is this pre/post exit loop code about? Handle something which cannot happen in the first place?

If at all this would warrant a:

        if (WARN_ON_ONCE(test_syscall_work(UACCESS_BUFFER_ENTRY)))
                do_something_sensible();

instead of adding undocumented voodoo w/o providing any rationale. Well, I can see why that was not provided because there is no rationale to begin with.

Seriously, I'm all for better instrumentation and analysis, but if the code provided for that is incomprehensible, uncommented and undocumented, then the result is worse than what we have now.

If you think that this qualifies as documentation:

Quoted 13 lines

(quoted diff) +11 -0

+/*
+ * uaccess_buffer_syscall_entry - hook to be run before syscall entry
+ */
 
+/*
+ * uaccess_buffer_syscall_exit - hook to be run after syscall exit
+ */
 
+/*
+ * uaccess_buffer_pre_exit_loop - hook to be run immediately before the
+ * pre-kernel-exit loop that handles signals, tracing etc. Returns a bool to
+ * be passed to uaccess_buffer_post_exit_loop.
+ */

Quoted 5 lines

+/*
+ * uaccess_buffer_post_exit_loop - hook to be run immediately after the
+ * pre-kernel-exit loop that handles signals, tracing etc.
+ * @pending: the bool returned from uaccess_buffer_pre_exit_loop.
+ */

then we have a very differrent understanding of what documentation should provide.

Quoted 5 lines · Thomas Gleixner

This restores the signal blocked mask _after_ exit_to_user_mode_loop() has completed, recalculates pending signals and goes out to user space with eventually pending signals.

How is this supposed to be even remotely correct?

Please see this paragraph from the documentation:

When entering the kernel with a non-zero uaccess descriptor address for a reason other than a syscall (for example, when IPI'd due to an incoming asynchronous signal), any signals other than ``SIGKILL`` and ``SIGSTOP`` are masked as if by calling ``sigprocmask(SIG_SETMASK, set, NULL)`` where ``set`` has been initialized with ``sigfillset(set)``. This is to prevent incoming signals from interfering with uaccess logging.

I believe that we will also go out to userspace with pending signals when one of the signals that came in was a masked (via sigprocmask) asynchronous signal, so this is an expected state.

Quoted 2 lines

So what is this pre/post exit loop code about? Handle something which cannot happen in the first place?

The pre/post_exit_loop() functions are checking UACCESS_BUFFER_ENTRY, which is set when prctl(PR_SET_UACCESS_DESCRIPTOR_ADDR_ADDR) has been used to set the uaccess descriptor address address to a non-zero value. It is a different flag from UACCESS_BUFFER_EXIT. It is certainly possible for the ENTRY flag to be set in your 2) above, since that flag is not normally modified while inside the kernel.

Quoted 2 lines

then we have a very differrent understanding of what documentation should provide.

This was intended as interface documentation, so it doesn't go into too many details. It could certainly be improved though by referencing the user documentation, as I mentioned above.

Peter,

Quoted 17 lines · Peter Collingbourne

Quoted 5 lines · Thomas Gleixner

This restores the signal blocked mask _after_ exit_to_user_mode_loop() has completed, recalculates pending signals and goes out to user space with eventually pending signals.

How is this supposed to be even remotely correct?

Please see this paragraph from the documentation:

When entering the kernel with a non-zero uaccess descriptor address for a reason other than a syscall (for example, when IPI'd due to an incoming asynchronous signal), any signals other than ``SIGKILL`` and ``SIGSTOP`` are masked as if by calling ``sigprocmask(SIG_SETMASK, set, NULL)`` where ``set`` has been initialized with ``sigfillset(set)``. This is to prevent incoming signals from interfering with uaccess logging.

Quoted 3 lines

I believe that we will also go out to userspace with pending signals when one of the signals that came in was a masked (via sigprocmask) asynchronous signal, so this is an expected state.

Believe is not part of a technical analysis, believe belongs into the realm of religion.

It's a fundamental difference whether the program masks signals itself or the kernel decides to do that just because.

Pending signals, which are not masked by the process, have to be delivered _before_ returning to user space.

That's the expected behaviour. Period.

Instrumentation which changes the semantics of the observed code is broken by definition.

Quoted 9 lines

Quoted 2 lines

So what is this pre/post exit loop code about? Handle something which cannot happen in the first place?

The pre/post_exit_loop() functions are checking UACCESS_BUFFER_ENTRY, which is set when prctl(PR_SET_UACCESS_DESCRIPTOR_ADDR_ADDR) has been used to set the uaccess descriptor address address to a non-zero value. It is a different flag from UACCESS_BUFFER_EXIT. It is certainly possible for the ENTRY flag to be set in your 2) above, since that flag is not normally modified while inside the kernel.

Let me try again. The logger is only active when:

1) PR_SET_UACCESS_DESCRIPTOR_ADDR_ADDR has set an address, which sets UACCESS_BUFFER_ENTRY

2) The task enters the kernel via syscall and the syscall entry observes UACCESS_BUFFER_ENTRY and sets UACCESS_BUFFER_EXIT

because the log functions only log when UACCESS_BUFFER_EXIT is set.

UACCESS_BUFFER_EXIT is cleared in the syscall exit path _before_ the exit to usermode loop is reached, which means signal delivery is _NOT_ logged at all.

A non-syscall entry from user space - interrupt, exception, NMI - will _NOT_ set UACCESS_BUFFER_EXIT because it takes a different entry path. So when that non-syscall entry returns and delivers a signal then there is no logging.

When the task has entered the kernel via a syscall and the kernel gets interrupted and that interruption raises a signal, then there is no signal delivery. The interrupt returns to kernel mode, which obviously does not go through exit_to_user_mode(). The raised signal is delivered when the task returns from the syscall to user mode, but that won't be logged because UACCESS_BUFFER_EXIT is already cleared before the exit to user mode loop is reached.

See?

Quoted 6 lines

Quoted 2 lines

then we have a very differrent understanding of what documentation should provide.

This was intended as interface documentation, so it doesn't go into too many details. It could certainly be improved though by referencing the user documentation, as I mentioned above.

Explanations which are required to make the code understandable have to be in the code/kernel-doc comments and not in some disjunct place. This disjunct documentation is guaranteed to be out of date within no time.

userspace program sets addr to 0. Furthermore, a userspace program setting addr to 0 would not automatically cause the pending signals to be delivered (because simply storing a value to memory from userspace will not necessarily trigger a kernel entry), and signals could therefore be left blocked for longer than expected (at least until the next kernel entry).

Peter,

Quoted 6 lines · Peter Collingbourne

userspace program sets addr to 0. Furthermore, a userspace program setting addr to 0 would not automatically cause the pending signals to be delivered (because simply storing a value to memory from userspace will not necessarily trigger a kernel entry), and signals could therefore be left blocked for longer than expected (at least until the next kernel entry).

Groan, so what you are trying to prevent is:

   *ptr = addr;

--> interrupt
signal raised

signal delivery

signal handler
syscall() <- Logs this syscall
sigreturn;

syscall() <- Is not logged

I must have missed that detail in these novel sized comments all over the place.

Yes, I can see how that pre/post muck solves this, but TBH while it is admittedly a smart hack it's also a horrible hack.

There are a two aspects which I really dislike:

- It's yet another ad hoc 'solution' to scratch 'my particular itch'

- It's adding a horrorshow in the syscall hotpath. We have already enough gunk there. No need to add more.

The problem you are trying to solve is to instrument user accesses of the kernel, which is special purpose tracing, right?

May I ask why this is not solvable via tracepoints?

  DECLARE_EVENT_CLASS(uaccess_class,....);
  DECLARE_EVENT(uaccess_class, uaccess_read,...);
  DECLARE_EVENT(uaccess_class, uaccess_write,...);

    trace_uaccess_read(from, n);

    trace_uaccess_write(to, n);

Tracepoints have filters, tooling, libraries etc. Zero code except for the tracepoints themself. They are disabled by default with a static key, which means very close to 0 overhead.

Aside of that such tracepoints can be used for other purposes as well and are therefore not bound to 'my particular itch'.

There are obviously some questions to solve versus filtering, but even with a stupid tid based filter, it's easy enough to filter out the stuff you're interested in. E.g. to filter out the signal scenario above all you need is to enable two more tracepoints:

signal:signal_deliver
syscalls:sys_enter_rt_sigreturn

and when analyzing the event stream you can just skip the noise between signal:signal_deliver and syscalls:sys_enter_rt_sigreturn trace entries.

There are other fancies like BPF which can be used for filtering and filling a map with entries.

The only downside is that system configuration restricts access and requires certain priviledges. But is that a real problem for the purpose at hand, sanitizers and validation tools?

I don't think it is, but you surely can provide more information here.

Add a kselftest for the uaccess logging feature.

Link https://linux-review.googlesource.com/id/I39e1707fb8aef53747c42bd55b46ecaa67205199

Signed-off-by Peter Collingbourne <[email protected]>

---
 tools/testing/selftests/Makefile              |   1 +
 .../testing/selftests/uaccess_buffer/Makefile |   4 +
 .../uaccess_buffer/uaccess_buffer_test.c      | 126 ++++++++++++++++++
 3 files changed, 131 insertions(+)
 create mode 100644 tools/testing/selftests/uaccess_buffer/Makefile
 create mode 100644 tools/testing/selftests/uaccess_buffer/uaccess_buffer_test.c

tools/testing/selftests/Makefile +1 -0

index c852eb40c4f7..291b62430557 100644
@@ -71,6 +71,7 @@ TARGETS += timers
 endif
 TARGETS += tmpfs
 TARGETS += tpm2
+TARGETS += uaccess_buffer
 TARGETS += user
 TARGETS += vDSO
 TARGETS += vm

tools/testing/selftests/uaccess_buffer/Makefile new file +4 -0

new file mode 100644
index 000000000000..e6e5fb43ce29
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0
+TEST_GEN_PROGS := uaccess_buffer_test
+
+include ../lib.mk

tools/testing/selftests/uaccess_buffer/uaccess_buffer_test.c new file +62 -0

new file mode 100644
index 000000000000..051062e4fbf9
@@ -0,0 +1,126 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "../kselftest_harness.h"
+
+#include <linux/uaccess-buffer.h>
+#include <sys/prctl.h>
+#include <sys/utsname.h>
+
+FIXTURE(uaccess_buffer)
+{
+	uint64_t addr;
+};
+
+FIXTURE_SETUP(uaccess_buffer)
+{
+	ASSERT_EQ(0, prctl(PR_SET_UACCESS_DESCRIPTOR_ADDR_ADDR, &self->addr, 0,
+			   0, 0));
+}
+
+FIXTURE_TEARDOWN(uaccess_buffer)
+{
+	ASSERT_EQ(0, prctl(PR_SET_UACCESS_DESCRIPTOR_ADDR_ADDR, 0, 0, 0, 0));
+}
+
+TEST_F(uaccess_buffer, uname)
+{
+	struct uaccess_descriptor desc;
+	struct uaccess_buffer_entry entries[64];
+	struct utsname un;
+
+	desc.addr = (uint64_t)(unsigned long)entries;
+	desc.size = 64;
+	self->addr = (uint64_t)(unsigned long)&desc;
+	ASSERT_EQ(0, uname(&un));
+	ASSERT_EQ(0, self->addr);
+
+	if (desc.size == 63) {
+		ASSERT_EQ((uint64_t)(unsigned long)(entries + 1), desc.addr);
+
+		ASSERT_EQ((uint64_t)(unsigned long)&un, entries[0].addr);
+		ASSERT_EQ(sizeof(struct utsname), entries[0].size);
+		ASSERT_EQ(UACCESS_BUFFER_FLAG_WRITE, entries[0].flags);
+	} else {
+		/* See override_architecture in kernel/sys.c */
+		ASSERT_EQ(62, desc.size);
+		ASSERT_EQ((uint64_t)(unsigned long)(entries + 2), desc.addr);
+
+		ASSERT_EQ((uint64_t)(unsigned long)&un, entries[0].addr);
+		ASSERT_EQ(sizeof(struct utsname), entries[0].size);
+		ASSERT_EQ(UACCESS_BUFFER_FLAG_WRITE, entries[0].flags);
+
+		ASSERT_EQ((uint64_t)(unsigned long)&un.machine,
+			  entries[1].addr);
+		ASSERT_EQ(UACCESS_BUFFER_FLAG_WRITE, entries[1].flags);
+	}
+}
+
+static bool handled;
+
+static void usr1_handler(int signo)
+{
+	handled = true;

tools/testing/selftests/uaccess_buffer/uaccess_buffer_test.c new file +23 -0

+	struct sigaction act = {}, oldact;
+	int pid;
+
+	handled = false;
+	act.sa_handler = usr1_handler;
+	sigaction(SIGUSR1, &act, &oldact);
+
+	pid = fork();
+	if (pid == 0) {
+		/*
+		 * Busy loop to synchronize instead of issuing syscalls because
+		 * we need to test the behavior in the case where no syscall is
+		 * issued by the parent process.
+		 */
+		while (!shared->ready)
+			;
+		kill(getppid(), SIGUSR1);
+		shared->killed = true;
+		_exit(0);
+	} else {
+		int i;
+
+		desc.addr = 0;

tools/testing/selftests/uaccess_buffer/uaccess_buffer_test.c new file +18 -0

+
+		ASSERT_FALSE(handled);
+
+		/*
+		 * Returning from the waitpid syscall should trigger the signal
+		 * handler. The signal itself may also interrupt waitpid, so
+		 * make sure to handle EINTR.
+		 */
+		while (waitpid(pid, NULL, 0) == -1)
+			ASSERT_EQ(EINTR, errno);
+		ASSERT_TRUE(handled);
+	}
+
+	munmap((void *)shared, getpagesize());
+	sigaction(SIGUSR1, &oldact, NULL);
+}
+
+TEST_HARNESS_MAIN

Signature · 1 line

2.34.1.173.g76aa8bc2d0-goog

Sent: 09 December 2021 22:16

This patch series introduces a kernel feature known as uaccess logging, which allows userspace programs to be made aware of the address and size of uaccesses performed by the kernel during the servicing of a syscall. More details on the motivation for and interface to this feature are available in the file Documentation/admin-guide/uaccess-logging.rst added by the final patch in the series.

From: Peter Collingbourne

Quoted 9 lines

Sent: 09 December 2021 22:16

This patch series introduces a kernel feature known as uaccess logging, which allows userspace programs to be made aware of the address and size of uaccesses performed by the kernel during the servicing of a syscall. More details on the motivation for and interface to this feature are available in the file Documentation/admin-guide/uaccess-logging.rst added by the final patch in the series.

How does this work when get_user() and put_user() are used to do optimised copies?

While adding checks to copy_to/from_user() is going to have a measurable performance impact - even if nothing is done, adding them to get/put_user() (and friends) is going to make some hot paths really slow.

So maybe you could add it so KASAN test kernels, but you can't sensibly enable it on a production kernel.

Now, it might be that you could semi-sensibly log 'data' transfers. But have you actually looked at all the transfers that happen for something like sendmsg().
The 'user copy hardening' code already has a significant impact on that code (in many places).

Sent: 13 December 2021 19:49

Quoted 20 lines · David Laight

From: Peter Collingbourne

Quoted 9 lines

Sent: 09 December 2021 22:16

This patch series introduces a kernel feature known as uaccess logging, which allows userspace programs to be made aware of the address and size of uaccesses performed by the kernel during the servicing of a syscall. More details on the motivation for and interface to this feature are available in the file Documentation/admin-guide/uaccess-logging.rst added by the final patch in the series.

How does this work when get_user() and put_user() are used to do optimised copies?

While adding checks to copy_to/from_user() is going to have a measurable performance impact - even if nothing is done, adding them to get/put_user() (and friends) is going to make some hot paths really slow.

Quoted 8 lines

So maybe you could add it so KASAN test kernels, but you can't sensibly enable it on a production kernel.

Now, it might be that you could semi-sensibly log 'data' transfers. But have you actually looked at all the transfers that happen for something like sendmsg().
The 'user copy hardening' code already has a significant impact on that code (in many places).

Hi David,

Yes, I realised after I sent out my patch (and while writing test cases for it) that it didn't cover get_user()/put_user(). I have a patch under development that will add this coverage. I used it to run my invalid syscall and uname benchmarks and the results were basically the same as without the coverage.

Are you aware of any benchmarks that cover sendmsg()? I can try to look at writing my own if not. I was also planning to write a benchmark that uses getresuid() as this was the simplest syscall that I could find that does multiple put_user() calls.

From: Peter Collingbourne

Quoted 40 lines

Sent: 13 December 2021 19:49

Quoted 20 lines · David Laight

From: Peter Collingbourne

Quoted 9 lines

Sent: 09 December 2021 22:16

This patch series introduces a kernel feature known as uaccess logging, which allows userspace programs to be made aware of the address and size of uaccesses performed by the kernel during the servicing of a syscall. More details on the motivation for and interface to this feature are available in the file Documentation/admin-guide/uaccess-logging.rst added by the final patch in the series.

How does this work when get_user() and put_user() are used to do optimised copies?

While adding checks to copy_to/from_user() is going to have a measurable performance impact - even if nothing is done, adding them to get/put_user() (and friends) is going to make some hot paths really slow.

Quoted 8 lines

So maybe you could add it so KASAN test kernels, but you can't sensibly enable it on a production kernel.

Now, it might be that you could semi-sensibly log 'data' transfers. But have you actually looked at all the transfers that happen for something like sendmsg().
The 'user copy hardening' code already has a significant impact on that code (in many places).

Hi David,

Yes, I realised after I sent out my patch (and while writing test cases for it) that it didn't cover get_user()/put_user(). I have a patch under development that will add this coverage. I used it to run my invalid syscall and uname benchmarks and the results were basically the same as without the coverage.

Quoted 4 lines

Are you aware of any benchmarks that cover sendmsg()? I can try to look at writing my own if not. I was also planning to write a benchmark that uses getresuid() as this was the simplest syscall that I could find that does multiple put_user() calls.

Also look at sys_poll() I think that uses __put/get_user().

I think you'll find some of the socket option code also uses get_user().

There is also the compat code for import_iovec().
IIRC that is actually faster than the non-compat version at the moment.

I did some benchmarking of writev("/dev/null", iov, 10); The cost of reading in the iovec is significant in that case. Maybe I ought to find time to sort out my patches.

For sendmsg() using __copy_from_user() to avoid the user-copy hardening checks also makes a measurable difference when sending UDP through raw sockets - which we do a lot of.

I think you'd need to instrument user_access_begin() and also be able to merge trace entries (for multiple get_user() calls).

You really don't have to look far to find places where copy_to/from_user() is optimised to multiple get/put_user() or __get/put_user() (or are they the 'nofault' variants?)
Those are all hot paths - at least for some workloads.
So adding anything there isn't likely to be accepted for production kernels.

Quoted 62 lines · David Laight

From: Peter Collingbourne

Quoted 40 lines

Sent: 13 December 2021 19:49

Quoted 20 lines · David Laight

From: Peter Collingbourne

Quoted 9 lines

Sent: 09 December 2021 22:16

This patch series introduces a kernel feature known as uaccess logging, which allows userspace programs to be made aware of the address and size of uaccesses performed by the kernel during the servicing of a syscall. More details on the motivation for and interface to this feature are available in the file Documentation/admin-guide/uaccess-logging.rst added by the final patch in the series.

How does this work when get_user() and put_user() are used to do optimised copies?

While adding checks to copy_to/from_user() is going to have a measurable performance impact - even if nothing is done, adding them to get/put_user() (and friends) is going to make some hot paths really slow.

Quoted 8 lines

So maybe you could add it so KASAN test kernels, but you can't sensibly enable it on a production kernel.

Now, it might be that you could semi-sensibly log 'data' transfers. But have you actually looked at all the transfers that happen for something like sendmsg().
The 'user copy hardening' code already has a significant impact on that code (in many places).

Hi David,

Yes, I realised after I sent out my patch (and while writing test cases for it) that it didn't cover get_user()/put_user(). I have a patch under development that will add this coverage. I used it to run my invalid syscall and uname benchmarks and the results were basically the same as without the coverage.

Quoted 4 lines

Are you aware of any benchmarks that cover sendmsg()? I can try to look at writing my own if not. I was also planning to write a benchmark that uses getresuid() as this was the simplest syscall that I could find that does multiple put_user() calls.

Also look at sys_poll() I think that uses __put/get_user().

I think you'll find some of the socket option code also uses get_user().

There is also the compat code for import_iovec().
IIRC that is actually faster than the non-compat version at the moment.

I did some benchmarking of writev("/dev/null", iov, 10); The cost of reading in the iovec is significant in that case. Maybe I ought to find time to sort out my patches.

For sendmsg() using __copy_from_user() to avoid the user-copy hardening checks also makes a measurable difference when sending UDP through raw sockets - which we do a lot of.

Quoted 8 lines

I think you'd need to instrument user_access_begin() and also be able to merge trace entries (for multiple get_user() calls).

You really don't have to look far to find places where copy_to/from_user() is optimised to multiple get/put_user() or __get/put_user() (or are they the 'nofault' variants?)
Those are all hot paths - at least for some workloads.
So adding anything there isn't likely to be accepted for production kernels.

Okay, but let's see what the benchmarks say first.

I added calls to uaccess_buffer_log_{read,write}() to __{get,put}_user() in arch/arm64/include/asm/uaccess.h and wrote a variant of my usual test program that does getresuid() in a loop and measured an overhead of 5.9% on the small cores and 2.5% on the big cores. The overhead appears to come from two sources:
1) The calling convention for the call to __uaccess_buffer_log_read/write() transforms some leaf functions into non-leaf functions, and as a result we end up spilling more registers.
2) We need to reload the flags from the task_struct every time to determine the feature enabled state.
I think it should be possible to reduce the cost down to one instruction (a conditional branch) per get/put_user() call, at least on arm64, by using a different calling convention for the call and maintaining the feature enabled state in one of the currently-ignored bits 60-63 of a reserved register (x18).

I have the patch below which reduces the overhead with my test program to 2.3% small 1.5% big (not fully functional yet because the rest of the code never actually flips that bit in x18). For syscalls like sendmsg() and poll() I would expect the overall overhead to be lower because of other required work done by these syscalls (glancing through their implementations this appears to be the case). But I'll benchmark it of course. The preserve_most attribute is Clang-only but I think it should be possible to use inline asm to get the same effect with GCC. We currently only reserve x18 with CONFIG_SHADOW_CALL_STACK enabled but I was unable to measure an overhead for getresuid() when reserving x18 unconditionally.