* [PATCHv5 bpf-next 1/8] uprobe: Wire up uretprobe system call
From: Jiri Olsa @ 2024-05-07 10:53 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240507105321.71524-1-jolsa@kernel.org>
Wiring up uretprobe system call, which comes in following changes.
We need to do the wiring before, because the uretprobe implementation
needs the syscall number.
Note at the moment uretprobe syscall is supported only for native
64-bit process.
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
include/linux/syscalls.h | 2 ++
include/uapi/asm-generic/unistd.h | 5 ++++-
kernel/sys_ni.c | 2 ++
4 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index 7e8d46f4147f..af0a33ab06ee 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -383,6 +383,7 @@
459 common lsm_get_self_attr sys_lsm_get_self_attr
460 common lsm_set_self_attr sys_lsm_set_self_attr
461 common lsm_list_modules sys_lsm_list_modules
+462 64 uretprobe sys_uretprobe
#
# Due to a historical design error, certain syscalls are numbered differently
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index e619ac10cd23..5318e0e76799 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -972,6 +972,8 @@ asmlinkage long sys_lsm_list_modules(u64 *ids, u32 *size, u32 flags);
/* x86 */
asmlinkage long sys_ioperm(unsigned long from, unsigned long num, int on);
+asmlinkage long sys_uretprobe(void);
+
/* pciconfig: alpha, arm, arm64, ia64, sparc */
asmlinkage long sys_pciconfig_read(unsigned long bus, unsigned long dfn,
unsigned long off, unsigned long len,
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 75f00965ab15..8a747cd1d735 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -842,8 +842,11 @@ __SYSCALL(__NR_lsm_set_self_attr, sys_lsm_set_self_attr)
#define __NR_lsm_list_modules 461
__SYSCALL(__NR_lsm_list_modules, sys_lsm_list_modules)
+#define __NR_uretprobe 462
+__SYSCALL(__NR_uretprobe, sys_uretprobe)
+
#undef __NR_syscalls
-#define __NR_syscalls 462
+#define __NR_syscalls 463
/*
* 32 bit systems traditionally used different
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index faad00cce269..be6195e0d078 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -391,3 +391,5 @@ COND_SYSCALL(setuid16);
/* restartable sequence */
COND_SYSCALL(rseq);
+
+COND_SYSCALL(uretprobe);
--
2.44.0
^ permalink raw reply related
* [PATCHv5 bpf-next 2/8] uprobe: Add uretprobe syscall to speed up return probe
From: Jiri Olsa @ 2024-05-07 10:53 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240507105321.71524-1-jolsa@kernel.org>
Adding uretprobe syscall instead of trap to speed up return probe.
At the moment the uretprobe setup/path is:
- install entry uprobe
- when the uprobe is hit, it overwrites probed function's return address
on stack with address of the trampoline that contains breakpoint
instruction
- the breakpoint trap code handles the uretprobe consumers execution and
jumps back to original return address
This patch replaces the above trampoline's breakpoint instruction with new
ureprobe syscall call. This syscall does exactly the same job as the trap
with some more extra work:
- syscall trampoline must save original value for rax/r11/rcx registers
on stack - rax is set to syscall number and r11/rcx are changed and
used by syscall instruction
- the syscall code reads the original values of those registers and
restore those values in task's pt_regs area
- only caller from trampoline exposed in '[uprobes]' is allowed,
the process will receive SIGILL signal otherwise
Even with some extra work, using the uretprobes syscall shows speed
improvement (compared to using standard breakpoint):
On Intel (11th Gen Intel(R) Core(TM) i7-1165G7 @ 2.80GHz)
current:
uretprobe-nop : 1.498 ± 0.000M/s
uretprobe-push : 1.448 ± 0.001M/s
uretprobe-ret : 0.816 ± 0.001M/s
with the fix:
uretprobe-nop : 1.969 ± 0.002M/s < 31% speed up
uretprobe-push : 1.910 ± 0.000M/s < 31% speed up
uretprobe-ret : 0.934 ± 0.000M/s < 14% speed up
On Amd (AMD Ryzen 7 5700U)
current:
uretprobe-nop : 0.778 ± 0.001M/s
uretprobe-push : 0.744 ± 0.001M/s
uretprobe-ret : 0.540 ± 0.001M/s
with the fix:
uretprobe-nop : 0.860 ± 0.001M/s < 10% speed up
uretprobe-push : 0.818 ± 0.001M/s < 10% speed up
uretprobe-ret : 0.578 ± 0.000M/s < 7% speed up
The performance test spawns a thread that runs loop which triggers
uprobe with attached bpf program that increments the counter that
gets printed in results above.
The uprobe (and uretprobe) kind is determined by which instruction
is being patched with breakpoint instruction. That's also important
for uretprobes, because uprobe is installed for each uretprobe.
The performance test is part of bpf selftests:
tools/testing/selftests/bpf/run_bench_uprobes.sh
Note at the moment uretprobe syscall is supported only for native
64-bit process, compat process still uses standard breakpoint.
Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/kernel/uprobes.c | 115 ++++++++++++++++++++++++++++++++++++++
include/linux/uprobes.h | 3 +
kernel/events/uprobes.c | 24 +++++---
3 files changed, 135 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 6c07f6daaa22..81e6ee95784d 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -12,6 +12,7 @@
#include <linux/ptrace.h>
#include <linux/uprobes.h>
#include <linux/uaccess.h>
+#include <linux/syscalls.h>
#include <linux/kdebug.h>
#include <asm/processor.h>
@@ -308,6 +309,120 @@ static int uprobe_init_insn(struct arch_uprobe *auprobe, struct insn *insn, bool
}
#ifdef CONFIG_X86_64
+
+asm (
+ ".pushsection .rodata\n"
+ ".global uretprobe_syscall_entry\n"
+ "uretprobe_syscall_entry:\n"
+ "pushq %rax\n"
+ "pushq %rcx\n"
+ "pushq %r11\n"
+ "movq $" __stringify(__NR_uretprobe) ", %rax\n"
+ "syscall\n"
+ ".global uretprobe_syscall_check\n"
+ "uretprobe_syscall_check:\n"
+ "popq %r11\n"
+ "popq %rcx\n"
+
+ /* The uretprobe syscall replaces stored %rax value with final
+ * return address, so we don't restore %rax in here and just
+ * call ret.
+ */
+ "retq\n"
+ ".global uretprobe_syscall_end\n"
+ "uretprobe_syscall_end:\n"
+ ".popsection\n"
+);
+
+extern u8 uretprobe_syscall_entry[];
+extern u8 uretprobe_syscall_check[];
+extern u8 uretprobe_syscall_end[];
+
+void *arch_uprobe_trampoline(unsigned long *psize)
+{
+ static uprobe_opcode_t insn = UPROBE_SWBP_INSN;
+ struct pt_regs *regs = task_pt_regs(current);
+
+ /*
+ * At the moment the uretprobe syscall trampoline is supported
+ * only for native 64-bit process, the compat process still uses
+ * standard breakpoint.
+ */
+ if (user_64bit_mode(regs)) {
+ *psize = uretprobe_syscall_end - uretprobe_syscall_entry;
+ return uretprobe_syscall_entry;
+ }
+
+ *psize = UPROBE_SWBP_INSN_SIZE;
+ return &insn;
+}
+
+static unsigned long trampoline_check_ip(void)
+{
+ unsigned long tramp = uprobe_get_trampoline_vaddr();
+
+ return tramp + (uretprobe_syscall_check - uretprobe_syscall_entry);
+}
+
+SYSCALL_DEFINE0(uretprobe)
+{
+ struct pt_regs *regs = task_pt_regs(current);
+ unsigned long err, ip, sp, r11_cx_ax[3];
+
+ if (regs->ip != trampoline_check_ip())
+ goto sigill;
+
+ err = copy_from_user(r11_cx_ax, (void __user *)regs->sp, sizeof(r11_cx_ax));
+ if (err)
+ goto sigill;
+
+ /* expose the "right" values of r11/cx/ax/sp to uprobe_consumer/s */
+ regs->r11 = r11_cx_ax[0];
+ regs->cx = r11_cx_ax[1];
+ regs->ax = r11_cx_ax[2];
+ regs->sp += sizeof(r11_cx_ax);
+ regs->orig_ax = -1;
+
+ ip = regs->ip;
+ sp = regs->sp;
+
+ uprobe_handle_trampoline(regs);
+
+ /*
+ * uprobe_consumer has changed sp, we can do nothing,
+ * just return via iret
+ */
+ if (regs->sp != sp)
+ return regs->ax;
+ regs->sp -= sizeof(r11_cx_ax);
+
+ /* for the case uprobe_consumer has changed r11/cx */
+ r11_cx_ax[0] = regs->r11;
+ r11_cx_ax[1] = regs->cx;
+
+ /*
+ * ax register is passed through as return value, so we can use
+ * its space on stack for ip value and jump to it through the
+ * trampoline's ret instruction
+ */
+ r11_cx_ax[2] = regs->ip;
+ regs->ip = ip;
+
+ err = copy_to_user((void __user *)regs->sp, r11_cx_ax, sizeof(r11_cx_ax));
+ if (err)
+ goto sigill;
+
+ /* ensure sysret, see do_syscall_64() */
+ regs->r11 = regs->flags;
+ regs->cx = regs->ip;
+
+ return regs->ax;
+
+sigill:
+ force_sig(SIGILL);
+ return -1;
+}
+
/*
* If arch_uprobe->insn doesn't use rip-relative addressing, return
* immediately. Otherwise, rewrite the instruction so that it accesses
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index f46e0ca0169c..b503fafb7fb3 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -138,6 +138,9 @@ extern bool arch_uretprobe_is_alive(struct return_instance *ret, enum rp_check c
extern bool arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
extern void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
void *src, unsigned long len);
+extern void uprobe_handle_trampoline(struct pt_regs *regs);
+extern void *arch_uprobe_trampoline(unsigned long *psize);
+extern unsigned long uprobe_get_trampoline_vaddr(void);
#else /* !CONFIG_UPROBES */
struct uprobes_state {
};
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index e4834d23e1d1..c550449d66be 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1474,11 +1474,20 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
return ret;
}
+void * __weak arch_uprobe_trampoline(unsigned long *psize)
+{
+ static uprobe_opcode_t insn = UPROBE_SWBP_INSN;
+
+ *psize = UPROBE_SWBP_INSN_SIZE;
+ return &insn;
+}
+
static struct xol_area *__create_xol_area(unsigned long vaddr)
{
struct mm_struct *mm = current->mm;
- uprobe_opcode_t insn = UPROBE_SWBP_INSN;
+ unsigned long insns_size;
struct xol_area *area;
+ void *insns;
area = kmalloc(sizeof(*area), GFP_KERNEL);
if (unlikely(!area))
@@ -1502,7 +1511,8 @@ static struct xol_area *__create_xol_area(unsigned long vaddr)
/* Reserve the 1st slot for get_trampoline_vaddr() */
set_bit(0, area->bitmap);
atomic_set(&area->slot_count, 1);
- arch_uprobe_copy_ixol(area->pages[0], 0, &insn, UPROBE_SWBP_INSN_SIZE);
+ insns = arch_uprobe_trampoline(&insns_size);
+ arch_uprobe_copy_ixol(area->pages[0], 0, insns, insns_size);
if (!xol_add_vma(mm, area))
return area;
@@ -1827,7 +1837,7 @@ void uprobe_copy_process(struct task_struct *t, unsigned long flags)
*
* Returns -1 in case the xol_area is not allocated.
*/
-static unsigned long get_trampoline_vaddr(void)
+unsigned long uprobe_get_trampoline_vaddr(void)
{
struct xol_area *area;
unsigned long trampoline_vaddr = -1;
@@ -1878,7 +1888,7 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
if (!ri)
return;
- trampoline_vaddr = get_trampoline_vaddr();
+ trampoline_vaddr = uprobe_get_trampoline_vaddr();
orig_ret_vaddr = arch_uretprobe_hijack_return_addr(trampoline_vaddr, regs);
if (orig_ret_vaddr == -1)
goto fail;
@@ -2123,7 +2133,7 @@ static struct return_instance *find_next_ret_chain(struct return_instance *ri)
return ri;
}
-static void handle_trampoline(struct pt_regs *regs)
+void uprobe_handle_trampoline(struct pt_regs *regs)
{
struct uprobe_task *utask;
struct return_instance *ri, *next;
@@ -2187,8 +2197,8 @@ static void handle_swbp(struct pt_regs *regs)
int is_swbp;
bp_vaddr = uprobe_get_swbp_addr(regs);
- if (bp_vaddr == get_trampoline_vaddr())
- return handle_trampoline(regs);
+ if (bp_vaddr == uprobe_get_trampoline_vaddr())
+ return uprobe_handle_trampoline(regs);
uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
if (!uprobe) {
--
2.44.0
^ permalink raw reply related
* [PATCHv5 bpf-next 3/8] selftests/bpf: Add uretprobe syscall test for regs integrity
From: Jiri Olsa @ 2024-05-07 10:53 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240507105321.71524-1-jolsa@kernel.org>
Add uretprobe syscall test that compares register values before
and after the uretprobe is hit. It also compares the register
values seen from attached bpf program.
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/include/linux/compiler.h | 4 +
.../selftests/bpf/prog_tests/uprobe_syscall.c | 163 ++++++++++++++++++
.../selftests/bpf/progs/uprobe_syscall.c | 15 ++
3 files changed, 182 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall.c
diff --git a/tools/include/linux/compiler.h b/tools/include/linux/compiler.h
index 8a63a9913495..6f7f22ac9da5 100644
--- a/tools/include/linux/compiler.h
+++ b/tools/include/linux/compiler.h
@@ -62,6 +62,10 @@
#define __nocf_check __attribute__((nocf_check))
#endif
+#ifndef __naked
+#define __naked __attribute__((__naked__))
+#endif
+
/* Are two types/vars the same type (ignoring qualifiers)? */
#ifndef __same_type
# define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
new file mode 100644
index 000000000000..311ac19d8992
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
@@ -0,0 +1,163 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <test_progs.h>
+
+#ifdef __x86_64__
+
+#include <unistd.h>
+#include <asm/ptrace.h>
+#include <linux/compiler.h>
+#include "uprobe_syscall.skel.h"
+
+__naked unsigned long uretprobe_regs_trigger(void)
+{
+ asm volatile (
+ "movq $0xdeadbeef, %rax\n"
+ "ret\n"
+ );
+}
+
+__naked void uretprobe_regs(struct pt_regs *before, struct pt_regs *after)
+{
+ asm volatile (
+ "movq %r15, 0(%rdi)\n"
+ "movq %r14, 8(%rdi)\n"
+ "movq %r13, 16(%rdi)\n"
+ "movq %r12, 24(%rdi)\n"
+ "movq %rbp, 32(%rdi)\n"
+ "movq %rbx, 40(%rdi)\n"
+ "movq %r11, 48(%rdi)\n"
+ "movq %r10, 56(%rdi)\n"
+ "movq %r9, 64(%rdi)\n"
+ "movq %r8, 72(%rdi)\n"
+ "movq %rax, 80(%rdi)\n"
+ "movq %rcx, 88(%rdi)\n"
+ "movq %rdx, 96(%rdi)\n"
+ "movq %rsi, 104(%rdi)\n"
+ "movq %rdi, 112(%rdi)\n"
+ "movq $0, 120(%rdi)\n" /* orig_rax */
+ "movq $0, 128(%rdi)\n" /* rip */
+ "movq $0, 136(%rdi)\n" /* cs */
+ "pushf\n"
+ "pop %rax\n"
+ "movq %rax, 144(%rdi)\n" /* eflags */
+ "movq %rsp, 152(%rdi)\n" /* rsp */
+ "movq $0, 160(%rdi)\n" /* ss */
+
+ /* save 2nd argument */
+ "pushq %rsi\n"
+ "call uretprobe_regs_trigger\n"
+
+ /* save return value and load 2nd argument pointer to rax */
+ "pushq %rax\n"
+ "movq 8(%rsp), %rax\n"
+
+ "movq %r15, 0(%rax)\n"
+ "movq %r14, 8(%rax)\n"
+ "movq %r13, 16(%rax)\n"
+ "movq %r12, 24(%rax)\n"
+ "movq %rbp, 32(%rax)\n"
+ "movq %rbx, 40(%rax)\n"
+ "movq %r11, 48(%rax)\n"
+ "movq %r10, 56(%rax)\n"
+ "movq %r9, 64(%rax)\n"
+ "movq %r8, 72(%rax)\n"
+ "movq %rcx, 88(%rax)\n"
+ "movq %rdx, 96(%rax)\n"
+ "movq %rsi, 104(%rax)\n"
+ "movq %rdi, 112(%rax)\n"
+ "movq $0, 120(%rax)\n" /* orig_rax */
+ "movq $0, 128(%rax)\n" /* rip */
+ "movq $0, 136(%rax)\n" /* cs */
+
+ /* restore return value and 2nd argument */
+ "pop %rax\n"
+ "pop %rsi\n"
+
+ "movq %rax, 80(%rsi)\n"
+
+ "pushf\n"
+ "pop %rax\n"
+
+ "movq %rax, 144(%rsi)\n" /* eflags */
+ "movq %rsp, 152(%rsi)\n" /* rsp */
+ "movq $0, 160(%rsi)\n" /* ss */
+ "ret\n"
+);
+}
+
+static void test_uretprobe_regs_equal(void)
+{
+ struct uprobe_syscall *skel = NULL;
+ struct pt_regs before = {}, after = {};
+ unsigned long *pb = (unsigned long *) &before;
+ unsigned long *pa = (unsigned long *) &after;
+ unsigned long *pp;
+ unsigned int i, cnt;
+ int err;
+
+ skel = uprobe_syscall__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "uprobe_syscall__open_and_load"))
+ goto cleanup;
+
+ err = uprobe_syscall__attach(skel);
+ if (!ASSERT_OK(err, "uprobe_syscall__attach"))
+ goto cleanup;
+
+ uretprobe_regs(&before, &after);
+
+ pp = (unsigned long *) &skel->bss->regs;
+ cnt = sizeof(before)/sizeof(*pb);
+
+ for (i = 0; i < cnt; i++) {
+ unsigned int offset = i * sizeof(unsigned long);
+
+ /*
+ * Check register before and after uretprobe_regs_trigger call
+ * that triggers the uretprobe.
+ */
+ switch (offset) {
+ case offsetof(struct pt_regs, rax):
+ ASSERT_EQ(pa[i], 0xdeadbeef, "return value");
+ break;
+ default:
+ if (!ASSERT_EQ(pb[i], pa[i], "register before-after value check"))
+ fprintf(stdout, "failed register offset %u\n", offset);
+ }
+
+ /*
+ * Check register seen from bpf program and register after
+ * uretprobe_regs_trigger call
+ */
+ switch (offset) {
+ /*
+ * These values will be different (not set in uretprobe_regs),
+ * we don't care.
+ */
+ case offsetof(struct pt_regs, orig_rax):
+ case offsetof(struct pt_regs, rip):
+ case offsetof(struct pt_regs, cs):
+ case offsetof(struct pt_regs, rsp):
+ case offsetof(struct pt_regs, ss):
+ break;
+ default:
+ if (!ASSERT_EQ(pp[i], pa[i], "register prog-after value check"))
+ fprintf(stdout, "failed register offset %u\n", offset);
+ }
+ }
+
+cleanup:
+ uprobe_syscall__destroy(skel);
+}
+#else
+static void test_uretprobe_regs_equal(void)
+{
+ test__skip();
+}
+#endif
+
+void test_uprobe_syscall(void)
+{
+ if (test__start_subtest("uretprobe_regs_equal"))
+ test_uretprobe_regs_equal();
+}
diff --git a/tools/testing/selftests/bpf/progs/uprobe_syscall.c b/tools/testing/selftests/bpf/progs/uprobe_syscall.c
new file mode 100644
index 000000000000..8a4fa6c7ef59
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/uprobe_syscall.c
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <string.h>
+
+struct pt_regs regs;
+
+char _license[] SEC("license") = "GPL";
+
+SEC("uretprobe//proc/self/exe:uretprobe_regs_trigger")
+int uretprobe(struct pt_regs *ctx)
+{
+ __builtin_memcpy(®s, ctx, sizeof(regs));
+ return 0;
+}
--
2.44.0
^ permalink raw reply related
* [PATCHv5 bpf-next 4/8] selftests/bpf: Add uretprobe syscall test for regs changes
From: Jiri Olsa @ 2024-05-07 10:53 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240507105321.71524-1-jolsa@kernel.org>
Adding test that creates uprobe consumer on uretprobe which changes some
of the registers. Making sure the changed registers are propagated to the
user space when the ureptobe syscall trampoline is used on x86_64.
To be able to do this, adding support to bpf_testmod to create uprobe via
new attribute file:
/sys/kernel/bpf_testmod_uprobe
This file is expecting file offset and creates related uprobe on current
process exe file and removes existing uprobe if offset is 0. The can be
only single uprobe at any time.
The uprobe has specific consumer that changes registers used in ureprobe
syscall trampoline and which are later checked in the test.
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
.../selftests/bpf/bpf_testmod/bpf_testmod.c | 123 +++++++++++++++++-
.../selftests/bpf/prog_tests/uprobe_syscall.c | 67 ++++++++++
2 files changed, 189 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index e93013fc7bf4..6c43d68fa323 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -18,6 +18,7 @@
#include <linux/in6.h>
#include <linux/un.h>
#include <net/sock.h>
+#include <linux/namei.h>
#include "bpf_testmod.h"
#include "bpf_testmod_kfunc.h"
@@ -355,6 +356,119 @@ static struct bin_attribute bin_attr_bpf_testmod_file __ro_after_init = {
.write = bpf_testmod_test_write,
};
+/* bpf_testmod_uprobe sysfs attribute is so far enabled for x86_64 only,
+ * please see test_uretprobe_regs_change test
+ */
+#ifdef __x86_64__
+
+static int
+uprobe_ret_handler(struct uprobe_consumer *self, unsigned long func,
+ struct pt_regs *regs)
+
+{
+ regs->ax = 0x12345678deadbeef;
+ regs->cx = 0x87654321feebdaed;
+ regs->r11 = (u64) -1;
+ return true;
+}
+
+struct testmod_uprobe {
+ struct path path;
+ loff_t offset;
+ struct uprobe_consumer consumer;
+};
+
+static DEFINE_MUTEX(testmod_uprobe_mutex);
+
+static struct testmod_uprobe uprobe = {
+ .consumer.ret_handler = uprobe_ret_handler,
+};
+
+static int testmod_register_uprobe(loff_t offset)
+{
+ int err = -EBUSY;
+
+ if (uprobe.offset)
+ return -EBUSY;
+
+ mutex_lock(&testmod_uprobe_mutex);
+
+ if (uprobe.offset)
+ goto out;
+
+ err = kern_path("/proc/self/exe", LOOKUP_FOLLOW, &uprobe.path);
+ if (err)
+ goto out;
+
+ err = uprobe_register_refctr(d_real_inode(uprobe.path.dentry),
+ offset, 0, &uprobe.consumer);
+ if (err)
+ path_put(&uprobe.path);
+ else
+ uprobe.offset = offset;
+
+out:
+ mutex_unlock(&testmod_uprobe_mutex);
+ return err;
+}
+
+static void testmod_unregister_uprobe(void)
+{
+ mutex_lock(&testmod_uprobe_mutex);
+
+ if (uprobe.offset) {
+ uprobe_unregister(d_real_inode(uprobe.path.dentry),
+ uprobe.offset, &uprobe.consumer);
+ uprobe.offset = 0;
+ }
+
+ mutex_unlock(&testmod_uprobe_mutex);
+}
+
+static ssize_t
+bpf_testmod_uprobe_write(struct file *file, struct kobject *kobj,
+ struct bin_attribute *bin_attr,
+ char *buf, loff_t off, size_t len)
+{
+ unsigned long offset;
+ int err;
+
+ if (kstrtoul(buf, 0, &offset))
+ return -EINVAL;
+
+ if (offset)
+ err = testmod_register_uprobe(offset);
+ else
+ testmod_unregister_uprobe();
+
+ return err ?: strlen(buf);
+}
+
+static struct bin_attribute bin_attr_bpf_testmod_uprobe_file __ro_after_init = {
+ .attr = { .name = "bpf_testmod_uprobe", .mode = 0666, },
+ .write = bpf_testmod_uprobe_write,
+};
+
+static int register_bpf_testmod_uprobe(void)
+{
+ return sysfs_create_bin_file(kernel_kobj, &bin_attr_bpf_testmod_uprobe_file);
+}
+
+static void unregister_bpf_testmod_uprobe(void)
+{
+ testmod_unregister_uprobe();
+ sysfs_remove_bin_file(kernel_kobj, &bin_attr_bpf_testmod_uprobe_file);
+}
+
+#else
+static int register_bpf_testmod_uprobe(void)
+{
+ return 0;
+}
+
+static void unregister_bpf_testmod_uprobe(void) { }
+#endif
+
BTF_KFUNCS_START(bpf_testmod_common_kfunc_ids)
BTF_ID_FLAGS(func, bpf_iter_testmod_seq_new, KF_ITER_NEW)
BTF_ID_FLAGS(func, bpf_iter_testmod_seq_next, KF_ITER_NEXT | KF_RET_NULL)
@@ -909,7 +1023,13 @@ static int bpf_testmod_init(void)
return -EINVAL;
sock = NULL;
mutex_init(&sock_lock);
- return sysfs_create_bin_file(kernel_kobj, &bin_attr_bpf_testmod_file);
+ ret = sysfs_create_bin_file(kernel_kobj, &bin_attr_bpf_testmod_file);
+ if (ret < 0)
+ return ret;
+ ret = register_bpf_testmod_uprobe();
+ if (ret < 0)
+ return ret;
+ return 0;
}
static void bpf_testmod_exit(void)
@@ -924,6 +1044,7 @@ static void bpf_testmod_exit(void)
bpf_kfunc_close_sock();
sysfs_remove_bin_file(kernel_kobj, &bin_attr_bpf_testmod_file);
+ unregister_bpf_testmod_uprobe();
}
module_init(bpf_testmod_init);
diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
index 311ac19d8992..1a50cd35205d 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
@@ -149,15 +149,82 @@ static void test_uretprobe_regs_equal(void)
cleanup:
uprobe_syscall__destroy(skel);
}
+
+#define BPF_TESTMOD_UPROBE_TEST_FILE "/sys/kernel/bpf_testmod_uprobe"
+
+static int write_bpf_testmod_uprobe(unsigned long offset)
+{
+ size_t n, ret;
+ char buf[30];
+ int fd;
+
+ n = sprintf(buf, "%lu", offset);
+
+ fd = open(BPF_TESTMOD_UPROBE_TEST_FILE, O_WRONLY);
+ if (fd < 0)
+ return -errno;
+
+ ret = write(fd, buf, n);
+ close(fd);
+ return ret != n ? (int) ret : 0;
+}
+
+static void test_uretprobe_regs_change(void)
+{
+ struct pt_regs before = {}, after = {};
+ unsigned long *pb = (unsigned long *) &before;
+ unsigned long *pa = (unsigned long *) &after;
+ unsigned long cnt = sizeof(before)/sizeof(*pb);
+ unsigned int i, err, offset;
+
+ offset = get_uprobe_offset(uretprobe_regs_trigger);
+
+ err = write_bpf_testmod_uprobe(offset);
+ if (!ASSERT_OK(err, "register_uprobe"))
+ return;
+
+ uretprobe_regs(&before, &after);
+
+ err = write_bpf_testmod_uprobe(0);
+ if (!ASSERT_OK(err, "unregister_uprobe"))
+ return;
+
+ for (i = 0; i < cnt; i++) {
+ unsigned int offset = i * sizeof(unsigned long);
+
+ switch (offset) {
+ case offsetof(struct pt_regs, rax):
+ ASSERT_EQ(pa[i], 0x12345678deadbeef, "rax");
+ break;
+ case offsetof(struct pt_regs, rcx):
+ ASSERT_EQ(pa[i], 0x87654321feebdaed, "rcx");
+ break;
+ case offsetof(struct pt_regs, r11):
+ ASSERT_EQ(pa[i], (__u64) -1, "r11");
+ break;
+ default:
+ if (!ASSERT_EQ(pa[i], pb[i], "register before-after value check"))
+ fprintf(stdout, "failed register offset %u\n", offset);
+ }
+ }
+}
+
#else
static void test_uretprobe_regs_equal(void)
{
test__skip();
}
+
+static void test_uretprobe_regs_change(void)
+{
+ test__skip();
+}
#endif
void test_uprobe_syscall(void)
{
if (test__start_subtest("uretprobe_regs_equal"))
test_uretprobe_regs_equal();
+ if (test__start_subtest("uretprobe_regs_change"))
+ test_uretprobe_regs_change();
}
--
2.44.0
^ permalink raw reply related
* [PATCHv5 bpf-next 5/8] selftests/bpf: Add uretprobe syscall call from user space test
From: Jiri Olsa @ 2024-05-07 10:53 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240507105321.71524-1-jolsa@kernel.org>
Adding test to verify that when called from outside of the
trampoline provided by kernel, the uretprobe syscall will cause
calling process to receive SIGILL signal and the attached bpf
program is not executed.
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
.../selftests/bpf/prog_tests/uprobe_syscall.c | 95 +++++++++++++++++++
.../bpf/progs/uprobe_syscall_executed.c | 17 ++++
2 files changed, 112 insertions(+)
create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall_executed.c
diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
index 1a50cd35205d..3ef324c2db50 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
@@ -7,7 +7,10 @@
#include <unistd.h>
#include <asm/ptrace.h>
#include <linux/compiler.h>
+#include <linux/stringify.h>
+#include <sys/wait.h>
#include "uprobe_syscall.skel.h"
+#include "uprobe_syscall_executed.skel.h"
__naked unsigned long uretprobe_regs_trigger(void)
{
@@ -209,6 +212,91 @@ static void test_uretprobe_regs_change(void)
}
}
+#ifndef __NR_uretprobe
+#define __NR_uretprobe 462
+#endif
+
+__naked unsigned long uretprobe_syscall_call_1(void)
+{
+ /*
+ * Pretend we are uretprobe trampoline to trigger the return
+ * probe invocation in order to verify we get SIGILL.
+ */
+ asm volatile (
+ "pushq %rax\n"
+ "pushq %rcx\n"
+ "pushq %r11\n"
+ "movq $" __stringify(__NR_uretprobe) ", %rax\n"
+ "syscall\n"
+ "popq %r11\n"
+ "popq %rcx\n"
+ "retq\n"
+ );
+}
+
+__naked unsigned long uretprobe_syscall_call(void)
+{
+ asm volatile (
+ "call uretprobe_syscall_call_1\n"
+ "retq\n"
+ );
+}
+
+static void test_uretprobe_syscall_call(void)
+{
+ LIBBPF_OPTS(bpf_uprobe_multi_opts, opts,
+ .retprobe = true,
+ );
+ struct uprobe_syscall_executed *skel;
+ int pid, status, err, go[2], c;
+
+ if (ASSERT_OK(pipe(go), "pipe"))
+ return;
+
+ skel = uprobe_syscall_executed__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "uprobe_syscall_executed__open_and_load"))
+ goto cleanup;
+
+ pid = fork();
+ if (!ASSERT_GE(pid, 0, "fork"))
+ goto cleanup;
+
+ /* child */
+ if (pid == 0) {
+ close(go[1]);
+
+ /* wait for parent's kick */
+ err = read(go[0], &c, 1);
+ if (err != 1)
+ exit(-1);
+
+ uretprobe_syscall_call();
+ _exit(0);
+ }
+
+ skel->links.test = bpf_program__attach_uprobe_multi(skel->progs.test, pid,
+ "/proc/self/exe",
+ "uretprobe_syscall_call", &opts);
+ if (!ASSERT_OK_PTR(skel->links.test, "bpf_program__attach_uprobe_multi"))
+ goto cleanup;
+
+ /* kick the child */
+ write(go[1], &c, 1);
+ err = waitpid(pid, &status, 0);
+ ASSERT_EQ(err, pid, "waitpid");
+
+ /* verify the child got killed with SIGILL */
+ ASSERT_EQ(WIFSIGNALED(status), 1, "WIFSIGNALED");
+ ASSERT_EQ(WTERMSIG(status), SIGILL, "WTERMSIG");
+
+ /* verify the uretprobe program wasn't called */
+ ASSERT_EQ(skel->bss->executed, 0, "executed");
+
+cleanup:
+ uprobe_syscall_executed__destroy(skel);
+ close(go[1]);
+ close(go[0]);
+}
#else
static void test_uretprobe_regs_equal(void)
{
@@ -219,6 +307,11 @@ static void test_uretprobe_regs_change(void)
{
test__skip();
}
+
+static void test_uretprobe_syscall_call(void)
+{
+ test__skip();
+}
#endif
void test_uprobe_syscall(void)
@@ -227,4 +320,6 @@ void test_uprobe_syscall(void)
test_uretprobe_regs_equal();
if (test__start_subtest("uretprobe_regs_change"))
test_uretprobe_regs_change();
+ if (test__start_subtest("uretprobe_syscall_call"))
+ test_uretprobe_syscall_call();
}
diff --git a/tools/testing/selftests/bpf/progs/uprobe_syscall_executed.c b/tools/testing/selftests/bpf/progs/uprobe_syscall_executed.c
new file mode 100644
index 000000000000..0d7f1a7db2e2
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/uprobe_syscall_executed.c
@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <string.h>
+
+struct pt_regs regs;
+
+char _license[] SEC("license") = "GPL";
+
+int executed = 0;
+
+SEC("uretprobe.multi")
+int test(struct pt_regs *regs)
+{
+ executed = 1;
+ return 0;
+}
--
2.44.0
^ permalink raw reply related
* [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Jiri Olsa @ 2024-05-07 10:53 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240507105321.71524-1-jolsa@kernel.org>
Adding return uprobe support to work with enabled shadow stack.
Currently the application with enabled shadow stack will crash
if it sets up return uprobe. The reason is the uretprobe kernel
code changes the user space task's stack, but does not update
shadow stack accordingly.
Adding new functions to update values on shadow stack and using
them in uprobe code to keep shadow stack in sync with uretprobe
changes to user stack.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/include/asm/shstk.h | 4 ++++
arch/x86/kernel/shstk.c | 29 +++++++++++++++++++++++++++++
arch/x86/kernel/uprobes.c | 12 +++++++++++-
3 files changed, 44 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/shstk.h b/arch/x86/include/asm/shstk.h
index 42fee8959df7..2e1ddcf98242 100644
--- a/arch/x86/include/asm/shstk.h
+++ b/arch/x86/include/asm/shstk.h
@@ -21,6 +21,8 @@ unsigned long shstk_alloc_thread_stack(struct task_struct *p, unsigned long clon
void shstk_free(struct task_struct *p);
int setup_signal_shadow_stack(struct ksignal *ksig);
int restore_signal_shadow_stack(void);
+int shstk_update_last_frame(unsigned long val);
+int shstk_push_frame(unsigned long val);
#else
static inline long shstk_prctl(struct task_struct *task, int option,
unsigned long arg2) { return -EINVAL; }
@@ -31,6 +33,8 @@ static inline unsigned long shstk_alloc_thread_stack(struct task_struct *p,
static inline void shstk_free(struct task_struct *p) {}
static inline int setup_signal_shadow_stack(struct ksignal *ksig) { return 0; }
static inline int restore_signal_shadow_stack(void) { return 0; }
+static inline int shstk_update_last_frame(unsigned long val) { return 0; }
+static inline int shstk_push_frame(unsigned long val) { return 0; }
#endif /* CONFIG_X86_USER_SHADOW_STACK */
#endif /* __ASSEMBLY__ */
diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
index 59e15dd8d0f8..66434dfde52e 100644
--- a/arch/x86/kernel/shstk.c
+++ b/arch/x86/kernel/shstk.c
@@ -577,3 +577,32 @@ long shstk_prctl(struct task_struct *task, int option, unsigned long arg2)
return wrss_control(true);
return -EINVAL;
}
+
+int shstk_update_last_frame(unsigned long val)
+{
+ unsigned long ssp;
+
+ if (!features_enabled(ARCH_SHSTK_SHSTK))
+ return 0;
+
+ ssp = get_user_shstk_addr();
+ return write_user_shstk_64((u64 __user *)ssp, (u64)val);
+}
+
+int shstk_push_frame(unsigned long val)
+{
+ unsigned long ssp;
+
+ if (!features_enabled(ARCH_SHSTK_SHSTK))
+ return 0;
+
+ ssp = get_user_shstk_addr();
+ ssp -= SS_FRAME_SIZE;
+ if (write_user_shstk_64((u64 __user *)ssp, (u64)val))
+ return -EFAULT;
+
+ fpregs_lock_and_load();
+ wrmsrl(MSR_IA32_PL3_SSP, ssp);
+ fpregs_unlock();
+ return 0;
+}
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 81e6ee95784d..ae6c3458a675 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -406,6 +406,11 @@ SYSCALL_DEFINE0(uretprobe)
* trampoline's ret instruction
*/
r11_cx_ax[2] = regs->ip;
+
+ /* make the shadow stack follow that */
+ if (shstk_push_frame(regs->ip))
+ goto sigill;
+
regs->ip = ip;
err = copy_to_user((void __user *)regs->sp, r11_cx_ax, sizeof(r11_cx_ax));
@@ -1191,8 +1196,13 @@ arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, struct pt_regs
return orig_ret_vaddr;
nleft = copy_to_user((void __user *)regs->sp, &trampoline_vaddr, rasize);
- if (likely(!nleft))
+ if (likely(!nleft)) {
+ if (shstk_update_last_frame(trampoline_vaddr)) {
+ force_sig(SIGSEGV);
+ return -1;
+ }
return orig_ret_vaddr;
+ }
if (nleft != rasize) {
pr_err("return address clobbered: pid=%d, %%sp=%#lx, %%ip=%#lx\n",
--
2.44.0
^ permalink raw reply related
* [PATCHv5 bpf-next 7/8] selftests/x86: Add return uprobe shadow stack test
From: Jiri Olsa @ 2024-05-07 10:53 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240507105321.71524-1-jolsa@kernel.org>
Adding return uprobe test for shadow stack and making sure it's
working properly. Borrowed some of the code from bpf selftests.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
.../testing/selftests/x86/test_shadow_stack.c | 142 ++++++++++++++++++
1 file changed, 142 insertions(+)
diff --git a/tools/testing/selftests/x86/test_shadow_stack.c b/tools/testing/selftests/x86/test_shadow_stack.c
index 757e6527f67e..1b919baa999b 100644
--- a/tools/testing/selftests/x86/test_shadow_stack.c
+++ b/tools/testing/selftests/x86/test_shadow_stack.c
@@ -34,6 +34,7 @@
#include <sys/ptrace.h>
#include <sys/signal.h>
#include <linux/elf.h>
+#include <linux/perf_event.h>
/*
* Define the ABI defines if needed, so people can run the tests
@@ -681,6 +682,141 @@ int test_32bit(void)
return !segv_triggered;
}
+static int parse_uint_from_file(const char *file, const char *fmt)
+{
+ int err, ret;
+ FILE *f;
+
+ f = fopen(file, "re");
+ if (!f) {
+ err = -errno;
+ printf("failed to open '%s': %d\n", file, err);
+ return err;
+ }
+ err = fscanf(f, fmt, &ret);
+ if (err != 1) {
+ err = err == EOF ? -EIO : -errno;
+ printf("failed to parse '%s': %d\n", file, err);
+ fclose(f);
+ return err;
+ }
+ fclose(f);
+ return ret;
+}
+
+static int determine_uprobe_perf_type(void)
+{
+ const char *file = "/sys/bus/event_source/devices/uprobe/type";
+
+ return parse_uint_from_file(file, "%d\n");
+}
+
+static int determine_uprobe_retprobe_bit(void)
+{
+ const char *file = "/sys/bus/event_source/devices/uprobe/format/retprobe";
+
+ return parse_uint_from_file(file, "config:%d\n");
+}
+
+static ssize_t get_uprobe_offset(const void *addr)
+{
+ size_t start, end, base;
+ char buf[256];
+ bool found = false;
+ FILE *f;
+
+ f = fopen("/proc/self/maps", "r");
+ if (!f)
+ return -errno;
+
+ while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &base) == 4) {
+ if (buf[2] == 'x' && (uintptr_t)addr >= start && (uintptr_t)addr < end) {
+ found = true;
+ break;
+ }
+ }
+
+ fclose(f);
+
+ if (!found)
+ return -ESRCH;
+
+ return (uintptr_t)addr - start + base;
+}
+
+static __attribute__((noinline)) void uretprobe_trigger(void)
+{
+ asm volatile ("");
+}
+
+/*
+ * This test setups return uprobe, which is sensitive to shadow stack
+ * (crashes without extra fix). After executing the uretprobe we fail
+ * the test if we receive SIGSEGV, no crash means we're good.
+ *
+ * Helper functions above borrowed from bpf selftests.
+ */
+static int test_uretprobe(void)
+{
+ const size_t attr_sz = sizeof(struct perf_event_attr);
+ const char *file = "/proc/self/exe";
+ int bit, fd = 0, type, err = 1;
+ struct perf_event_attr attr;
+ struct sigaction sa = {};
+ ssize_t offset;
+
+ type = determine_uprobe_perf_type();
+ if (type < 0)
+ return 1;
+
+ offset = get_uprobe_offset(uretprobe_trigger);
+ if (offset < 0)
+ return 1;
+
+ bit = determine_uprobe_retprobe_bit();
+ if (bit < 0)
+ return 1;
+
+ sa.sa_sigaction = segv_gp_handler;
+ sa.sa_flags = SA_SIGINFO;
+ if (sigaction(SIGSEGV, &sa, NULL))
+ return 1;
+
+ /* Setup return uprobe through perf event interface. */
+ memset(&attr, 0, attr_sz);
+ attr.size = attr_sz;
+ attr.type = type;
+ attr.config = 1 << bit;
+ attr.config1 = (__u64) (unsigned long) file;
+ attr.config2 = offset;
+
+ fd = syscall(__NR_perf_event_open, &attr, 0 /* pid */, -1 /* cpu */,
+ -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
+ if (fd < 0)
+ goto out;
+
+ if (sigsetjmp(jmp_buffer, 1))
+ goto out;
+
+ ARCH_PRCTL(ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK);
+
+ /*
+ * This either segfaults and goes through sigsetjmp above
+ * or succeeds and we're good.
+ */
+ uretprobe_trigger();
+
+ printf("[OK]\tUretprobe test\n");
+ err = 0;
+
+out:
+ ARCH_PRCTL(ARCH_SHSTK_DISABLE, ARCH_SHSTK_SHSTK);
+ signal(SIGSEGV, SIG_DFL);
+ if (fd)
+ close(fd);
+ return err;
+}
+
void segv_handler_ptrace(int signum, siginfo_t *si, void *uc)
{
/* The SSP adjustment caused a segfault. */
@@ -867,6 +1003,12 @@ int main(int argc, char *argv[])
goto out;
}
+ if (test_uretprobe()) {
+ ret = 1;
+ printf("[FAIL]\turetprobe test\n");
+ goto out;
+ }
+
return ret;
out:
--
2.44.0
^ permalink raw reply related
* [PATCHv5 8/8] man2: Add uretprobe syscall page
From: Jiri Olsa @ 2024-05-07 10:53 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240507105321.71524-1-jolsa@kernel.org>
Adding man page for new uretprobe syscall.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
man2/uretprobe.2 | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)
create mode 100644 man2/uretprobe.2
diff --git a/man2/uretprobe.2 b/man2/uretprobe.2
new file mode 100644
index 000000000000..690fe3b1a44f
--- /dev/null
+++ b/man2/uretprobe.2
@@ -0,0 +1,50 @@
+.\" Copyright (C) 2024, Jiri Olsa <jolsa@kernel.org>
+.\"
+.\" SPDX-License-Identifier: Linux-man-pages-copyleft
+.\"
+.TH uretprobe 2 (date) "Linux man-pages (unreleased)"
+.SH NAME
+uretprobe \- execute pending return uprobes
+.SH SYNOPSIS
+.nf
+.B int uretprobe(void)
+.fi
+.SH DESCRIPTION
+The
+.BR uretprobe ()
+syscall is an alternative to breakpoint instructions for
+triggering return uprobe consumers.
+.P
+Calls to
+.BR uretprobe ()
+suscall are only made from the user-space trampoline provided by the kernel.
+Calls from any other place result in a
+.BR SIGILL .
+
+.SH RETURN VALUE
+The
+.BR uretprobe ()
+syscall return value is architecture-specific.
+
+.SH VERSIONS
+This syscall is not specified in POSIX,
+and details of its behavior vary across systems.
+.SH STANDARDS
+None.
+.SH HISTORY
+TBD
+.SH NOTES
+The
+.BR uretprobe ()
+syscall was initially introduced for the x86_64 architecture where it was shown
+to be faster than breakpoint traps. It might be extended to other architectures.
+.P
+The
+.BR uretprobe ()
+syscall exists only to allow the invocation of return uprobe consumers.
+It should
+.B never
+be called directly.
+Details of the arguments (if any) passed to
+.BR uretprobe ()
+and the return value are architecture-specific.
--
2.44.0
^ permalink raw reply related
* Re: [PATCH v6 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: Aleksa Sarai @ 2024-05-07 11:58 UTC (permalink / raw)
To: stsp
Cc: linux-kernel, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, David Laight, linux-fsdevel,
linux-api, Paolo Bonzini, Christian Göttsche
In-Reply-To: <5b5cc31f-a5be-4f64-a97b-7708466ace82@yandex.ru>
[-- Attachment #1: Type: text/plain, Size: 1758 bytes --]
On 2024-05-07, stsp <stsp2@yandex.ru> wrote:
> 07.05.2024 10:50, Aleksa Sarai пишет:
> > If you are a privileged process which plans to change users,
>
> Not privileged at all. But I think what you say is still possible with
> userns?
It is possible to configure MOUNT_ATTR_IDMAP in a user namespace but
there are some restrictions that I suspect will make this complicated.
If you try to do something with a regular filesystem you'll probably run
into issues because you won't have CAP_SYS_ADMIN in the super block's
userns. But you could probably do it with tmpfs.
> > A new attack I just thought of while writing this mail is that because
> > there is no RESOLVE_NO_XDEV requirement, it should be possible for the
> > process to get an arbitrary write primitive by creating a new
> > userns+mountns and then bind-mounting / underneath the directory.
> Doesn't this need a write perm to a
> directory? In his case this is not a threat,
> because you are not supposed to have a
> write perm to that dir. OA2_CRED_INHERIT
> is the only way to write.
No, bind-mounts don't require write permission. As long as you can
resolve the target path you can bind-mount on top of it, so if there's a
subdirectory you can bind-mount / underneath (and if there is only a
file you can bind-mount any file you want to access/overwrite instead).
There are restrictions on mounting through /proc/self/fd/... but they
don't apply here (all files opened by a process doing setns/unshare have
their vfsmounts updated to be from the new mount namespace, meaning you
can do mounts through them with /proc/self/fd/... without issue.)
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v6 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: stsp @ 2024-05-07 12:48 UTC (permalink / raw)
To: Aleksa Sarai
Cc: linux-kernel, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, David Laight, linux-fsdevel,
linux-api, Paolo Bonzini, Christian Göttsche
In-Reply-To: <20240507.110127-muggy.duff.trained.hobby-u9ZNUZ9CW5k@cyphar.com>
07.05.2024 14:58, Aleksa Sarai пишет:
> On 2024-05-07, stsp <stsp2@yandex.ru> wrote:
>> 07.05.2024 10:50, Aleksa Sarai пишет:
>>> If you are a privileged process which plans to change users,
>> Not privileged at all. But I think what you say is still possible with
>> userns?
> It is possible to configure MOUNT_ATTR_IDMAP in a user namespace but
> there are some restrictions that I suspect will make this complicated.
> If you try to do something with a regular filesystem you'll probably run
> into issues because you won't have CAP_SYS_ADMIN in the super block's
> userns. But you could probably do it with tmpfs.
Then its likely not a replacement for
my proposal, as I really don't need that
on tmpfs.
Perhaps right now I can use the helper
process and an rpc as a replacement.
This is much more work and is slower,
but more or less can approximate my
original design decision quite precisely.
Another disadvantage of an rpc approach
is that the fds I get from the helper
process, can not be trusted, as in this
case kernel doesn't guarantee the fd
actually refers to the resource I requested.
I've seen a few OSes where rpc is checked
by a trusted entity to avoid such problem.
>>> A new attack I just thought of while writing this mail is that because
>>> there is no RESOLVE_NO_XDEV requirement, it should be possible for the
>>> process to get an arbitrary write primitive by creating a new
>>> userns+mountns and then bind-mounting / underneath the directory.
>> Doesn't this need a write perm to a
>> directory? In his case this is not a threat,
>> because you are not supposed to have a
>> write perm to that dir. OA2_CRED_INHERIT
>> is the only way to write.
> No, bind-mounts don't require write permission.
Oh, isn't this a problem by itself?
Yes, in this case my patch needs to
avoid RESOLVE_NO_XDEV, but I find this a harsh restriction. Maybe the
bind mount was done before a priv drop? Then it is fully legitimate.
Anyway, I don't know if I should work on it or not, as there seem to be
no indication of a possible acceptance.
^ permalink raw reply
* Re: [PATCHv5 bpf-next 5/8] selftests/bpf: Add uretprobe syscall call from user space test
From: Andrii Nakryiko @ 2024-05-07 16:57 UTC (permalink / raw)
To: Jiri Olsa
Cc: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240507105321.71524-6-jolsa@kernel.org>
On Tue, May 7, 2024 at 3:54 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding test to verify that when called from outside of the
> trampoline provided by kernel, the uretprobe syscall will cause
> calling process to receive SIGILL signal and the attached bpf
> program is not executed.
>
> Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> .../selftests/bpf/prog_tests/uprobe_syscall.c | 95 +++++++++++++++++++
> .../bpf/progs/uprobe_syscall_executed.c | 17 ++++
> 2 files changed, 112 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall_executed.c
>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
> diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> index 1a50cd35205d..3ef324c2db50 100644
> --- a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> @@ -7,7 +7,10 @@
> #include <unistd.h>
> #include <asm/ptrace.h>
> #include <linux/compiler.h>
> +#include <linux/stringify.h>
> +#include <sys/wait.h>
> #include "uprobe_syscall.skel.h"
> +#include "uprobe_syscall_executed.skel.h"
>
[...]
^ permalink raw reply
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Edgecombe, Rick P @ 2024-05-07 17:35 UTC (permalink / raw)
To: jolsa@kernel.org, mhiramat@kernel.org, rostedt@goodmis.org,
ast@kernel.org, andrii@kernel.org, oleg@redhat.com,
daniel@iogearbox.net
Cc: debug@rivosinc.com, luto@kernel.org, bp@alien8.de, yhs@fb.com,
songliubraving@fb.com, linux-api@vger.kernel.org, x86@kernel.org,
linux-kernel@vger.kernel.org, john.fastabend@gmail.com,
tglx@linutronix.de, mingo@redhat.com,
linux-trace-kernel@vger.kernel.org, bpf@vger.kernel.org,
linux-man@vger.kernel.org, peterz@infradead.org
In-Reply-To: <20240507105321.71524-7-jolsa@kernel.org>
On Tue, 2024-05-07 at 12:53 +0200, Jiri Olsa wrote:
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 81e6ee95784d..ae6c3458a675 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -406,6 +406,11 @@ SYSCALL_DEFINE0(uretprobe)
> * trampoline's ret instruction
> */
> r11_cx_ax[2] = regs->ip;
> +
> + /* make the shadow stack follow that */
> + if (shstk_push_frame(regs->ip))
> + goto sigill;
> +
> regs->ip = ip;
>
Per the earlier discussion, this cannot be reached unless uretprobes are in use,
which cannot happen without something with privileges taking an action. But are
uretprobes ever used for monitoring applications where security is important? Or
is it strictly a debug-time thing?
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: Andy Lutomirski @ 2024-05-07 20:38 UTC (permalink / raw)
To: Christian Brauner
Cc: Aleksa Sarai, Stas Sergeev, Serge E. Hallyn, linux-kernel,
Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <20240507-verpennen-defekt-b6f2c9a46916@brauner>
On Tue, May 7, 2024 at 12:42 AM Christian Brauner <brauner@kernel.org> wrote:
>
> > With my kernel hat on, maybe I agree. But with my *user* hat on, I
> > think I pretty strongly disagree. Look, idmapis lousy for
> > unprivileged use:
> >
> > $ install -m 0700 -d test_directory
> > $ echo 'hi there' >test_directory/file
> > $ podman run -it --rm
> > --mount=type=bind,src=test_directory,dst=/tmp,idmap [debian-slim]
>
> $ podman run -it --rm --mount=type=bind,src=test_directory,dst=/tmp,idmap [debian-slim]
>
> as an unprivileged user doesn't use idmapped mounts at all. So I'm not
> sure what this is showing. I suppose you're talking about idmaps in
> general.
Meh, fair enough. But I don't think this would have worked any better
with privilege.
Can idmaps be programmed by an otherwise unprivileged owner of a
userns and a mountns inside?
> Many idmappings to one is in principle possible and I've noted that idea
> down as a possible extension at
> https://github.com/uapi-group/kernel-features quite a while (2 years?) ago.
>
> > I haven't looked at the idmap implementation nearly enough to have any
> > opinion as to whether squashing UID is practical or whether there's
>
> It's doable. The interesting bit to me was that if we want to allow
> writes we'd need a way to determine what the uid/gid would be to write
> down. Imho, that's not super difficult to solve though. The most obvious
> one is that userspace can just determine it when creating the idmapped
> mount.
Seems reasonable to me. If this is set up by someone unprivileged, it
would need to be that uid/gid.
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: Christian Brauner @ 2024-05-08 7:32 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Aleksa Sarai, Stas Sergeev, Serge E. Hallyn, linux-kernel,
Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <CALCETrWuVQ-ggnak40AX16PUnM43zhogceFN-3c_YAKZGvs5Og@mail.gmail.com>
On Tue, May 07, 2024 at 01:38:42PM -0700, Andy Lutomirski wrote:
> On Tue, May 7, 2024 at 12:42 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> > > With my kernel hat on, maybe I agree. But with my *user* hat on, I
> > > think I pretty strongly disagree. Look, idmapis lousy for
> > > unprivileged use:
> > >
> > > $ install -m 0700 -d test_directory
> > > $ echo 'hi there' >test_directory/file
> > > $ podman run -it --rm
> > > --mount=type=bind,src=test_directory,dst=/tmp,idmap [debian-slim]
> >
> > $ podman run -it --rm --mount=type=bind,src=test_directory,dst=/tmp,idmap [debian-slim]
> >
> > as an unprivileged user doesn't use idmapped mounts at all. So I'm not
> > sure what this is showing. I suppose you're talking about idmaps in
> > general.
>
> Meh, fair enough. But I don't think this would have worked any better
> with privilege.
>
> Can idmaps be programmed by an otherwise unprivileged owner of a
> userns and a mountns inside?
Yes, but only for userns mountable filesystems that support idmapped
mounts. IOW, you need privilege over the superblock and the idmapping
you're trying to use.
>
> > Many idmappings to one is in principle possible and I've noted that idea
> > down as a possible extension at
> > https://github.com/uapi-group/kernel-features quite a while (2 years?) ago.
> >
> > > I haven't looked at the idmap implementation nearly enough to have any
> > > opinion as to whether squashing UID is practical or whether there's
> >
> > It's doable. The interesting bit to me was that if we want to allow
> > writes we'd need a way to determine what the uid/gid would be to write
> > down. Imho, that's not super difficult to solve though. The most obvious
> > one is that userspace can just determine it when creating the idmapped
> > mount.
>
> Seems reasonable to me. If this is set up by someone unprivileged, it
> would need to be that uid/gid.
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: Andy Lutomirski @ 2024-05-08 17:30 UTC (permalink / raw)
To: Christian Brauner
Cc: Aleksa Sarai, Stas Sergeev, Serge E. Hallyn, linux-kernel,
Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <20240508-flugverbindung-sonnig-dcfa4971152e@brauner>
On Wed, May 8, 2024 at 12:32 AM Christian Brauner <brauner@kernel.org> wrote:
>
> On Tue, May 07, 2024 at 01:38:42PM -0700, Andy Lutomirski wrote:
> > On Tue, May 7, 2024 at 12:42 AM Christian Brauner <brauner@kernel.org> wrote:
> > >
> > > > With my kernel hat on, maybe I agree. But with my *user* hat on, I
> > > > think I pretty strongly disagree. Look, idmapis lousy for
> > > > unprivileged use:
> > > >
> > > > $ install -m 0700 -d test_directory
> > > > $ echo 'hi there' >test_directory/file
> > > > $ podman run -it --rm
> > > > --mount=type=bind,src=test_directory,dst=/tmp,idmap [debian-slim]
> > >
> > > $ podman run -it --rm --mount=type=bind,src=test_directory,dst=/tmp,idmap [debian-slim]
> > >
> > > as an unprivileged user doesn't use idmapped mounts at all. So I'm not
> > > sure what this is showing. I suppose you're talking about idmaps in
> > > general.
> >
> > Meh, fair enough. But I don't think this would have worked any better
> > with privilege.
> >
> > Can idmaps be programmed by an otherwise unprivileged owner of a
> > userns and a mountns inside?
>
> Yes, but only for userns mountable filesystems that support idmapped
> mounts. IOW, you need privilege over the superblock and the idmapping
> you're trying to use.
Hmm. Is there a good reason to require privilege over the superblock?
Obviously creating an idmap that allows one to impersonate someone
else seems like a problem, but if an unprivileged task already "owns"
(see below) a UID or GID, then effectively delegating that UID or GID
is would need caution but is not fundamentally terrible.
So, if I'm 1000:1000, then creating an idmap that makes some other
task (that isn't 1000:1000) get to act as 1000:1000 doesn't grant new
powers. But maybe something even more general could be done (although
I'm not sure this is worthwhile): if I own a userns and that userns
has an outside UID 1001 mapped (via newuidmap, for example), then
perhaps letting me configure an idmap that grants UID 1001 seems not
especially dangerous. But maybe that particular job should also be
delegated to newuidmap.
Out of an abundance of caution, maybe this whole thing should be
opt-in. For example, there could be a new CAP_DELEGATE that allows
delegation of one's own uid and gid. The idea is that it should be
safe to grant regular users CAP_DELEGATE as an ambient capability.
--Andy
^ permalink raw reply
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Jiri Olsa @ 2024-05-09 8:30 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: mhiramat@kernel.org, rostedt@goodmis.org, ast@kernel.org,
andrii@kernel.org, oleg@redhat.com, daniel@iogearbox.net,
debug@rivosinc.com, luto@kernel.org, bp@alien8.de, yhs@fb.com,
songliubraving@fb.com, linux-api@vger.kernel.org, x86@kernel.org,
linux-kernel@vger.kernel.org, john.fastabend@gmail.com,
tglx@linutronix.de, mingo@redhat.com,
linux-trace-kernel@vger.kernel.org, bpf@vger.kernel.org,
linux-man@vger.kernel.org, peterz@infradead.org
In-Reply-To: <a08a955c74682e9dc6eb6d49b91c6968c9b62f75.camel@intel.com>
On Tue, May 07, 2024 at 05:35:54PM +0000, Edgecombe, Rick P wrote:
> On Tue, 2024-05-07 at 12:53 +0200, Jiri Olsa wrote:
> > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> > index 81e6ee95784d..ae6c3458a675 100644
> > --- a/arch/x86/kernel/uprobes.c
> > +++ b/arch/x86/kernel/uprobes.c
> > @@ -406,6 +406,11 @@ SYSCALL_DEFINE0(uretprobe)
> > * trampoline's ret instruction
> > */
> > r11_cx_ax[2] = regs->ip;
> > +
> > + /* make the shadow stack follow that */
> > + if (shstk_push_frame(regs->ip))
> > + goto sigill;
> > +
> > regs->ip = ip;
> >
>
> Per the earlier discussion, this cannot be reached unless uretprobes are in use,
> which cannot happen without something with privileges taking an action. But are
> uretprobes ever used for monitoring applications where security is important? Or
> is it strictly a debug-time thing?
sorry, I don't have that level of detail, but we do have customers
that use uprobes in general or want to use it and complain about
the speed
there are several tools in bcc [1] that use uretprobes in scripts,
like:
memleak, sslsniff, trace, bashreadline, gethostlatency, argdist,
funclatency
jirka
[1] https://github.com/iovisor/bcc
^ permalink raw reply
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Edgecombe, Rick P @ 2024-05-09 16:24 UTC (permalink / raw)
To: olsajiri@gmail.com
Cc: songliubraving@fb.com, luto@kernel.org, mhiramat@kernel.org,
andrii@kernel.org, debug@rivosinc.com, john.fastabend@gmail.com,
linux-api@vger.kernel.org, linux-kernel@vger.kernel.org,
mingo@redhat.com, rostedt@goodmis.org, ast@kernel.org,
tglx@linutronix.de, yhs@fb.com, oleg@redhat.com,
peterz@infradead.org, linux-man@vger.kernel.org,
daniel@iogearbox.net, linux-trace-kernel@vger.kernel.org,
bp@alien8.de, bpf@vger.kernel.org, x86@kernel.org
In-Reply-To: <ZjyJsl_u_FmYHrki@krava>
On Thu, 2024-05-09 at 10:30 +0200, Jiri Olsa wrote:
> > Per the earlier discussion, this cannot be reached unless uretprobes are in
> > use,
> > which cannot happen without something with privileges taking an action. But
> > are
> > uretprobes ever used for monitoring applications where security is
> > important? Or
> > is it strictly a debug-time thing?
>
> sorry, I don't have that level of detail, but we do have customers
> that use uprobes in general or want to use it and complain about
> the speed
>
> there are several tools in bcc [1] that use uretprobes in scripts,
> like:
> memleak, sslsniff, trace, bashreadline, gethostlatency, argdist,
> funclatency
Is it possible to have shadow stack only use the non-syscall solution? It seems
it exposes a more limited compatibility in that it only allows writing the
specific trampoline address. (IIRC) Then shadow stack users could still use
uretprobes, but just not the new optimized solution. There are already
operations that are slower with shadow stack, like longjmp(), so this could be
ok maybe.
^ permalink raw reply
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Jiri Olsa @ 2024-05-11 21:09 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: olsajiri@gmail.com, songliubraving@fb.com, luto@kernel.org,
mhiramat@kernel.org, andrii@kernel.org, debug@rivosinc.com,
john.fastabend@gmail.com, linux-api@vger.kernel.org,
linux-kernel@vger.kernel.org, mingo@redhat.com,
rostedt@goodmis.org, ast@kernel.org, tglx@linutronix.de,
yhs@fb.com, oleg@redhat.com, peterz@infradead.org,
linux-man@vger.kernel.org, daniel@iogearbox.net,
linux-trace-kernel@vger.kernel.org, bp@alien8.de,
bpf@vger.kernel.org, x86@kernel.org
In-Reply-To: <a8b7be15e6dbb1e8f2acaee7dae21fec7775194c.camel@intel.com>
On Thu, May 09, 2024 at 04:24:37PM +0000, Edgecombe, Rick P wrote:
> On Thu, 2024-05-09 at 10:30 +0200, Jiri Olsa wrote:
> > > Per the earlier discussion, this cannot be reached unless uretprobes are in
> > > use,
> > > which cannot happen without something with privileges taking an action. But
> > > are
> > > uretprobes ever used for monitoring applications where security is
> > > important? Or
> > > is it strictly a debug-time thing?
> >
> > sorry, I don't have that level of detail, but we do have customers
> > that use uprobes in general or want to use it and complain about
> > the speed
> >
> > there are several tools in bcc [1] that use uretprobes in scripts,
> > like:
> > memleak, sslsniff, trace, bashreadline, gethostlatency, argdist,
> > funclatency
>
> Is it possible to have shadow stack only use the non-syscall solution? It seems
> it exposes a more limited compatibility in that it only allows writing the
> specific trampoline address. (IIRC) Then shadow stack users could still use
> uretprobes, but just not the new optimized solution. There are already
> operations that are slower with shadow stack, like longjmp(), so this could be
> ok maybe.
I guess it's doable, we'd need to keep both trampolines around, because
shadow stack is enabled by app dynamically and use one based on the
state of shadow stack when uretprobe is installed
so you're worried the optimized syscall path could be somehow exploited
to add data on shadow stack?
jirka
^ permalink raw reply
* Re: [PATCHv5 bpf-next 7/8] selftests/x86: Add return uprobe shadow stack test
From: Masami Hiramatsu @ 2024-05-13 9:45 UTC (permalink / raw)
To: Jiri Olsa
Cc: Steven Rostedt, Oleg Nesterov, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, linux-kernel,
linux-trace-kernel, linux-api, linux-man, x86, bpf, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), Ingo Molnar, Andy Lutomirski,
Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240507105321.71524-8-jolsa@kernel.org>
On Tue, 7 May 2024 12:53:20 +0200
Jiri Olsa <jolsa@kernel.org> wrote:
> Adding return uprobe test for shadow stack and making sure it's
> working properly. Borrowed some of the code from bpf selftests.
Hi Jiri,
I can not find "SKIP" result in this change. If CONFIG_UPROBES=n,
this should skip uprobe test.
Thank you,
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> .../testing/selftests/x86/test_shadow_stack.c | 142 ++++++++++++++++++
> 1 file changed, 142 insertions(+)
>
> diff --git a/tools/testing/selftests/x86/test_shadow_stack.c b/tools/testing/selftests/x86/test_shadow_stack.c
> index 757e6527f67e..1b919baa999b 100644
> --- a/tools/testing/selftests/x86/test_shadow_stack.c
> +++ b/tools/testing/selftests/x86/test_shadow_stack.c
> @@ -34,6 +34,7 @@
> #include <sys/ptrace.h>
> #include <sys/signal.h>
> #include <linux/elf.h>
> +#include <linux/perf_event.h>
>
> /*
> * Define the ABI defines if needed, so people can run the tests
> @@ -681,6 +682,141 @@ int test_32bit(void)
> return !segv_triggered;
> }
>
> +static int parse_uint_from_file(const char *file, const char *fmt)
> +{
> + int err, ret;
> + FILE *f;
> +
> + f = fopen(file, "re");
> + if (!f) {
> + err = -errno;
> + printf("failed to open '%s': %d\n", file, err);
> + return err;
> + }
> + err = fscanf(f, fmt, &ret);
> + if (err != 1) {
> + err = err == EOF ? -EIO : -errno;
> + printf("failed to parse '%s': %d\n", file, err);
> + fclose(f);
> + return err;
> + }
> + fclose(f);
> + return ret;
> +}
> +
> +static int determine_uprobe_perf_type(void)
> +{
> + const char *file = "/sys/bus/event_source/devices/uprobe/type";
> +
> + return parse_uint_from_file(file, "%d\n");
> +}
> +
> +static int determine_uprobe_retprobe_bit(void)
> +{
> + const char *file = "/sys/bus/event_source/devices/uprobe/format/retprobe";
> +
> + return parse_uint_from_file(file, "config:%d\n");
> +}
> +
> +static ssize_t get_uprobe_offset(const void *addr)
> +{
> + size_t start, end, base;
> + char buf[256];
> + bool found = false;
> + FILE *f;
> +
> + f = fopen("/proc/self/maps", "r");
> + if (!f)
> + return -errno;
> +
> + while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &base) == 4) {
> + if (buf[2] == 'x' && (uintptr_t)addr >= start && (uintptr_t)addr < end) {
> + found = true;
> + break;
> + }
> + }
> +
> + fclose(f);
> +
> + if (!found)
> + return -ESRCH;
> +
> + return (uintptr_t)addr - start + base;
> +}
> +
> +static __attribute__((noinline)) void uretprobe_trigger(void)
> +{
> + asm volatile ("");
> +}
> +
> +/*
> + * This test setups return uprobe, which is sensitive to shadow stack
> + * (crashes without extra fix). After executing the uretprobe we fail
> + * the test if we receive SIGSEGV, no crash means we're good.
> + *
> + * Helper functions above borrowed from bpf selftests.
> + */
> +static int test_uretprobe(void)
> +{
> + const size_t attr_sz = sizeof(struct perf_event_attr);
> + const char *file = "/proc/self/exe";
> + int bit, fd = 0, type, err = 1;
> + struct perf_event_attr attr;
> + struct sigaction sa = {};
> + ssize_t offset;
> +
> + type = determine_uprobe_perf_type();
> + if (type < 0)
> + return 1;
> +
> + offset = get_uprobe_offset(uretprobe_trigger);
> + if (offset < 0)
> + return 1;
> +
> + bit = determine_uprobe_retprobe_bit();
> + if (bit < 0)
> + return 1;
> +
> + sa.sa_sigaction = segv_gp_handler;
> + sa.sa_flags = SA_SIGINFO;
> + if (sigaction(SIGSEGV, &sa, NULL))
> + return 1;
> +
> + /* Setup return uprobe through perf event interface. */
> + memset(&attr, 0, attr_sz);
> + attr.size = attr_sz;
> + attr.type = type;
> + attr.config = 1 << bit;
> + attr.config1 = (__u64) (unsigned long) file;
> + attr.config2 = offset;
> +
> + fd = syscall(__NR_perf_event_open, &attr, 0 /* pid */, -1 /* cpu */,
> + -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
> + if (fd < 0)
> + goto out;
> +
> + if (sigsetjmp(jmp_buffer, 1))
> + goto out;
> +
> + ARCH_PRCTL(ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK);
> +
> + /*
> + * This either segfaults and goes through sigsetjmp above
> + * or succeeds and we're good.
> + */
> + uretprobe_trigger();
> +
> + printf("[OK]\tUretprobe test\n");
> + err = 0;
> +
> +out:
> + ARCH_PRCTL(ARCH_SHSTK_DISABLE, ARCH_SHSTK_SHSTK);
> + signal(SIGSEGV, SIG_DFL);
> + if (fd)
> + close(fd);
> + return err;
> +}
> +
> void segv_handler_ptrace(int signum, siginfo_t *si, void *uc)
> {
> /* The SSP adjustment caused a segfault. */
> @@ -867,6 +1003,12 @@ int main(int argc, char *argv[])
> goto out;
> }
>
> + if (test_uretprobe()) {
> + ret = 1;
> + printf("[FAIL]\turetprobe test\n");
> + goto out;
> + }
> +
> return ret;
>
> out:
> --
> 2.44.0
>
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Masami Hiramatsu @ 2024-05-13 9:50 UTC (permalink / raw)
To: Jiri Olsa
Cc: Edgecombe, Rick P, songliubraving@fb.com, luto@kernel.org,
mhiramat@kernel.org, andrii@kernel.org, debug@rivosinc.com,
john.fastabend@gmail.com, linux-api@vger.kernel.org,
linux-kernel@vger.kernel.org, mingo@redhat.com,
rostedt@goodmis.org, ast@kernel.org, tglx@linutronix.de,
yhs@fb.com, oleg@redhat.com, peterz@infradead.org,
linux-man@vger.kernel.org, daniel@iogearbox.net,
linux-trace-kernel@vger.kernel.org, bp@alien8.de,
bpf@vger.kernel.org, x86@kernel.org
In-Reply-To: <Zj_enIB_J6pGJ6Nu@krava>
On Sat, 11 May 2024 15:09:48 -0600
Jiri Olsa <olsajiri@gmail.com> wrote:
> On Thu, May 09, 2024 at 04:24:37PM +0000, Edgecombe, Rick P wrote:
> > On Thu, 2024-05-09 at 10:30 +0200, Jiri Olsa wrote:
> > > > Per the earlier discussion, this cannot be reached unless uretprobes are in
> > > > use,
> > > > which cannot happen without something with privileges taking an action. But
> > > > are
> > > > uretprobes ever used for monitoring applications where security is
> > > > important? Or
> > > > is it strictly a debug-time thing?
> > >
> > > sorry, I don't have that level of detail, but we do have customers
> > > that use uprobes in general or want to use it and complain about
> > > the speed
> > >
> > > there are several tools in bcc [1] that use uretprobes in scripts,
> > > like:
> > > memleak, sslsniff, trace, bashreadline, gethostlatency, argdist,
> > > funclatency
> >
> > Is it possible to have shadow stack only use the non-syscall solution? It seems
> > it exposes a more limited compatibility in that it only allows writing the
> > specific trampoline address. (IIRC) Then shadow stack users could still use
> > uretprobes, but just not the new optimized solution. There are already
> > operations that are slower with shadow stack, like longjmp(), so this could be
> > ok maybe.
>
> I guess it's doable, we'd need to keep both trampolines around, because
> shadow stack is enabled by app dynamically and use one based on the
> state of shadow stack when uretprobe is installed
>
> so you're worried the optimized syscall path could be somehow exploited
> to add data on shadow stack?
Good point. For the security concerning (e.g. leaking sensitive information
from secure process which uses shadow stack), we need another limitation
which prohibits probing such process even for debugging. But I think that
needs another series of patches. We also need to discuss when it should be
prohibited and how (e.g. audit interface? SELinux?).
But I think this series is just optimizing currently available uprobes with
a new syscall. I don't think it changes such security concerning.
Thank you,
>
> jirka
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* [RFC PATCH] LSM, net: Add SO_PEERCONTEXT for peer LSM data
From: Casey Schaufler @ 2024-05-13 17:03 UTC (permalink / raw)
To: LSM List, netdev, linux-api, Linux kernel mailing list
In-Reply-To: <763db426-6f60-4d36-b3f9-b316008889f7.ref@schaufler-ca.com>
We recently introduced system calls to access process attributes that
are used by Linux Security Modules (LSM). An important aspect of these
system calls is that they provide the LSM attribute data in a format
that identifies the LSM to which the data applies. Another aspect is that
it can be used to provide multiple instances of the attribute for the
case where more than one LSM supplies the attribute.
We wish to take advantage of this format for data about network peers.
The existing mechanism, SO_PEERSEC, provides peer security data as a
text string. This is sufficient when the LSM providing the information
is known by the user of SO_PEERSEC, and there is only one LSM providing
the information. It fails, however, if the user does not know which
LSM is providing the information.
Discussions about extending SO_PEERSEC to accomodate either the new
format or some other encoding scheme invariably lead to the conclusion
that doing so would lead to tears. Hence, we introduce SO_PEERCONTEXT
which uses the same API data as the LSM system calls.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
arch/alpha/include/uapi/asm/socket.h | 1 +
arch/mips/include/uapi/asm/socket.h | 1 +
arch/parisc/include/uapi/asm/socket.h | 1 +
arch/sparc/include/uapi/asm/socket.h | 1 +
include/linux/lsm_hook_defs.h | 2 +
include/linux/security.h | 18 ++++++++
include/uapi/asm-generic/socket.h | 1 +
net/core/sock.c | 4 ++
security/apparmor/lsm.c | 39 ++++++++++++++++
security/security.c | 86 +++++++++++++++++++++++++++++++++++
security/selinux/hooks.c | 35 ++++++++++++++
security/smack/smack_lsm.c | 25 ++++++++++
12 files changed, 214 insertions(+)
diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h
index e94f621903fe..e49ed0a765ce 100644
--- a/arch/alpha/include/uapi/asm/socket.h
+++ b/arch/alpha/include/uapi/asm/socket.h
@@ -139,6 +139,7 @@
#define SO_PASSPIDFD 76
#define SO_PEERPIDFD 77
+#define SO_PEERCONTEXT 78
#if !defined(__KERNEL__)
diff --git a/arch/mips/include/uapi/asm/socket.h b/arch/mips/include/uapi/asm/socket.h
index 60ebaed28a4c..e647f065b3c9 100644
--- a/arch/mips/include/uapi/asm/socket.h
+++ b/arch/mips/include/uapi/asm/socket.h
@@ -150,6 +150,7 @@
#define SO_PASSPIDFD 76
#define SO_PEERPIDFD 77
+#define SO_PEERCONTEXT 78
#if !defined(__KERNEL__)
diff --git a/arch/parisc/include/uapi/asm/socket.h b/arch/parisc/include/uapi/asm/socket.h
index be264c2b1a11..4f48bef863bc 100644
--- a/arch/parisc/include/uapi/asm/socket.h
+++ b/arch/parisc/include/uapi/asm/socket.h
@@ -131,6 +131,7 @@
#define SO_PASSPIDFD 0x404A
#define SO_PEERPIDFD 0x404B
+#define SO_PEERCONTEXT 0x404C
#if !defined(__KERNEL__)
diff --git a/arch/sparc/include/uapi/asm/socket.h b/arch/sparc/include/uapi/asm/socket.h
index 682da3714686..b54c56056323 100644
--- a/arch/sparc/include/uapi/asm/socket.h
+++ b/arch/sparc/include/uapi/asm/socket.h
@@ -132,6 +132,7 @@
#define SO_PASSPIDFD 0x0055
#define SO_PEERPIDFD 0x0056
+#define SO_PEERCONTEXT 0x0057
#if !defined(__KERNEL__)
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 27e6384ec779..d923acb40c20 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -336,6 +336,8 @@ LSM_HOOK(int, 0, socket_shutdown, struct socket *sock, int how)
LSM_HOOK(int, 0, socket_sock_rcv_skb, struct sock *sk, struct sk_buff *skb)
LSM_HOOK(int, -ENOPROTOOPT, socket_getpeersec_stream, struct socket *sock,
sockptr_t optval, sockptr_t optlen, unsigned int len)
+LSM_HOOK(int, -ENOPROTOOPT, socket_getpeerctx_stream, struct socket *sock,
+ sockptr_t optval, sockptr_t optlen, unsigned int len)
LSM_HOOK(int, -ENOPROTOOPT, socket_getpeersec_dgram, struct socket *sock,
struct sk_buff *skb, u32 *secid)
LSM_HOOK(int, 0, sk_alloc_security, struct sock *sk, int family, gfp_t priority)
diff --git a/include/linux/security.h b/include/linux/security.h
index a54543ded9fc..d4f449b7deea 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -563,6 +563,8 @@ int security_inode_getsecctx(struct inode *inode, struct lsmcontext *cp);
int security_locked_down(enum lockdown_reason what);
int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len,
void *val, size_t val_len, u64 id, u64 flags);
+int lsm_fill_socket_ctx(sockptr_t optval, sockptr_t optlen, void *val,
+ size_t val_len, u64 id, u64 flags);
#else /* CONFIG_SECURITY */
static inline int call_blocking_lsm_notifier(enum lsm_event event, void *data)
@@ -1552,6 +1554,12 @@ static inline int lsm_fill_user_ctx(struct lsm_ctx __user *uctx,
{
return -EOPNOTSUPP;
}
+static inline int lsm_fill_socket_ctx(sockptr_t optval, sockptr_t optlen,
+ void *val, size_t val_len, u64 id,
+ u64 flags)
+{
+ return -EOPNOTSUPP;
+}
#endif /* CONFIG_SECURITY */
#if defined(CONFIG_SECURITY) && defined(CONFIG_WATCH_QUEUE)
@@ -1599,6 +1607,8 @@ int security_socket_shutdown(struct socket *sock, int how);
int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb);
int security_socket_getpeersec_stream(struct socket *sock, sockptr_t optval,
sockptr_t optlen, unsigned int len);
+int security_socket_getpeerctx_stream(struct socket *sock, sockptr_t optval,
+ sockptr_t optlen, unsigned int len);
int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid);
int security_sk_alloc(struct sock *sk, int family, gfp_t priority);
void security_sk_free(struct sock *sk);
@@ -1744,6 +1754,14 @@ static inline int security_socket_getpeersec_stream(struct socket *sock,
return -ENOPROTOOPT;
}
+static inline int security_socket_getpeerctx_stream(struct socket *sock,
+ sockptr_t optval,
+ sockptr_t optlen,
+ unsigned int len)
+{
+ return -ENOPROTOOPT;
+}
+
static inline int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid)
{
return -ENOPROTOOPT;
diff --git a/include/uapi/asm-generic/socket.h b/include/uapi/asm-generic/socket.h
index 8ce8a39a1e5f..e0166ff53670 100644
--- a/include/uapi/asm-generic/socket.h
+++ b/include/uapi/asm-generic/socket.h
@@ -134,6 +134,7 @@
#define SO_PASSPIDFD 76
#define SO_PEERPIDFD 77
+#define SO_PEERCONTEXT 78
#if !defined(__KERNEL__)
diff --git a/net/core/sock.c b/net/core/sock.c
index 0963689a5950..251346eccfa5 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1849,6 +1849,10 @@ int sk_getsockopt(struct sock *sk, int level, int optname,
return security_socket_getpeersec_stream(sock,
optval, optlen, len);
+ case SO_PEERCONTEXT:
+ return security_socket_getpeerctx_stream(sock,
+ optval, optlen, len);
+
case SO_MARK:
v.val = READ_ONCE(sk->sk_mark);
break;
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index c478493f3eaf..66d4ecd6afcb 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1357,6 +1357,43 @@ static int apparmor_socket_getpeersec_stream(struct socket *sock,
return error;
}
+/**
+ * apparmor_socket_getpeerctx_stream - get security context of peer
+ * @sock: socket that we are trying to get the peer context of
+ * @optval: output - buffer to copy peer name to
+ * @optlen: output - size of copied name in @optval
+ * @len: size of @optval buffer
+ * Returns: 0 on success, -errno of failure
+ *
+ * Note: for tcp only valid if using ipsec or cipso on lan
+ */
+static int apparmor_socket_getpeerctx_stream(struct socket *sock,
+ sockptr_t optval, sockptr_t optlen,
+ unsigned int len)
+{
+ char *name = NULL;
+ int slen, error = 0;
+ struct aa_label *label;
+ struct aa_label *peer;
+
+ label = begin_current_label_crit_section();
+ peer = sk_peer_label(sock->sk);
+ if (IS_ERR(peer)) {
+ error = PTR_ERR(peer);
+ goto done;
+ }
+ slen = aa_label_asxprint(&name, labels_ns(label), peer,
+ FLAG_SHOW_MODE | FLAG_VIEW_SUBNS |
+ FLAG_HIDDEN_UNCONFINED, GFP_KERNEL);
+
+ error = lsm_fill_socket_ctx(optval, optlen, name, slen,
+ LSM_ID_APPARMOR, 0);
+done:
+ end_current_label_crit_section(label);
+ kfree(name);
+ return error;
+}
+
/**
* apparmor_sock_graft - Initialize newly created socket
* @sk: child sock
@@ -1466,6 +1503,8 @@ static struct security_hook_list apparmor_hooks[] __ro_after_init = {
#endif
LSM_HOOK_INIT(socket_getpeersec_stream,
apparmor_socket_getpeersec_stream),
+ LSM_HOOK_INIT(socket_getpeerctx_stream,
+ apparmor_socket_getpeerctx_stream),
LSM_HOOK_INIT(sock_graft, apparmor_sock_graft),
#ifdef CONFIG_NETWORK_SECMARK
LSM_HOOK_INIT(inet_conn_request, apparmor_inet_conn_request),
diff --git a/security/security.c b/security/security.c
index e387614cb054..fd4919c28e8f 100644
--- a/security/security.c
+++ b/security/security.c
@@ -874,6 +874,64 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len,
return rc;
}
+/**
+ * lsm_fill_socket_ctx - Fill a socket lsm_ctx structure
+ * @optval: a socket LSM context to be filled
+ * @optlen: uctx size
+ * @val: the new LSM context value
+ * @val_len: the size of the new LSM context value
+ * @id: LSM id
+ * @flags: LSM defined flags
+ *
+ * Fill all of the fields in a lsm_ctx structure. If @optval is NULL
+ * simply calculate the required size to output via @optlen and return
+ * success.
+ *
+ * Returns 0 on success, -E2BIG if userspace buffer is not large enough,
+ * -EFAULT on a copyout error, -ENOMEM if memory can't be allocated.
+ */
+int lsm_fill_socket_ctx(sockptr_t optval, sockptr_t optlen, void *val,
+ size_t val_len, u64 id, u64 flags)
+{
+ struct lsm_ctx *nctx = NULL;
+ unsigned int nctx_len;
+ int loptlen;
+ int rc = 0;
+
+ if (copy_from_sockptr(&loptlen, optlen, sizeof(int)))
+ return -EFAULT;
+
+ nctx_len = ALIGN(struct_size(nctx, ctx, val_len), sizeof(void *));
+ if (nctx_len > loptlen && !sockptr_is_null(optval))
+ rc = -E2BIG;
+
+ /* no buffer - return success/0 and set @uctx_len to the req size */
+ if (sockptr_is_null(optval) || rc)
+ goto out;
+
+ nctx = kzalloc(nctx_len, GFP_KERNEL);
+ if (!nctx) {
+ rc = -ENOMEM;
+ goto out;
+ }
+ nctx->id = id;
+ nctx->flags = flags;
+ nctx->len = nctx_len;
+ nctx->ctx_len = val_len;
+ memcpy(nctx->ctx, val, val_len);
+
+ if (copy_to_sockptr(optval, nctx, nctx_len))
+ rc = -EFAULT;
+
+ kfree(nctx);
+out:
+ if (copy_to_sockptr(optlen, &nctx_len, sizeof(int)))
+ rc = -EFAULT;
+
+ return rc;
+}
+
+
/*
* The default value of the LSM hook is defined in linux/lsm_hook_defs.h and
* can be accessed with:
@@ -4743,6 +4801,34 @@ int security_socket_getpeersec_stream(struct socket *sock, sockptr_t optval,
return LSM_RET_DEFAULT(socket_getpeersec_stream);
}
+/**
+ * security_socket_getpeerctx_stream() - Get the remote peer label
+ * @sock: socket
+ * @optval: destination buffer
+ * @optlen: size of peer label copied into the buffer
+ * @len: maximum size of the destination buffer
+ *
+ * This hook allows the security module to provide peer socket security state
+ * for unix or connected tcp sockets to userspace via getsockopt
+ * SO_GETPEERCONTEXT. For tcp sockets this can be meaningful if the socket
+ * is associated with an ipsec SA.
+ *
+ * Return: Returns 0 if all is well, otherwise, typical getsockopt return
+ * values.
+ */
+int security_socket_getpeerctx_stream(struct socket *sock, sockptr_t optval,
+ sockptr_t optlen, unsigned int len)
+{
+ struct security_hook_list *hp;
+
+ hlist_for_each_entry(hp, &security_hook_heads.socket_getpeerctx_stream,
+ list)
+ return hp->hook.socket_getpeerctx_stream(sock, optval, optlen,
+ len);
+
+ return LSM_RET_DEFAULT(socket_getpeerctx_stream);
+}
+
/**
* security_socket_getpeersec_dgram() - Get the remote peer label
* @sock: socket
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 83d30579e568..3c34cbc2b197 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5221,6 +5221,39 @@ static int selinux_socket_getpeersec_stream(struct socket *sock,
return err;
}
+static int selinux_socket_getpeerctx_stream(struct socket *sock,
+ sockptr_t optval, sockptr_t optlen,
+ unsigned int len)
+{
+ int err = 0;
+ char *scontext = NULL;
+ u32 scontext_len;
+ struct sk_security_struct *sksec = selinux_sock(sock->sk);
+ u32 peer_sid = SECSID_NULL;
+
+ if (sksec->sclass == SECCLASS_UNIX_STREAM_SOCKET ||
+ sksec->sclass == SECCLASS_TCP_SOCKET ||
+ sksec->sclass == SECCLASS_SCTP_SOCKET)
+ peer_sid = sksec->peer_sid;
+ if (peer_sid == SECSID_NULL)
+ return -ENOPROTOOPT;
+
+ err = security_sid_to_context(peer_sid, &scontext,
+ &scontext_len);
+ if (err)
+ return err;
+ if (scontext_len > len) {
+ err = -ERANGE;
+ goto out_len;
+ }
+
+ err = lsm_fill_socket_ctx(optval, optlen, scontext, scontext_len,
+ LSM_ID_SELINUX, 0);
+out_len:
+ kfree(scontext);
+ return err;
+}
+
static int selinux_socket_getpeersec_dgram(struct socket *sock,
struct sk_buff *skb, u32 *secid)
{
@@ -7345,6 +7378,8 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
LSM_HOOK_INIT(socket_sock_rcv_skb, selinux_socket_sock_rcv_skb),
LSM_HOOK_INIT(socket_getpeersec_stream,
selinux_socket_getpeersec_stream),
+ LSM_HOOK_INIT(socket_getpeerctx_stream,
+ selinux_socket_getpeerctx_stream),
LSM_HOOK_INIT(socket_getpeersec_dgram, selinux_socket_getpeersec_dgram),
LSM_HOOK_INIT(sk_free_security, selinux_sk_free_security),
LSM_HOOK_INIT(sk_clone_security, selinux_sk_clone_security),
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 3380a9e91fb8..dd0ea4da7eeb 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4297,6 +4297,30 @@ static int smack_socket_getpeersec_stream(struct socket *sock,
return rc;
}
+/**
+ * smack_socket_getpeerctx_stream - pull in packet label
+ * @sock: the socket
+ * @optval: user's destination
+ * @optlen: size thereof
+ * @len: max thereof
+ *
+ * returns zero on success, an error code otherwise
+ */
+static int smack_socket_getpeerctx_stream(struct socket *sock,
+ sockptr_t optval, sockptr_t optlen,
+ unsigned int len)
+{
+ struct socket_smack *ssp;
+ size_t slen;
+ char *rcp = "";
+
+ ssp = smack_sock(sock->sk);
+ if (ssp->smk_packet)
+ rcp = ssp->smk_packet->smk_known;
+ slen = strlen(rcp) + 1;
+
+ return lsm_fill_socket_ctx(optval, optlen, rcp, slen, LSM_ID_SMACK, 0);
+}
/**
* smack_socket_getpeersec_dgram - pull in packet label
@@ -5206,6 +5230,7 @@ static struct security_hook_list smack_hooks[] __ro_after_init = {
LSM_HOOK_INIT(socket_sendmsg, smack_socket_sendmsg),
LSM_HOOK_INIT(socket_sock_rcv_skb, smack_socket_sock_rcv_skb),
LSM_HOOK_INIT(socket_getpeersec_stream, smack_socket_getpeersec_stream),
+ LSM_HOOK_INIT(socket_getpeerctx_stream, smack_socket_getpeerctx_stream),
LSM_HOOK_INIT(socket_getpeersec_dgram, smack_socket_getpeersec_dgram),
LSM_HOOK_INIT(sk_alloc_security, smack_sk_alloc_security),
#ifdef SMACK_IPV6_PORT_LABELING
^ permalink raw reply related
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Edgecombe, Rick P @ 2024-05-13 17:12 UTC (permalink / raw)
To: olsajiri@gmail.com, mhiramat@kernel.org
Cc: songliubraving@fb.com, luto@kernel.org, andrii@kernel.org,
debug@rivosinc.com, john.fastabend@gmail.com,
linux-api@vger.kernel.org, linux-kernel@vger.kernel.org,
mingo@redhat.com, rostedt@goodmis.org, ast@kernel.org,
tglx@linutronix.de, yhs@fb.com, oleg@redhat.com,
linux-man@vger.kernel.org, daniel@iogearbox.net,
peterz@infradead.org, linux-trace-kernel@vger.kernel.org,
bp@alien8.de, bpf@vger.kernel.org, x86@kernel.org
In-Reply-To: <20240513185040.416d62bc4a71e79367c1cd9c@kernel.org>
On Mon, 2024-05-13 at 18:50 +0900, Masami Hiramatsu wrote:
> > I guess it's doable, we'd need to keep both trampolines around, because
> > shadow stack is enabled by app dynamically and use one based on the
> > state of shadow stack when uretprobe is installed
> >
> > so you're worried the optimized syscall path could be somehow exploited
> > to add data on shadow stack?
Shadow stack allows for modification to the shadow stack only through a few
limited ways (call, ret, etc). The kernel has the ability to write through
shadow stack protections (for example when pushing and popping signal frames),
but the ways in which it does this are limited in order to try to prevent
providing extra capabilities to attackers wanting to craft their own shadow
stacks.
But the HW features have optional abilities to allow extra patterns of shadow
stack modification for userspace as well. This can facilitate unusual patterns
of stack modification (like in this series). For, x86 there is the ability to
allow an instruction (called WRSS) such that userspace can also write arbitrary
data to the shadow stack. Arm has something likes that, plus an instruction to
push to the shadow stack.
There was some debate about whether to use these features, as glibc could not
perfectly match compatibility for features that play with the stack like
longjmp(). As in, without using those extra HW capabilities, some apps would
require modifications to work with shadow stack.
There has been a lot of design tension between security, performance and
compatibility in figuring out how to fit this feature into existing software. In
the end the consensus was to not use these extra HW capabilities, and lean
towards security in the implementation. To try to summarize the debate, this was
because we could get pretty close to compatibility without enabling these extra
features.
So since this solution does something like enabling these extra capabilities in
software that were purposely disabled in HW, it raises eyebrows. Glibc has some
operations that now have extra steps because of shadow stack. So if we could do
something that was still functional, but slower and more secure, then it seems
roughly in line with the tradeoffs we have gone with so far.
But shadow stack is not in widespread use yet, so whether we have the final
tradeoffs settled is still open I think. For example, other libcs have expressed
interest in using WRSS.
I'm also not clear on the typical use of uretprobes (debugging vs production).
And whether shadow stack + debugging + production will happen seems pretty
unknown.
>
> Good point. For the security concerning (e.g. leaking sensitive information
> from secure process which uses shadow stack), we need another limitation
> which prohibits probing such process even for debugging. But I think that
> needs another series of patches. We also need to discuss when it should be
> prohibited and how (e.g. audit interface? SELinux?).
> But I think this series is just optimizing currently available uprobes with
> a new syscall. I don't think it changes such security concerning.
Patch 6 adds support for shadow stack for uretprobes. Currently there is no
support.
Peterz had asked that the new solution consider shadow stack support, so I think
that is how this series grew kind of two goals: new faster uretprobes and
initial shadow stack support.
^ permalink raw reply
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Jiri Olsa @ 2024-05-13 21:23 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: olsajiri@gmail.com, mhiramat@kernel.org, songliubraving@fb.com,
luto@kernel.org, andrii@kernel.org, debug@rivosinc.com,
john.fastabend@gmail.com, linux-api@vger.kernel.org,
linux-kernel@vger.kernel.org, mingo@redhat.com,
rostedt@goodmis.org, ast@kernel.org, tglx@linutronix.de,
yhs@fb.com, oleg@redhat.com, linux-man@vger.kernel.org,
daniel@iogearbox.net, peterz@infradead.org,
linux-trace-kernel@vger.kernel.org, bp@alien8.de,
bpf@vger.kernel.org, x86@kernel.org
In-Reply-To: <c56ae75e9cf0878ac46185a14a18f6ff7e8f891a.camel@intel.com>
On Mon, May 13, 2024 at 05:12:31PM +0000, Edgecombe, Rick P wrote:
> On Mon, 2024-05-13 at 18:50 +0900, Masami Hiramatsu wrote:
> > > I guess it's doable, we'd need to keep both trampolines around, because
> > > shadow stack is enabled by app dynamically and use one based on the
> > > state of shadow stack when uretprobe is installed
> > >
> > > so you're worried the optimized syscall path could be somehow exploited
> > > to add data on shadow stack?
>
> Shadow stack allows for modification to the shadow stack only through a few
> limited ways (call, ret, etc). The kernel has the ability to write through
> shadow stack protections (for example when pushing and popping signal frames),
> but the ways in which it does this are limited in order to try to prevent
> providing extra capabilities to attackers wanting to craft their own shadow
> stacks.
>
> But the HW features have optional abilities to allow extra patterns of shadow
> stack modification for userspace as well. This can facilitate unusual patterns
> of stack modification (like in this series). For, x86 there is the ability to
> allow an instruction (called WRSS) such that userspace can also write arbitrary
> data to the shadow stack. Arm has something likes that, plus an instruction to
> push to the shadow stack.
>
> There was some debate about whether to use these features, as glibc could not
> perfectly match compatibility for features that play with the stack like
> longjmp(). As in, without using those extra HW capabilities, some apps would
> require modifications to work with shadow stack.
>
> There has been a lot of design tension between security, performance and
> compatibility in figuring out how to fit this feature into existing software. In
> the end the consensus was to not use these extra HW capabilities, and lean
> towards security in the implementation. To try to summarize the debate, this was
> because we could get pretty close to compatibility without enabling these extra
> features.
>
> So since this solution does something like enabling these extra capabilities in
> software that were purposely disabled in HW, it raises eyebrows. Glibc has some
> operations that now have extra steps because of shadow stack. So if we could do
> something that was still functional, but slower and more secure, then it seems
> roughly in line with the tradeoffs we have gone with so far.
so at the moment the patch 6 changes shadow stack for
1) current uretprobe which are not working at the moment and we change
the top value of shadow stack with shstk_push_frame
2) optimized uretprobe which needs to push new frame on shadow stack
with shstk_update_last_frame
I think we should do 1) and have current uretprobe working with shadow
stack, which is broken at the moment
I'm ok with not using optimized uretprobe when shadow stack is detected
as enabled and we go with current uretprobe in that case
would this work for you?
thanks,
jirka
>
> But shadow stack is not in widespread use yet, so whether we have the final
> tradeoffs settled is still open I think. For example, other libcs have expressed
> interest in using WRSS.
>
> I'm also not clear on the typical use of uretprobes (debugging vs production).
> And whether shadow stack + debugging + production will happen seems pretty
> unknown.
>
> >
> > Good point. For the security concerning (e.g. leaking sensitive information
> > from secure process which uses shadow stack), we need another limitation
> > which prohibits probing such process even for debugging. But I think that
> > needs another series of patches. We also need to discuss when it should be
> > prohibited and how (e.g. audit interface? SELinux?).
> > But I think this series is just optimizing currently available uprobes with
> > a new syscall. I don't think it changes such security concerning.
>
> Patch 6 adds support for shadow stack for uretprobes. Currently there is no
> support.
>
> Peterz had asked that the new solution consider shadow stack support, so I think
> that is how this series grew kind of two goals: new faster uretprobes and
> initial shadow stack support.
>
>
^ permalink raw reply
* Re: [PATCHv5 bpf-next 7/8] selftests/x86: Add return uprobe shadow stack test
From: Jiri Olsa @ 2024-05-13 21:28 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Steven Rostedt, Oleg Nesterov, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, linux-kernel,
linux-trace-kernel, linux-api, linux-man, x86, bpf, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), Ingo Molnar, Andy Lutomirski,
Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240513184507.215ec89dea4790243d17a52c@kernel.org>
On Mon, May 13, 2024 at 06:45:07PM +0900, Masami Hiramatsu wrote:
> On Tue, 7 May 2024 12:53:20 +0200
> Jiri Olsa <jolsa@kernel.org> wrote:
>
> > Adding return uprobe test for shadow stack and making sure it's
> > working properly. Borrowed some of the code from bpf selftests.
>
> Hi Jiri,
>
> I can not find "SKIP" result in this change. If CONFIG_UPROBES=n,
> this should skip uprobe test.
ah it should be detected by parse_uint_from_file returning ENOENT
or something like that.. will add that
thanks,
jirka
>
> Thank you,
>
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > .../testing/selftests/x86/test_shadow_stack.c | 142 ++++++++++++++++++
> > 1 file changed, 142 insertions(+)
> >
> > diff --git a/tools/testing/selftests/x86/test_shadow_stack.c b/tools/testing/selftests/x86/test_shadow_stack.c
> > index 757e6527f67e..1b919baa999b 100644
> > --- a/tools/testing/selftests/x86/test_shadow_stack.c
> > +++ b/tools/testing/selftests/x86/test_shadow_stack.c
> > @@ -34,6 +34,7 @@
> > #include <sys/ptrace.h>
> > #include <sys/signal.h>
> > #include <linux/elf.h>
> > +#include <linux/perf_event.h>
> >
> > /*
> > * Define the ABI defines if needed, so people can run the tests
> > @@ -681,6 +682,141 @@ int test_32bit(void)
> > return !segv_triggered;
> > }
> >
> > +static int parse_uint_from_file(const char *file, const char *fmt)
> > +{
> > + int err, ret;
> > + FILE *f;
> > +
> > + f = fopen(file, "re");
> > + if (!f) {
> > + err = -errno;
> > + printf("failed to open '%s': %d\n", file, err);
> > + return err;
> > + }
> > + err = fscanf(f, fmt, &ret);
> > + if (err != 1) {
> > + err = err == EOF ? -EIO : -errno;
> > + printf("failed to parse '%s': %d\n", file, err);
> > + fclose(f);
> > + return err;
> > + }
> > + fclose(f);
> > + return ret;
> > +}
> > +
> > +static int determine_uprobe_perf_type(void)
> > +{
> > + const char *file = "/sys/bus/event_source/devices/uprobe/type";
> > +
> > + return parse_uint_from_file(file, "%d\n");
> > +}
> > +
> > +static int determine_uprobe_retprobe_bit(void)
> > +{
> > + const char *file = "/sys/bus/event_source/devices/uprobe/format/retprobe";
> > +
> > + return parse_uint_from_file(file, "config:%d\n");
> > +}
> > +
> > +static ssize_t get_uprobe_offset(const void *addr)
> > +{
> > + size_t start, end, base;
> > + char buf[256];
> > + bool found = false;
> > + FILE *f;
> > +
> > + f = fopen("/proc/self/maps", "r");
> > + if (!f)
> > + return -errno;
> > +
> > + while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &base) == 4) {
> > + if (buf[2] == 'x' && (uintptr_t)addr >= start && (uintptr_t)addr < end) {
> > + found = true;
> > + break;
> > + }
> > + }
> > +
> > + fclose(f);
> > +
> > + if (!found)
> > + return -ESRCH;
> > +
> > + return (uintptr_t)addr - start + base;
> > +}
> > +
> > +static __attribute__((noinline)) void uretprobe_trigger(void)
> > +{
> > + asm volatile ("");
> > +}
> > +
> > +/*
> > + * This test setups return uprobe, which is sensitive to shadow stack
> > + * (crashes without extra fix). After executing the uretprobe we fail
> > + * the test if we receive SIGSEGV, no crash means we're good.
> > + *
> > + * Helper functions above borrowed from bpf selftests.
> > + */
> > +static int test_uretprobe(void)
> > +{
> > + const size_t attr_sz = sizeof(struct perf_event_attr);
> > + const char *file = "/proc/self/exe";
> > + int bit, fd = 0, type, err = 1;
> > + struct perf_event_attr attr;
> > + struct sigaction sa = {};
> > + ssize_t offset;
> > +
> > + type = determine_uprobe_perf_type();
> > + if (type < 0)
> > + return 1;
> > +
> > + offset = get_uprobe_offset(uretprobe_trigger);
> > + if (offset < 0)
> > + return 1;
> > +
> > + bit = determine_uprobe_retprobe_bit();
> > + if (bit < 0)
> > + return 1;
> > +
> > + sa.sa_sigaction = segv_gp_handler;
> > + sa.sa_flags = SA_SIGINFO;
> > + if (sigaction(SIGSEGV, &sa, NULL))
> > + return 1;
> > +
> > + /* Setup return uprobe through perf event interface. */
> > + memset(&attr, 0, attr_sz);
> > + attr.size = attr_sz;
> > + attr.type = type;
> > + attr.config = 1 << bit;
> > + attr.config1 = (__u64) (unsigned long) file;
> > + attr.config2 = offset;
> > +
> > + fd = syscall(__NR_perf_event_open, &attr, 0 /* pid */, -1 /* cpu */,
> > + -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
> > + if (fd < 0)
> > + goto out;
> > +
> > + if (sigsetjmp(jmp_buffer, 1))
> > + goto out;
> > +
> > + ARCH_PRCTL(ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK);
> > +
> > + /*
> > + * This either segfaults and goes through sigsetjmp above
> > + * or succeeds and we're good.
> > + */
> > + uretprobe_trigger();
> > +
> > + printf("[OK]\tUretprobe test\n");
> > + err = 0;
> > +
> > +out:
> > + ARCH_PRCTL(ARCH_SHSTK_DISABLE, ARCH_SHSTK_SHSTK);
> > + signal(SIGSEGV, SIG_DFL);
> > + if (fd)
> > + close(fd);
> > + return err;
> > +}
> > +
> > void segv_handler_ptrace(int signum, siginfo_t *si, void *uc)
> > {
> > /* The SSP adjustment caused a segfault. */
> > @@ -867,6 +1003,12 @@ int main(int argc, char *argv[])
> > goto out;
> > }
> >
> > + if (test_uretprobe()) {
> > + ret = 1;
> > + printf("[FAIL]\turetprobe test\n");
> > + goto out;
> > + }
> > +
> > return ret;
> >
> > out:
> > --
> > 2.44.0
> >
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCH v4 02/27] ntsync: Introduce NTSYNC_IOC_WAIT_ALL.
From: Elizabeth Figura @ 2024-05-14 4:15 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan,
linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Andy Lutomirski, linux-doc,
linux-kselftest, Randy Dunlap, Ingo Molnar, Will Deacon,
Waiman Long, Boqun Feng
In-Reply-To: <20240419162814.GA39162@noisy.programming.kicks-ass.net>
On Friday, April 19, 2024 11:28:14 AM CDT Peter Zijlstra wrote:
> On Thu, Apr 18, 2024 at 11:35:11AM +0200, Peter Zijlstra wrote:
> > On Wed, Apr 17, 2024 at 03:03:05PM -0500, Elizabeth Figura wrote:
> > > Ach. I wrote this with the idea that the race isn't meaningful, but
> > > looking at it again you're right—there is a harmful race here.
> > >
> > > I think it should be fixable by moving the atomic_read inside the lock,
> > > though.
> >
> > Right, I've ended up with the (as yet untested) below. I'll see if I can
> > find time later to actually test things.
>
> Latest hackery... I tried testing this but I'm not having luck using the
> patched wine as per the other email.
>
I converted the rest of the direct uses of spin_lock() using the below patch
and tested it myself, and it passes Wine tests. As far as I can tell the logic
is correct, too; I couldn't find any races.
I'll incorporate these changes into the next revision, unless there's a good
reason not to.
---
--- a/drivers/misc/ntsync.c
+++ b/drivers/misc/ntsync.c
@@ -569,17 +569,19 @@ static int ntsync_event_set(struct ntsync_obj *event,
void __user *argp, bool pu
static int ntsync_event_reset(struct ntsync_obj *event, void __user *argp)
{
+ struct ntsync_device *dev = event->dev;
__u32 prev_state;
+ bool all;
if (event->type != NTSYNC_TYPE_EVENT)
return -EINVAL;
- spin_lock(&event->lock);
+ all = ntsync_lock_obj(dev, event);
prev_state = event->u.event.signaled;
event->u.event.signaled = false;
- spin_unlock(&event->lock);
+ ntsync_unlock_obj(dev, event, all);
if (put_user(prev_state, (__u32 __user *)argp))
return -EFAULT;
@@ -590,16 +592,21 @@ static int ntsync_event_reset(struct ntsync_obj *event,
void __user *argp)
static int ntsync_sem_read(struct ntsync_obj *sem, void __user *argp)
{
struct ntsync_sem_args __user *user_args = argp;
+ struct ntsync_device *dev = sem->dev;
struct ntsync_sem_args args;
+ bool all;
if (sem->type != NTSYNC_TYPE_SEM)
return -EINVAL;
args.sem = 0;
- spin_lock(&sem->lock);
+
+ all = ntsync_lock_obj(dev, sem);
+
args.count = sem->u.sem.count;
args.max = sem->u.sem.max;
- spin_unlock(&sem->lock);
+
+ ntsync_unlock_obj(dev, sem, all);
if (copy_to_user(user_args, &args, sizeof(args)))
return -EFAULT;
@@ -609,18 +616,23 @@ static int ntsync_sem_read(struct ntsync_obj *sem, void
__user *argp)
static int ntsync_mutex_read(struct ntsync_obj *mutex, void __user *argp)
{
struct ntsync_mutex_args __user *user_args = argp;
+ struct ntsync_device *dev = mutex->dev;
struct ntsync_mutex_args args;
+ bool all;
int ret;
if (mutex->type != NTSYNC_TYPE_MUTEX)
return -EINVAL;
args.mutex = 0;
- spin_lock(&mutex->lock);
+
+ all = ntsync_lock_obj(dev, mutex);
+
args.count = mutex->u.mutex.count;
args.owner = mutex->u.mutex.owner;
ret = mutex->u.mutex.ownerdead ? -EOWNERDEAD : 0;
- spin_unlock(&mutex->lock);
+
+ ntsync_unlock_obj(dev, mutex, all);
if (copy_to_user(user_args, &args, sizeof(args)))
return -EFAULT;
@@ -630,16 +642,21 @@ static int ntsync_mutex_read(struct ntsync_obj *mutex,
void __user *argp)
static int ntsync_event_read(struct ntsync_obj *event, void __user *argp)
{
struct ntsync_event_args __user *user_args = argp;
+ struct ntsync_device *dev = event->dev;
struct ntsync_event_args args;
+ bool all;
if (event->type != NTSYNC_TYPE_EVENT)
return -EINVAL;
args.event = 0;
- spin_lock(&event->lock);
+
+ all = ntsync_lock_obj(dev, event);
+
args.manual = event->u.event.manual;
args.signaled = event->u.event.signaled;
- spin_unlock(&event->lock);
+
+ ntsync_unlock_obj(dev, event, all);
if (copy_to_user(user_args, &args, sizeof(args)))
return -EFAULT;
@@ -962,6 +979,7 @@ static int ntsync_wait_any(struct ntsync_device *dev, void
__user *argp)
__u32 i, total_count;
struct ntsync_q *q;
int signaled;
+ bool all;
int ret;
if (copy_from_user(&args, argp, sizeof(args)))
@@ -981,9 +999,9 @@ static int ntsync_wait_any(struct ntsync_device *dev, void
__user *argp)
struct ntsync_q_entry *entry = &q->entries[i];
struct ntsync_obj *obj = entry->obj;
- spin_lock(&obj->lock);
+ all = ntsync_lock_obj(dev, obj);
list_add_tail(&entry->node, &obj->any_waiters);
- spin_unlock(&obj->lock);
+ ntsync_unlock_obj(dev, obj, all);
}
/*
@@ -1000,9 +1018,9 @@ static int ntsync_wait_any(struct ntsync_device *dev,
void __user *argp)
if (atomic_read(&q->signaled) != -1)
break;
- spin_lock(&obj->lock);
+ all = ntsync_lock_obj(dev, obj);
try_wake_any_obj(obj);
- spin_unlock(&obj->lock);
+ ntsync_unlock_obj(dev, obj, all);
}
/* sleep */
@@ -1015,9 +1033,9 @@ static int ntsync_wait_any(struct ntsync_device *dev,
void __user *argp)
struct ntsync_q_entry *entry = &q->entries[i];
struct ntsync_obj *obj = entry->obj;
- spin_lock(&obj->lock);
+ all = ntsync_lock_obj(dev, obj);
list_del(&entry->node);
- spin_unlock(&obj->lock);
+ ntsync_unlock_obj(dev, obj, all);
put_obj(obj);
}
@@ -1075,9 +1093,9 @@ static int ntsync_wait_all(struct ntsync_device *dev,
void __user *argp)
struct ntsync_q_entry *entry = &q->entries[args.count];
struct ntsync_obj *obj = entry->obj;
- spin_lock_nest_lock(&obj->lock, &dev->wait_all_lock);
+ dev_lock_obj(dev, obj);
list_add_tail(&entry->node, &obj->any_waiters);
- spin_unlock(&obj->lock);
+ dev_unlock_obj(dev, obj);
}
/* check if we are already signaled */
@@ -1095,9 +1113,9 @@ static int ntsync_wait_all(struct ntsync_device *dev,
void __user *argp)
struct ntsync_obj *obj = q->entries[args.count].obj;
if (atomic_read(&q->signaled) == -1) {
- spin_lock(&obj->lock);
+ dev_lock_obj(dev, obj);
try_wake_any_obj(obj);
- spin_unlock(&obj->lock);
+ dev_unlock_obj(dev, obj);
}
}
@@ -1127,9 +1145,9 @@ static int ntsync_wait_all(struct ntsync_device *dev,
void __user *argp)
struct ntsync_q_entry *entry = &q->entries[args.count];
struct ntsync_obj *obj = entry->obj;
- spin_lock_nest_lock(&obj->lock, &dev->wait_all_lock);
+ dev_lock_obj(dev, obj);
list_del(&entry->node);
- spin_unlock(&obj->lock);
+ dev_unlock_obj(dev, obj);
put_obj(obj);
}
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox