Skip to contentSkip to conversation

Fix BPF probe memory helpers

PATCH Superseded Daniel Borkmann 2 participants 17 messages Oct 25, 2019

Subsystems (10)
Networking Network drivers Memory management Kselftest +6 more subsystems

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Daniel Borkmann

author maintainer
#0 lore raw

cover Fix BPF probe memory helpers

This set adds probe_read_{user,kernel}(), probe_read_str_{user,kernel}() helpers, fixes probe_write_user() helper and selftests. For details please see individual patches.

Thanks!

Daniel Borkmann (5):
uaccess: Add non-pagefault user-space write function
bpf: Make use of probe_user_write in probe write helper
bpf: Add probe_read_{user,kernel} and probe_read_str_{user,kernel} helpers bpf, samples: Use bpf_probe_read_user where appropriate
bpf, testing: Add selftest to read/write sockaddr from user space

 include/linux/uaccess.h                       |  12 ++
 include/uapi/linux/bpf.h                      | 119 ++++++++++-----
 kernel/trace/bpf_trace.c                      | 139 ++++++++++++------
 mm/maccess.c                                  |  45 +++++-
 samples/bpf/map_perf_test_kern.c              |   4 +-
 samples/bpf/test_map_in_map_kern.c            |   4 +-
 samples/bpf/test_probe_write_user_kern.c      |   2 +-
 tools/include/uapi/linux/bpf.h                | 119 ++++++++++-----
 .../selftests/bpf/prog_tests/probe_user.c     |  80 ++++++++++
 .../selftests/bpf/progs/test_probe_user.c     |  33 +++++
 10 files changed, 426 insertions(+), 131 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/probe_user.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_probe_user.c
Signature · 1 line
2.21.0
author maintainer
#1 lore raw

PATCH 1/5 uaccess: Add non-pagefault user-space write function

Commit 3d7081822f7f ("uaccess: Add non-pagefault user-space read functions") missed to add probe write function, therefore factor out a probe_write_common() helper with most logic of probe_kernel_write() except setting KERNEL_DS, and add a new probe_user_write() helper so it can be used from BPF side.

Again, on some archs, the user address space and kernel address space can co-exist and be overlapping, so in such case, setting KERNEL_DS would mean that the given address is treated as being in kernel address space.

Signed-off-by Daniel Borkmann <[email protected]>
---
 include/linux/uaccess.h | 12 +++++++++++
 mm/maccess.c            | 45 +++++++++++++++++++++++++++++++++++++----
 2 files changed, 53 insertions(+), 4 deletions(-)
include/linux/uaccess.h +12 -0
index e47d0522a1f4..86dcf2894672 100644
@@ -337,6 +337,18 @@ extern long __probe_user_read(void *dst, const void __user *src, size_t size);
 extern long notrace probe_kernel_write(void *dst, const void *src, size_t size);
 extern long notrace __probe_kernel_write(void *dst, const void *src, size_t size);
 
+/*
+ * probe_user_write(): safely attempt to write to a location in user space
+ * @dst: address to write to
+ * @src: pointer to the data that shall be written
+ * @size: size of the data chunk
+ *
+ * Safely write to address @dst from the buffer at @src.  If a kernel fault
+ * happens, handle that and return -EFAULT.
+ */
+extern long notrace probe_user_write(void __user *dst, const void *src, size_t size);
+extern long notrace __probe_user_write(void __user *dst, const void *src, size_t size);
+
 extern long strncpy_from_unsafe(char *dst, const void *unsafe_addr, long count);
 extern long strncpy_from_unsafe_user(char *dst, const void __user *unsafe_addr,
 				     long count);
mm/maccess.c +41 -4

68 lines not on this page · raw message

Signature · 1 line
2.21.0
Acked-by on v1 1/5
#2 lore raw

Re: [PATCH bpf-next 1/5] uaccess: Add non-pagefault user-space write function

Quoted 6 lines · Daniel Borkmann

Commit 3d7081822f7f ("uaccess: Add non-pagefault user-space read functions") missed to add probe write function, therefore factor out a probe_write_common() helper with most logic of probe_kernel_write() except setting KERNEL_DS, and add a new probe_user_write() helper so it can be used from BPF side.

Quoted 6 lines

Again, on some archs, the user address space and kernel address space can co-exist and be overlapping, so in such case, setting KERNEL_DS would mean that the given address is treated as being in kernel address space.

Signed-off-by Daniel Borkmann <[email protected]>

---

LGTM. See an EFAULT comment below, though.

Acked-by Andrii Nakryiko <[email protected]>
Quoted 30 lines
 include/linux/uaccess.h | 12 +++++++++++
 mm/maccess.c            | 45 +++++++++++++++++++++++++++++++++++++----
 2 files changed, 53 insertions(+), 4 deletions(-)
include/linux/uaccess.h +12 -0
index e47d0522a1f4..86dcf2894672 100644
@@ -337,6 +337,18 @@ extern long __probe_user_read(void *dst, const void __user *src, size_t size);
 extern long notrace probe_kernel_write(void *dst, const void *src, size_t size);
 extern long notrace __probe_kernel_write(void *dst, const void *src, size_t size);
 
+/*
+ * probe_user_write(): safely attempt to write to a location in user space
+ * @dst: address to write to
+ * @src: pointer to the data that shall be written
+ * @size: size of the data chunk
+ *
+ * Safely write to address @dst from the buffer at @src.  If a kernel fault
+ * happens, handle that and return -EFAULT.
+ */
+extern long notrace probe_user_write(void __user *dst, const void *src, size_t size);
+extern long notrace __probe_user_write(void __user *dst, const void *src, size_t size);
+
 extern long strncpy_from_unsafe(char *dst, const void *unsafe_addr, long count);
 extern long strncpy_from_unsafe_user(char *dst, const void __user *unsafe_addr,
                                     long count);
mm/maccess.c +0 -1
index d065736f6b87..2d3c3d01064c 100644
--- a/mm/maccess.c

[...]

mm/maccess.c long __probe_kernel_write(void *dst, const void *src, size_t size) · line 128

Quoted 13 lines
mm/maccess.c +12 -0
+/**
+ * probe_user_write(): safely attempt to write to a user-space location
+ * @dst: address to write to
+ * @src: pointer to the data that shall be written
+ * @size: size of the data chunk
+ *
+ * Safely write to address @dst from the buffer at @src.  If a kernel fault
+ * happens, handle that and return -EFAULT.
+ */
+
+long __weak probe_user_write(void __user *dst, const void *src, size_t size)
+    __attribute__((alias("__probe_user_write")));

curious, why is there this dance of probe_user_write alias to __probe_user_write (and for other pairs of functions as well)?

mm/maccess.c long __probe_kernel_write(void *dst, const void *src, size_t size) · line 132

Quoted 4 lines
mm/maccess.c +4 -0
+
+long __probe_user_write(void __user *dst, const void *src, size_t size)
+{
+       long ret = -EFAULT;

This initialization is not necessary, is it? Similarly in __probe_user_read higher in this file.

Quoted 16 lines
mm/maccess.c +10 -1
+       mm_segment_t old_fs = get_fs();
+
+       set_fs(USER_DS);
+       if (access_ok(dst, size))
+               ret = probe_write_common(dst, src, size);
+       set_fs(old_fs);
+
+       return ret;
+}
+EXPORT_SYMBOL_GPL(probe_user_write);
 
 /**
  * strncpy_from_unsafe: - Copy a NUL terminated string from unsafe address.
--

2.21.0

author maintainer
#3 lore raw

Re: [PATCH bpf-next 1/5] uaccess: Add non-pagefault user-space write function

Quoted 19 lines · Andrii Nakryiko
Quoted 11 lines · Daniel Borkmann

Commit 3d7081822f7f ("uaccess: Add non-pagefault user-space read functions") missed to add probe write function, therefore factor out a probe_write_common() helper with most logic of probe_kernel_write() except setting KERNEL_DS, and add a new probe_user_write() helper so it can be used from BPF side.

Again, on some archs, the user address space and kernel address space can co-exist and be overlapping, so in such case, setting KERNEL_DS would mean that the given address is treated as being in kernel address space.

Signed-off-by Daniel Borkmann <[email protected]>

LGTM. See an EFAULT comment below, though.

Acked-by Andrii Nakryiko <[email protected]>

[...]

Quoted 15 lines

mm/maccess.c long __probe_kernel_write(void *dst, const void *src, size_t size) · line 128

Quoted 12 lines
mm/maccess.c +12 -0
+/**
+ * probe_user_write(): safely attempt to write to a user-space location
+ * @dst: address to write to
+ * @src: pointer to the data that shall be written
+ * @size: size of the data chunk
+ *
+ * Safely write to address @dst from the buffer at @src.  If a kernel fault
+ * happens, handle that and return -EFAULT.
+ */
+
+long __weak probe_user_write(void __user *dst, const void *src, size_t size)
+    __attribute__((alias("__probe_user_write")));

curious, why is there this dance of probe_user_write alias to __probe_user_write (and for other pairs of functions as well)?

Seems done by convention to allow archs to override the __weak marked functions in order to add additional checks and being able to then call into the __ prefixed variant.

Quoted 6 lines

mm/maccess.c long __probe_kernel_write(void *dst, const void *src, size_t size) · line 132

Quoted 3 lines
mm/maccess.c +3 -0
+long __probe_user_write(void __user *dst, const void *src, size_t size)
+{
+       long ret = -EFAULT;

This initialization is not necessary, is it? Similarly in __probe_user_read higher in this file.

Not entirely sure what you mean. In both there's access_ok() check before invoking the common helper.

Quoted 16 lines
Quoted 16 lines
mm/maccess.c +10 -1
+       mm_segment_t old_fs = get_fs();
+
+       set_fs(USER_DS);
+       if (access_ok(dst, size))
+               ret = probe_write_common(dst, src, size);
+       set_fs(old_fs);
+
+       return ret;
+}
+EXPORT_SYMBOL_GPL(probe_user_write);
 
 /**
  * strncpy_from_unsafe: - Copy a NUL terminated string from unsafe address.
--

2.21.0

#4 lore raw

Re: [PATCH bpf-next 1/5] uaccess: Add non-pagefault user-space write function

Quoted 48 lines · Daniel Borkmann
Quoted 19 lines · Andrii Nakryiko
Quoted 11 lines · Daniel Borkmann

Commit 3d7081822f7f ("uaccess: Add non-pagefault user-space read functions") missed to add probe write function, therefore factor out a probe_write_common() helper with most logic of probe_kernel_write() except setting KERNEL_DS, and add a new probe_user_write() helper so it can be used from BPF side.

Again, on some archs, the user address space and kernel address space can co-exist and be overlapping, so in such case, setting KERNEL_DS would mean that the given address is treated as being in kernel address space.

Signed-off-by Daniel Borkmann <[email protected]>

LGTM. See an EFAULT comment below, though.

Acked-by Andrii Nakryiko <[email protected]>

[...]

Quoted 15 lines

mm/maccess.c long __probe_kernel_write(void *dst, const void *src, size_t size) · line 128

Quoted 12 lines
mm/maccess.c +12 -0
+/**
+ * probe_user_write(): safely attempt to write to a user-space location
+ * @dst: address to write to
+ * @src: pointer to the data that shall be written
+ * @size: size of the data chunk
+ *
+ * Safely write to address @dst from the buffer at @src.  If a kernel fault
+ * happens, handle that and return -EFAULT.
+ */
+
+long __weak probe_user_write(void __user *dst, const void *src, size_t size)
+    __attribute__((alias("__probe_user_write")));

curious, why is there this dance of probe_user_write alias to __probe_user_write (and for other pairs of functions as well)?

Seems done by convention to allow archs to override the __weak marked functions in order to add additional checks and being able to then call into the __ prefixed variant.

Quoted 6 lines

mm/maccess.c long __probe_kernel_write(void *dst, const void *src, size_t size) · line 132

Quoted 3 lines
mm/maccess.c +3 -0
+long __probe_user_write(void __user *dst, const void *src, size_t size)
+{
+       long ret = -EFAULT;

This initialization is not necessary, is it? Similarly in __probe_user_read higher in this file.

Quoted 2 lines

Not entirely sure what you mean. In both there's access_ok() check before invoking the common helper.

ah, right, if, yeah, never mind then.

Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
mm/maccess.c +10 -1
+       mm_segment_t old_fs = get_fs();
+
+       set_fs(USER_DS);
+       if (access_ok(dst, size))
+               ret = probe_write_common(dst, src, size);
+       set_fs(old_fs);
+
+       return ret;
+}
+EXPORT_SYMBOL_GPL(probe_user_write);
 
 /**
  * strncpy_from_unsafe: - Copy a NUL terminated string from unsafe address.
--

2.21.0

author maintainer
#5 lore raw

PATCH 2/5 bpf: Make use of probe_user_write in probe write helper

In reply to Daniel Borkmann

Convert the bpf_probe_write_user() helper to probe_user_write() such that writes are not attempted under KERNEL_DS anymore which is buggy as kernel and user space pointers can have overlapping addresses. Also, given we have the access_ok() check inside probe_user_write(), the helper doesn't need to do it twice.

Fixes 96ae52279594 ("bpf: Add bpf_probe_write_user BPF helper to be called in tracers")
Signed-off-by Daniel Borkmann <[email protected]>
---
 kernel/trace/bpf_trace.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
kernel/trace/bpf_trace.c +2 -4
index c3240898cc44..79919a26cd59 100644
@@ -163,7 +163,7 @@ static const struct bpf_func_proto bpf_probe_read_proto = {
 	.arg3_type	= ARG_ANYTHING,
 };
 
-BPF_CALL_3(bpf_probe_write_user, void *, unsafe_ptr, const void *, src,
+BPF_CALL_3(bpf_probe_write_user, void __user *, unsafe_ptr, const void *, src,
 	   u32, size)
 {
 	/*

@@ -186,10 +186,8 @@ BPF_CALL_3(bpf_probe_write_user, void *, unsafe_ptr, const void *, src,
 		return -EPERM;
 	if (unlikely(!nmi_uaccess_okay()))
 		return -EPERM;
-	if (!access_ok(unsafe_ptr, size))
-		return -EPERM;
 
-	return probe_kernel_write(unsafe_ptr, src, size);
+	return probe_user_write(unsafe_ptr, src, size);
 }
 
 static const struct bpf_func_proto bpf_probe_write_user_proto = {
Signature · 1 line
2.21.0
Acked-by on v1 2/5
#6 lore raw

Re: [PATCH bpf-next 2/5] bpf: Make use of probe_user_write in probe write helper

Quoted 10 lines · Daniel Borkmann

Convert the bpf_probe_write_user() helper to probe_user_write() such that writes are not attempted under KERNEL_DS anymore which is buggy as kernel and user space pointers can have overlapping addresses. Also, given we have the access_ok() check inside probe_user_write(), the helper doesn't need to do it twice.

Fixes 96ae52279594 ("bpf: Add bpf_probe_write_user BPF helper to be called in tracers")
Signed-off-by Daniel Borkmann <[email protected]>

---

Acked-by Andrii Nakryiko <[email protected]>

[...]

author maintainer
#7 lore raw

PATCH 3/5 bpf: Add probe_read_{user,kernel} and probe_read_str_{user,kernel} helpers

In reply to Daniel Borkmann

The current bpf_probe_read() and bpf_probe_read_str() helpers are broken in that they assume they can be used for probing memory access for kernel space addresses /as well as/ user space addresses.

However, plain use of probe_kernel_read() for both cases will attempt to always access kernel space address space given access is performed under KERNEL_DS and some archs in-fact have overlapping address spaces where a kernel pointer and user pointer would have the /same/ address value and therefore accessing application memory via bpf_probe_read{,_str}() would read garbage values.

Lets fix BPF side by making use of recently added 3d7081822f7f ("uaccess:
Add non-pagefault user-space read functions"). Unfortunately, the only way to fix this status quo is to add dedicated bpf_probe_read_{user,kernel}() and bpf_probe_read_str_{user,kernel}() helpers. The bpf_probe_read{,_str}() helpers are aliased to the *_kernel() variants to retain their current behavior; for API consistency and ease of use the latter have been added so that it is immediately *obvious* which address space the memory is being probed on (user,kernel). The two *_user() variants attempt the access under USER_DS set.

Fixes a5e8c07059d0 ("bpf: add bpf_probe_read_str helper")
Fixes 2541517c32be ("tracing, perf: Implement BPF programs attached to kprobes")
Signed-off-by Daniel Borkmann <[email protected]>
---
 include/uapi/linux/bpf.h       | 119 ++++++++++++++++++++---------
 kernel/trace/bpf_trace.c       | 133 ++++++++++++++++++++++-----------
 tools/include/uapi/linux/bpf.h | 119 ++++++++++++++++++++---------
 3 files changed, 253 insertions(+), 118 deletions(-)
include/uapi/linux/bpf.h +82 -37
index 4af8b0819a32..b8ffb419df51 100644
@@ -564,7 +564,11 @@ union bpf_attr {
  * int bpf_probe_read(void *dst, u32 size, const void *src)
  * 	Description
  * 		For tracing programs, safely attempt to read *size* bytes from
- * 		address *src* and store the data in *dst*.
+ * 		kernel space address *src* and store the data in *dst*.
+ *
+ * 		This helper is an alias to bpf_probe_read_kernel().
+ *
+ * 		Generally, use bpf_probe_read_user() or bpf_probe_read_kernel() instead.
  * 	Return
  * 		0 on success, or a negative error in case of failure.
  *

@@ -1428,43 +1432,14 @@ union bpf_attr {
  *
  * int bpf_probe_read_str(void *dst, int size, const void *unsafe_ptr)
  * 	Description
- * 		Copy a NUL terminated string from an unsafe address
- * 		*unsafe_ptr* to *dst*. The *size* should include the
- * 		terminating NUL byte. In case the string length is smaller than
- * 		*size*, the target is not padded with further NUL bytes. If the
- * 		string length is larger than *size*, just *size*-1 bytes are
- * 		copied and the last byte is set to NUL.
- *
- * 		On success, the length of the copied string is returned. This
- * 		makes this helper useful in tracing programs for reading
- * 		strings, and more importantly to get its length at runtime. See
- * 		the following snippet:
- *
- * 		::
- *
- * 			SEC("kprobe/sys_open")
- * 			void bpf_sys_open(struct pt_regs *ctx)
- * 			{
- * 			        char buf[PATHLEN]; // PATHLEN is defined to 256
- * 			        int res = bpf_probe_read_str(buf, sizeof(buf),
- * 				                             ctx->di);
- *
- * 				// Consume buf, for example push it to
- * 				// userspace via bpf_perf_event_output(); we
- * 				// can use res (the string length) as event
- * 				// size, after checking its boundaries.
- * 			}
+ * 		Copy a NUL terminated string from an unsafe kernel address
+ * 		*unsafe_ptr* to *dst*. See bpf_probe_read_str_kernel() for
+ * 		more details.
  *
- * 		In comparison, using **bpf_probe_read()** helper here instead
- * 		to read the string would require to estimate the length at
- * 		compile time, and would often result in copying more memory
- * 		than necessary.
+ * 		This helper is an alias to bpf_probe_read_str_kernel().
  *
- * 		Another useful use case is when parsing individual process
- * 		arguments or individual environment variables navigating
- * 		*current*\ **->mm->arg_start** and *current*\
- * 		**->mm->env_start**: using this helper and the return value,
- * 		one can quickly iterate at the right offset of the memory area.
+ * 		Generally, use bpf_probe_read_str_user() or bpf_probe_read_str_kernel()
+ * 		instead.
  * 	Return
  * 		On success, the strictly positive length of the string,
  * 		including the trailing NUL character. On error, a negative

@@ -2775,6 +2750,72 @@ union bpf_attr {
  * 		restricted to raw_tracepoint bpf programs.
  * 	Return
  * 		0 on success, or a negative error in case of failure.
+ *
+ * int bpf_probe_read_user(void *dst, u32 size, const void *src)
+ * 	Description
+ * 		Safely attempt to read *size* bytes from user space address
+ * 		*src* and store the data in *dst*.
+ * 	Return
+ * 		0 on success, or a negative error in case of failure.
+ *
+ * int bpf_probe_read_kernel(void *dst, u32 size, const void *src)
+ * 	Description
+ * 		Safely attempt to read *size* bytes from kernel space address
+ * 		*src* and store the data in *dst*.
+ * 	Return
+ * 		0 on success, or a negative error in case of failure.
+ *
+ * int bpf_probe_read_str_user(void *dst, int size, const void *unsafe_ptr)
+ * 	Description
+ * 		Copy a NUL terminated string from an unsafe user address
+ * 		*unsafe_ptr* to *dst*. The *size* should include the
+ * 		terminating NUL byte. In case the string length is smaller than
+ * 		*size*, the target is not padded with further NUL bytes. If the
+ * 		string length is larger than *size*, just *size*-1 bytes are
+ * 		copied and the last byte is set to NUL.
+ *
+ * 		On success, the length of the copied string is returned. This
+ * 		makes this helper useful in tracing programs for reading
+ * 		strings, and more importantly to get its length at runtime. See
+ * 		the following snippet:
+ *
+ * 		::
+ *
+ * 			SEC("kprobe/sys_open")
+ * 			void bpf_sys_open(struct pt_regs *ctx)
+ * 			{
+ * 			        char buf[PATHLEN]; // PATHLEN is defined to 256
+ * 			        int res = bpf_probe_read_str_user(buf, sizeof(buf),
+ * 				                                  ctx->di);
+ *
+ * 				// Consume buf, for example push it to
+ * 				// userspace via bpf_perf_event_output(); we
+ * 				// can use res (the string length) as event
+ * 				// size, after checking its boundaries.
+ * 			}
+ *
+ * 		In comparison, using **bpf_probe_read_user()** helper here
+ * 		instead to read the string would require to estimate the length
+ * 		at compile time, and would often result in copying more memory
+ * 		than necessary.
+ *
+ * 		Another useful use case is when parsing individual process
+ * 		arguments or individual environment variables navigating
+ * 		*current*\ **->mm->arg_start** and *current*\
+ * 		**->mm->env_start**: using this helper and the return value,
+ * 		one can quickly iterate at the right offset of the memory area.
+ * 	Return
+ * 		On success, the strictly positive length of the string,
+ * 		including the trailing NUL character. On error, a negative
+ * 		value.
+ *
+ * int bpf_probe_read_str_kernel(void *dst, int size, const void *unsafe_ptr)
+ * 	Description
+ * 		Copy a NUL terminated string from an unsafe kernel address *unsafe_ptr*
+ * 		to *dst*. Same semantics as with bpf_probe_read_str_user() apply.
+ * 	Return
+ * 		On success, the strictly positive length of the string,	including
+ * 		the trailing NUL character. On error, a negative value.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\

@@ -2888,7 +2929,11 @@ union bpf_attr {
 	FN(sk_storage_delete),		\
 	FN(send_signal),		\
 	FN(tcp_gen_syncookie),		\
-	FN(skb_output),
+	FN(skb_output),			\
+	FN(probe_read_user),		\
+	FN(probe_read_kernel),		\
+	FN(probe_read_str_user),	\
+	FN(probe_read_str_kernel),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
kernel/trace/bpf_trace.c +89 -44

166 lines not on this page · raw message

tools/include/uapi/linux/bpf.h +82 -37

145 lines not on this page · raw message

Signature · 1 line
2.21.0
#8 lore raw

Re: [PATCH bpf-next 3/5] bpf: Add probe_read_{user,kernel} and probe_read_str_{user,kernel} helpers

Quoted 49 lines · Daniel Borkmann

The current bpf_probe_read() and bpf_probe_read_str() helpers are broken in that they assume they can be used for probing memory access for kernel space addresses /as well as/ user space addresses.

However, plain use of probe_kernel_read() for both cases will attempt to always access kernel space address space given access is performed under KERNEL_DS and some archs in-fact have overlapping address spaces where a kernel pointer and user pointer would have the /same/ address value and therefore accessing application memory via bpf_probe_read{,_str}() would read garbage values.

Lets fix BPF side by making use of recently added 3d7081822f7f ("uaccess:
Add non-pagefault user-space read functions"). Unfortunately, the only way to fix this status quo is to add dedicated bpf_probe_read_{user,kernel}() and bpf_probe_read_str_{user,kernel}() helpers. The bpf_probe_read{,_str}() helpers are aliased to the *_kernel() variants to retain their current behavior; for API consistency and ease of use the latter have been added so that it is immediately *obvious* which address space the memory is being probed on (user,kernel). The two *_user() variants attempt the access under USER_DS set.

Fixes a5e8c07059d0 ("bpf: add bpf_probe_read_str helper")
Fixes 2541517c32be ("tracing, perf: Implement BPF programs attached to kprobes")
Signed-off-by Daniel Borkmann <[email protected]>
---
 include/uapi/linux/bpf.h       | 119 ++++++++++++++++++++---------
 kernel/trace/bpf_trace.c       | 133 ++++++++++++++++++++++-----------
 tools/include/uapi/linux/bpf.h | 119 ++++++++++++++++++++---------
 3 files changed, 253 insertions(+), 118 deletions(-)
include/uapi/linux/bpf.h +5 -1
index 4af8b0819a32..b8ffb419df51 100644
@@ -564,7 +564,11 @@ union bpf_attr {
  * int bpf_probe_read(void *dst, u32 size, const void *src)
  *     Description
  *             For tracing programs, safely attempt to read *size* bytes from
- *             address *src* and store the data in *dst*.
+ *             kernel space address *src* and store the data in *dst*.
+ *
+ *             This helper is an alias to bpf_probe_read_kernel().
+ *
+ *             Generally, use bpf_probe_read_user() or bpf_probe_read_kernel() instead.
  *     Return
  *             0 on success, or a negative error in case of failure.
  *

include/uapi/linux/bpf.h union bpf_attr { · line 1433

Quoted 2 lines
include/uapi/linux/bpf.h +0 -0
@@ -1428,43 +1432,14 @@ union bpf_attr {
  *
  * int bpf_probe_read_str(void *dst, int size, const void *unsafe_ptr)

seems like an approriate time to standardize terminology. Should it be unsafe_ptr like here, or src like in bpf_probe_read description?

Quoted 8 lines
include/uapi/linux/bpf.h +0 -7
  *     Description
- *             Copy a NUL terminated string from an unsafe address
- *             *unsafe_ptr* to *dst*. The *size* should include the
- *             terminating NUL byte. In case the string length is smaller than
- *             *size*, the target is not padded with further NUL bytes. If the
- *             string length is larger than *size*, just *size*-1 bytes are
- *             copied and the last byte is set to NUL.
- *

[...]

include/uapi/linux/bpf.h union bpf_attr { · line 2936

Quoted 12 lines
 #define __BPF_FUNC_MAPPER(FN)          \
        FN(unspec),                     \
include/uapi/linux/bpf.h +5 -1
@@ -2888,7 +2929,11 @@ union bpf_attr {
        FN(sk_storage_delete),          \
        FN(send_signal),                \
        FN(tcp_gen_syncookie),          \
-       FN(skb_output),
+       FN(skb_output),                 \
+       FN(probe_read_user),            \
+       FN(probe_read_kernel),          \
+       FN(probe_read_str_user),        \
+       FN(probe_read_str_kernel),

naming is subjective, but I'd go with probe_{user,kernel}_read[_str] scheme, but given bpf_probe_write_user and desire to stay consistent, I'd still stick to slightly different probe_read_{user,kernel}[_str] scheme.

Quoted 11 lines
include/uapi/linux/bpf.h +0 -0
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
kernel/trace/bpf_trace.c +0 -0
index 79919a26cd59..ff001b766799 100644
@@ -138,12 +138,52 @@ static const struct bpf_func_proto bpf_override_return_proto = {
 };
 #endif

[...]

author maintainer
#9 lore raw

Re: [PATCH bpf-next 3/5] bpf: Add probe_read_{user,kernel} and probe_read_str_{user,kernel} helpers

Quoted 47 lines · Andrii Nakryiko
Quoted 46 lines · Daniel Borkmann

The current bpf_probe_read() and bpf_probe_read_str() helpers are broken in that they assume they can be used for probing memory access for kernel space addresses /as well as/ user space addresses.

However, plain use of probe_kernel_read() for both cases will attempt to always access kernel space address space given access is performed under KERNEL_DS and some archs in-fact have overlapping address spaces where a kernel pointer and user pointer would have the /same/ address value and therefore accessing application memory via bpf_probe_read{,_str}() would read garbage values.

Lets fix BPF side by making use of recently added 3d7081822f7f ("uaccess:
Add non-pagefault user-space read functions"). Unfortunately, the only way to fix this status quo is to add dedicated bpf_probe_read_{user,kernel}() and bpf_probe_read_str_{user,kernel}() helpers. The bpf_probe_read{,_str}() helpers are aliased to the *_kernel() variants to retain their current behavior; for API consistency and ease of use the latter have been added so that it is immediately *obvious* which address space the memory is being probed on (user,kernel). The two *_user() variants attempt the access under USER_DS set.

Fixes a5e8c07059d0 ("bpf: add bpf_probe_read_str helper")
Fixes 2541517c32be ("tracing, perf: Implement BPF programs attached to kprobes")
Signed-off-by Daniel Borkmann <[email protected]>
---
 include/uapi/linux/bpf.h       | 119 ++++++++++++++++++++---------
 kernel/trace/bpf_trace.c       | 133 ++++++++++++++++++++++-----------
 tools/include/uapi/linux/bpf.h | 119 ++++++++++++++++++++---------
 3 files changed, 253 insertions(+), 118 deletions(-)
include/uapi/linux/bpf.h +5 -1
index 4af8b0819a32..b8ffb419df51 100644
@@ -564,7 +564,11 @@ union bpf_attr {
  * int bpf_probe_read(void *dst, u32 size, const void *src)
  *     Description
  *             For tracing programs, safely attempt to read *size* bytes from
- *             address *src* and store the data in *dst*.
+ *             kernel space address *src* and store the data in *dst*.
+ *
+ *             This helper is an alias to bpf_probe_read_kernel().
+ *
+ *             Generally, use bpf_probe_read_user() or bpf_probe_read_kernel() instead.
  *     Return
Quoted 8 lines

include/uapi/linux/bpf.h union bpf_attr { · line 1433

Quoted 5 lines
include/uapi/linux/bpf.h +0 -0
  *             0 on success, or a negative error in case of failure.
  *

@@ -1428,43 +1432,14 @@ union bpf_attr {
  *
  * int bpf_probe_read_str(void *dst, int size, const void *unsafe_ptr)

seems like an approriate time to standardize terminology. Should it be unsafe_ptr like here, or src like in bpf_probe_read description?

Makes sense, I'll go for unsafe_ptr in v2 as it feels more descriptive.

Quoted 20 lines
Quoted 8 lines
include/uapi/linux/bpf.h +0 -7
  *     Description
- *             Copy a NUL terminated string from an unsafe address
- *             *unsafe_ptr* to *dst*. The *size* should include the
- *             terminating NUL byte. In case the string length is smaller than
- *             *size*, the target is not padded with further NUL bytes. If the
- *             string length is larger than *size*, just *size*-1 bytes are
- *             copied and the last byte is set to NUL.
- *

[...]

Quoted 9 lines
 #define __BPF_FUNC_MAPPER(FN)          \
        FN(unspec),                     \
include/uapi/linux/bpf.h +2 -1
@@ -2888,7 +2929,11 @@ union bpf_attr {
        FN(sk_storage_delete),          \
        FN(send_signal),                \
        FN(tcp_gen_syncookie),          \
-       FN(skb_output),
+       FN(skb_output),                 \
+       FN(probe_read_user),            \
Quoted 8 lines

include/uapi/linux/bpf.h union bpf_attr { · line 2936

Quoted 3 lines
include/uapi/linux/bpf.h +3 -0
+       FN(probe_read_kernel),          \
+       FN(probe_read_str_user),        \
+       FN(probe_read_str_kernel),

naming is subjective, but I'd go with probe_{user,kernel}_read[_str] scheme, but given bpf_probe_write_user and desire to stay consistent, I'd still stick to slightly different probe_read_{user,kernel}[_str] scheme.

Yeah, I'm fine with changing into probe_read_{user,kernel}[_str], and it's still in line with bpf_probe_{read,write}_{user,kernel} helpers.

Quoted 12 lines
Quoted 10 lines
include/uapi/linux/bpf.h +0 -0
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
kernel/trace/bpf_trace.c +0 -0
index 79919a26cd59..ff001b766799 100644
@@ -138,12 +138,52 @@ static const struct bpf_func_proto bpf_override_return_proto = {
 };
 #endif

[...]

author maintainer
#10 lore raw

PATCH 4/5 bpf, samples: Use bpf_probe_read_user where appropriate

In reply to Daniel Borkmann

Use bpf_probe_read_user() helper instead of bpf_probe_read() for samples that attach to kprobes probing on user addresses.

Signed-off-by Daniel Borkmann <[email protected]>
---
 samples/bpf/map_perf_test_kern.c         | 4 ++--
 samples/bpf/test_map_in_map_kern.c       | 4 ++--
 samples/bpf/test_probe_write_user_kern.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)
samples/bpf/map_perf_test_kern.c +2 -2
index 5c11aefbc489..281bcdaee58e 100644
@@ -181,8 +181,8 @@ int stress_lru_hmap_alloc(struct pt_regs *ctx)
 	if (addrlen != sizeof(*in6))
 		return 0;
 
-	ret = bpf_probe_read(test_params.dst6, sizeof(test_params.dst6),
-			     &in6->sin6_addr);
+	ret = bpf_probe_read_user(test_params.dst6, sizeof(test_params.dst6),
+				  &in6->sin6_addr);
 	if (ret)
 		goto done;
samples/bpf/test_map_in_map_kern.c +2 -2
index 4f80cbe74c72..32ee752f19df 100644
@@ -118,7 +118,7 @@ int trace_sys_connect(struct pt_regs *ctx)
 	if (addrlen != sizeof(*in6))
 		return 0;
 
-	ret = bpf_probe_read(dst6, sizeof(dst6), &in6->sin6_addr);
+	ret = bpf_probe_read_user(dst6, sizeof(dst6), &in6->sin6_addr);
 	if (ret) {
 		inline_ret = ret;
 		goto done;

@@ -129,7 +129,7 @@ int trace_sys_connect(struct pt_regs *ctx)
 
 	test_case = dst6[7];
 
-	ret = bpf_probe_read(&port, sizeof(port), &in6->sin6_port);
+	ret = bpf_probe_read_user(&port, sizeof(port), &in6->sin6_port);
 	if (ret) {
 		inline_ret = ret;
 		goto done;
samples/bpf/test_probe_write_user_kern.c +1 -1
index a543358218e6..b7c48f37132c 100644
@@ -37,7 +37,7 @@ int bpf_prog1(struct pt_regs *ctx)
 	if (sockaddr_len > sizeof(orig_addr))
 		return 0;
 
-	if (bpf_probe_read(&orig_addr, sizeof(orig_addr), sockaddr_arg) != 0)
+	if (bpf_probe_read_user(&orig_addr, sizeof(orig_addr), sockaddr_arg) != 0)
 		return 0;
 
 	mapped_addr = bpf_map_lookup_elem(&dnat_map, &orig_addr);
Signature · 1 line
2.21.0
Acked-by on v1 4/5
#11 lore raw

Re: [PATCH bpf-next 4/5] bpf, samples: Use bpf_probe_read_user where appropriate

Quoted 6 lines · Daniel Borkmann

Use bpf_probe_read_user() helper instead of bpf_probe_read() for samples that attach to kprobes probing on user addresses.

Signed-off-by Daniel Borkmann <[email protected]>

---

Acked-by Andrii Nakryiko <[email protected]>
Quoted 5 lines
 samples/bpf/map_perf_test_kern.c         | 4 ++--
 samples/bpf/test_map_in_map_kern.c       | 4 ++--
 samples/bpf/test_probe_write_user_kern.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

[...]

author maintainer
#12 lore raw

PATCH 5/5 bpf, testing: Add selftest to read/write sockaddr from user space

In reply to Daniel Borkmann

Tested on x86-64 and Ilya was also kind enough to give it a spin on s390x, both passing with probe_user:OK there. The test is using the newly added bpf_probe_read_user() to dump sockaddr from connect call into BPF map and overrides the user buffer via bpf_probe_write_user():

  # ./test_progs
  [...]
  #17 pkt_md_access:OK
  #18 probe_user:OK
  #19 prog_run_xattr:OK
  [...]
Signed-off-by Daniel Borkmann <[email protected]>
Tested-by Ilya Leoshkevich <[email protected]>
---
 .../selftests/bpf/prog_tests/probe_user.c     | 80 +++++++++++++++++++
 .../selftests/bpf/progs/test_probe_user.c     | 33 ++++++++
 2 files changed, 113 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/probe_user.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_probe_user.c
tools/testing/selftests/bpf/prog_tests/probe_user.c new file +80 -0
new file mode 100644
index 000000000000..e37761bda8a4
@@ -0,0 +1,80 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+
+void test_probe_user(void)
+{
+#define kprobe_name "__sys_connect"
+	const char *prog_name = "kprobe/" kprobe_name;
+	const char *obj_file = "./test_probe_user.o";
+	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
+		.relaxed_maps = true,
+	);
+	int err, results_map_fd, sock_fd, duration;
+	struct sockaddr curr, orig, tmp;
+	struct sockaddr_in *in = (struct sockaddr_in *)&curr;
+	struct bpf_link *kprobe_link = NULL;
+	struct bpf_program *kprobe_prog;
+	struct bpf_object *obj;
+	static const int zero = 0;
+
+	obj = bpf_object__open_file(obj_file, &opts);
+	if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj)))
+		return;
+
+	kprobe_prog = bpf_object__find_program_by_title(obj, prog_name);
+	if (CHECK(!kprobe_prog, "find_probe",
+		  "prog '%s' not found\n", prog_name))
+		goto cleanup;
+
+	err = bpf_object__load(obj);
+	if (CHECK(err, "obj_load", "err %d\n", err))
+		goto cleanup;
+
+	results_map_fd = bpf_find_map(__func__, obj, "results_map");
+	if (CHECK(results_map_fd < 0, "find_results_map",
+		  "err %d\n", results_map_fd))
+		goto cleanup;
+
+	kprobe_link = bpf_program__attach_kprobe(kprobe_prog, false,
+						 kprobe_name);
+	if (CHECK(IS_ERR(kprobe_link), "attach_kprobe",
+		  "err %ld\n", PTR_ERR(kprobe_link))) {
+		kprobe_link = NULL;
+		goto cleanup;
+	}
+
+	memset(&curr, 0, sizeof(curr));
+	in->sin_family = AF_INET;
+	in->sin_port = htons(5555);
+	in->sin_addr.s_addr = inet_addr("255.255.255.255");
+	memcpy(&orig, &curr, sizeof(curr));
+
+	sock_fd = socket(AF_INET, SOCK_STREAM, 0);
+	if (CHECK(sock_fd < 0, "create_sock_fd", "err %d\n", sock_fd))
+		goto cleanup;
+
+	connect(sock_fd, &curr, sizeof(curr));
+	close(sock_fd);
+
+	err = bpf_map_lookup_elem(results_map_fd, &zero, &tmp);
+	if (CHECK(err, "get_kprobe_res",
+		  "failed to get kprobe res: %d\n", err))
+		goto cleanup;
+
+	in = (struct sockaddr_in *)&tmp;
+	if (CHECK(memcmp(&tmp, &orig, sizeof(orig)), "check_kprobe_res",
+		  "wrong kprobe res from probe read: %s:%u\n",
+		  inet_ntoa(in->sin_addr), ntohs(in->sin_port)))
+		goto cleanup;
+
+	memset(&tmp, 0xab, sizeof(tmp));
+
+	in = (struct sockaddr_in *)&curr;
+	if (CHECK(memcmp(&curr, &tmp, sizeof(tmp)), "check_kprobe_res",
+		  "wrong kprobe res from probe write: %s:%u\n",
+		  inet_ntoa(in->sin_addr), ntohs(in->sin_port)))
+		goto cleanup;
+cleanup:
+	bpf_link__destroy(kprobe_link);
+	bpf_object__close(obj);
+}
tools/testing/selftests/bpf/progs/test_probe_user.c new file +33 -0
new file mode 100644
index 000000000000..a9b8a0bde0b9
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/ptrace.h>
+#include <linux/bpf.h>
+
+#include <netinet/in.h>
+
+#include "bpf_helpers.h"
+#include "bpf_tracing.h"
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(max_entries, 1);
+	__type(key, int);
+	__type(value, struct sockaddr_in);
+} results_map SEC(".maps");
+
+SEC("kprobe/__sys_connect")
+int handle_sys_connect(struct pt_regs *ctx)
+{
+	void *ptr = (void *)PT_REGS_PARM2(ctx);
+	struct sockaddr_in old, new;
+	const int zero = 0;
+
+	bpf_probe_read_user(&old, sizeof(old), ptr);
+	bpf_map_update_elem(&results_map, &zero, &old, 0);
+	__builtin_memset(&new, 0xab, sizeof(new));
+	bpf_probe_write_user(ptr, &new, sizeof(new));
+
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
Signature · 1 line
2.21.0
#13 lore raw

Re: [PATCH bpf-next 5/5] bpf, testing: Add selftest to read/write sockaddr from user space

Quoted 28 lines · Daniel Borkmann

Tested on x86-64 and Ilya was also kind enough to give it a spin on s390x, both passing with probe_user:OK there. The test is using the newly added bpf_probe_read_user() to dump sockaddr from connect call into BPF map and overrides the user buffer via bpf_probe_write_user():

  # ./test_progs
  [...]
  #17 pkt_md_access:OK
  #18 probe_user:OK
  #19 prog_run_xattr:OK
  [...]
Signed-off-by Daniel Borkmann <[email protected]>
Tested-by Ilya Leoshkevich <[email protected]>
---
 .../selftests/bpf/prog_tests/probe_user.c     | 80 +++++++++++++++++++
 .../selftests/bpf/progs/test_probe_user.c     | 33 ++++++++
 2 files changed, 113 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/probe_user.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_probe_user.c
tools/testing/selftests/bpf/prog_tests/probe_user.c new file +0 -0
new file mode 100644
index 000000000000..e37761bda8a4

tools/testing/selftests/bpf/prog_tests/probe_user.c line 10

Quoted 10 lines
tools/testing/selftests/bpf/prog_tests/probe_user.c new file +10 -0
@@ -0,0 +1,80 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+
+void test_probe_user(void)
+{
+#define kprobe_name "__sys_connect"
+       const char *prog_name = "kprobe/" kprobe_name;
+       const char *obj_file = "./test_probe_user.o";
+       DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
+               .relaxed_maps = true,

do we need relaxed_maps in this case?

Quoted 9 lines
tools/testing/selftests/bpf/prog_tests/probe_user.c new file +9 -0
+       );
+       int err, results_map_fd, sock_fd, duration;
+       struct sockaddr curr, orig, tmp;
+       struct sockaddr_in *in = (struct sockaddr_in *)&curr;
+       struct bpf_link *kprobe_link = NULL;
+       struct bpf_program *kprobe_prog;
+       struct bpf_object *obj;
+       static const int zero = 0;
+

[...]

Quoted 7 lines
tools/testing/selftests/bpf/prog_tests/probe_user.c new file +7 -0
+
+struct {
+       __uint(type, BPF_MAP_TYPE_ARRAY);
+       __uint(max_entries, 1);
+       __type(key, int);
+       __type(value, struct sockaddr_in);
+} results_map SEC(".maps");

tools/testing/selftests/bpf/prog_tests/probe_user.c

Quoted 10 lines
tools/testing/selftests/bpf/prog_tests/probe_user.c new file +10 -0
+
+SEC("kprobe/__sys_connect")
+int handle_sys_connect(struct pt_regs *ctx)
+{
+       void *ptr = (void *)PT_REGS_PARM2(ctx);
+       struct sockaddr_in old, new;
+       const int zero = 0;
+
+       bpf_probe_read_user(&old, sizeof(old), ptr);
+       bpf_map_update_elem(&results_map, &zero, &old, 0);

could have used global data and read directly into it :)

Quoted 10 lines
tools/testing/selftests/bpf/prog_tests/probe_user.c new file +7 -1
+       __builtin_memset(&new, 0xab, sizeof(new));
+       bpf_probe_write_user(ptr, &new, sizeof(new));
+
+       return 0;
+}
+
+char _license[] SEC("license") = "GPL";
--

2.21.0

author maintainer
#14 lore raw

Re: [PATCH bpf-next 5/5] bpf, testing: Add selftest to read/write sockaddr from user space

Quoted 33 lines · Andrii Nakryiko
Quoted 32 lines · Daniel Borkmann

Tested on x86-64 and Ilya was also kind enough to give it a spin on s390x, both passing with probe_user:OK there. The test is using the newly added bpf_probe_read_user() to dump sockaddr from connect call into BPF map and overrides the user buffer via bpf_probe_write_user():

  # ./test_progs
  [...]
  #17 pkt_md_access:OK
  #18 probe_user:OK
  #19 prog_run_xattr:OK
  [...]
Signed-off-by Daniel Borkmann <[email protected]>
Tested-by Ilya Leoshkevich <[email protected]>
---
 .../selftests/bpf/prog_tests/probe_user.c     | 80 +++++++++++++++++++
 .../selftests/bpf/progs/test_probe_user.c     | 33 ++++++++
 2 files changed, 113 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/probe_user.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_probe_user.c
tools/testing/selftests/bpf/prog_tests/probe_user.c new file +4 -0
new file mode 100644
index 000000000000..e37761bda8a4
@@ -0,0 +1,80 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+
+void test_probe_user(void)
Quoted 8 lines

tools/testing/selftests/bpf/prog_tests/probe_user.c line 10

Quoted 6 lines
tools/testing/selftests/bpf/prog_tests/probe_user.c new file +6 -0
+{
+#define kprobe_name "__sys_connect"
+       const char *prog_name = "kprobe/" kprobe_name;
+       const char *obj_file = "./test_probe_user.o";
+       DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
+               .relaxed_maps = true,

do we need relaxed_maps in this case?

Ah yeap, I'll remove. Test runs fine w/o it. Any particular reason you added it back in 928ca75e59d7 ("selftests/bpf: switch tests to new bpf_object__open_{file, mem}() APIs")?

Quoted 23 lines
Quoted 9 lines
tools/testing/selftests/bpf/prog_tests/probe_user.c new file +9 -0
+       );
+       int err, results_map_fd, sock_fd, duration;
+       struct sockaddr curr, orig, tmp;
+       struct sockaddr_in *in = (struct sockaddr_in *)&curr;
+       struct bpf_link *kprobe_link = NULL;
+       struct bpf_program *kprobe_prog;
+       struct bpf_object *obj;
+       static const int zero = 0;
+

[...]

Quoted 11 lines
tools/testing/selftests/bpf/prog_tests/probe_user.c new file +11 -0
+
+struct {
+       __uint(type, BPF_MAP_TYPE_ARRAY);
+       __uint(max_entries, 1);
+       __type(key, int);
+       __type(value, struct sockaddr_in);
+} results_map SEC(".maps");
+
+SEC("kprobe/__sys_connect")
+int handle_sys_connect(struct pt_regs *ctx)
+{
Quoted 8 lines

tools/testing/selftests/bpf/prog_tests/probe_user.c

Quoted 6 lines
tools/testing/selftests/bpf/prog_tests/probe_user.c new file +6 -0
+       void *ptr = (void *)PT_REGS_PARM2(ctx);
+       struct sockaddr_in old, new;
+       const int zero = 0;
+
+       bpf_probe_read_user(&old, sizeof(old), ptr);
+       bpf_map_update_elem(&results_map, &zero, &old, 0);

could have used global data and read directly into it :)

Hehe, yeah sure, though that we have covered separately. :-) Wasn't planning to bug Ilya once again to recompile everything on his s390x box.

Quoted 10 lines
Quoted 10 lines
tools/testing/selftests/bpf/prog_tests/probe_user.c new file +7 -1
+       __builtin_memset(&new, 0xab, sizeof(new));
+       bpf_probe_write_user(ptr, &new, sizeof(new));
+
+       return 0;
+}
+
+char _license[] SEC("license") = "GPL";
--

2.21.0

#15 lore raw

Re: [PATCH bpf-next 5/5] bpf, testing: Add selftest to read/write sockaddr from user space

Quoted 44 lines · Daniel Borkmann
Quoted 33 lines · Andrii Nakryiko
Quoted 32 lines · Daniel Borkmann

Tested on x86-64 and Ilya was also kind enough to give it a spin on s390x, both passing with probe_user:OK there. The test is using the newly added bpf_probe_read_user() to dump sockaddr from connect call into BPF map and overrides the user buffer via bpf_probe_write_user():

  # ./test_progs
  [...]
  #17 pkt_md_access:OK
  #18 probe_user:OK
  #19 prog_run_xattr:OK
  [...]
Signed-off-by Daniel Borkmann <[email protected]>
Tested-by Ilya Leoshkevich <[email protected]>
---
 .../selftests/bpf/prog_tests/probe_user.c     | 80 +++++++++++++++++++
 .../selftests/bpf/progs/test_probe_user.c     | 33 ++++++++
 2 files changed, 113 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/probe_user.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_probe_user.c
tools/testing/selftests/bpf/prog_tests/probe_user.c new file +4 -0
new file mode 100644
index 000000000000..e37761bda8a4
@@ -0,0 +1,80 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+
+void test_probe_user(void)
Quoted 8 lines

tools/testing/selftests/bpf/prog_tests/probe_user.c line 10

Quoted 6 lines
tools/testing/selftests/bpf/prog_tests/probe_user.c new file +6 -0
+{
+#define kprobe_name "__sys_connect"
+       const char *prog_name = "kprobe/" kprobe_name;
+       const char *obj_file = "./test_probe_user.o";
+       DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
+               .relaxed_maps = true,

do we need relaxed_maps in this case?

Quoted 2 lines

Ah yeap, I'll remove. Test runs fine w/o it. Any particular reason you added it back in 928ca75e59d7 ("selftests/bpf: switch tests to new bpf_object__open_{file, mem}() APIs")?

Hmm, I'm not sure about those tests... probably just copy/pasted something mechanically. We need .relaxed_maps for tests that add numa fields, otherwise libbpf will reject them. I shouldn't have added them.

Quoted 33 lines
Quoted 24 lines
Quoted 10 lines
tools/testing/selftests/bpf/prog_tests/probe_user.c new file +9 -0
+       );
+       int err, results_map_fd, sock_fd, duration;
+       struct sockaddr curr, orig, tmp;
+       struct sockaddr_in *in = (struct sockaddr_in *)&curr;
+       struct bpf_link *kprobe_link = NULL;
+       struct bpf_program *kprobe_prog;
+       struct bpf_object *obj;
+       static const int zero = 0;
+

[...]

Quoted 11 lines
tools/testing/selftests/bpf/prog_tests/probe_user.c new file +11 -0
+
+struct {
+       __uint(type, BPF_MAP_TYPE_ARRAY);
+       __uint(max_entries, 1);
+       __type(key, int);
+       __type(value, struct sockaddr_in);
+} results_map SEC(".maps");
+
+SEC("kprobe/__sys_connect")
+int handle_sys_connect(struct pt_regs *ctx)
+{
Quoted 8 lines

tools/testing/selftests/bpf/prog_tests/probe_user.c

Quoted 6 lines
tools/testing/selftests/bpf/prog_tests/probe_user.c new file +6 -0
+       void *ptr = (void *)PT_REGS_PARM2(ctx);
+       struct sockaddr_in old, new;
+       const int zero = 0;
+
+       bpf_probe_read_user(&old, sizeof(old), ptr);
+       bpf_map_update_elem(&results_map, &zero, &old, 0);

could have used global data and read directly into it :)

Quoted 2 lines

Hehe, yeah sure, though that we have covered separately. :-) Wasn't planning to bug Ilya once again to recompile everything on his s390x box.

Oh, it's not to test global data, it's because global data make BPF side of tests much cleaner. But it's minor, feel free to ignore. Once we have a good interface to global data from user-space, though, there will be a bigger motivation to switch them to global data, because both BPF and user side will be much more succinct.

Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
tools/testing/selftests/bpf/prog_tests/probe_user.c new file +7 -1
+       __builtin_memset(&new, 0xab, sizeof(new));
+       bpf_probe_write_user(ptr, &new, sizeof(new));
+
+       return 0;
+}
+
+char _license[] SEC("license") = "GPL";
--

2.21.0

Acked-by on v1 5/5
#16 lore raw

Re: [PATCH bpf-next 5/5] bpf, testing: Add selftest to read/write sockaddr from user space

In reply to Daniel Borkmann
Quoted 13 lines · Daniel Borkmann

Tested on x86-64 and Ilya was also kind enough to give it a spin on s390x, both passing with probe_user:OK there. The test is using the newly added bpf_probe_read_user() to dump sockaddr from connect call into BPF map and overrides the user buffer via bpf_probe_write_user():

  # ./test_progs
  [...]
  #17 pkt_md_access:OK
  #18 probe_user:OK
  #19 prog_run_xattr:OK
  [...]
Quoted 3 lines
Signed-off-by Daniel Borkmann <[email protected]>
Tested-by Ilya Leoshkevich <[email protected]>

---

Acked-by Andrii Nakryiko <[email protected]>
Quoted 5 lines
 .../selftests/bpf/prog_tests/probe_user.c     | 80 +++++++++++++++++++
 .../selftests/bpf/progs/test_probe_user.c     | 33 ++++++++
 2 files changed, 113 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/probe_user.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_probe_user.c

[...]