[PATCH bpf 0/3] Restrict bpf_probe_read{,str}() and bpf_trace_printk()'s %s

[PATCH bpf 0/3] Restrict bpf_probe_read{,str}() and bpf_trace_printk()'s %s

[Daniel Borkmann]

Small set of fixes in order to restrict BPF helpers for tracing which are
broken on archs with overlapping address ranges as per discussion in [0].
I've targetted this for -bpf tree so they can be routed as fixes. Thanks!

  [0] https://lore.kernel.org/bpf/CAHk-=wjJKo0GVixYLmqPn-Q22WFu0xHaBSjKEo7e7Yw72y5SPQ@mail.gmail.com/T/

Daniel Borkmann (3):
  bpf: restrict bpf_probe_read{,str}() only to archs where they work
  bpf: add bpf_probe_read_{user, kernel}_str() to do_refine_retval_range
  bpf: restrict bpf_trace_printk()'s %s usage and add %psK, %psU specifier

 Documentation/core-api/printk-formats.rst | 14 ++++
 arch/arm/Kconfig                          |  1 +
 arch/arm64/Kconfig                        |  1 +
 arch/x86/Kconfig                          |  1 +
 init/Kconfig                              |  3 +
 kernel/bpf/verifier.c                     |  4 +-
 kernel/trace/bpf_trace.c                  | 98 +++++++++++++++--------
 lib/vsprintf.c                            |  7 +-
 8 files changed, 94 insertions(+), 35 deletions(-)

-- 
2.21.0

[PATCH bpf 1/3] bpf: restrict bpf_probe_read{,str}() only to archs where they work

[Daniel Borkmann]

Given the legacy bpf_probe_read{,str}() BPF helpers are broken on archs
with overlapping address ranges, we should really take the next step to
disable them from BPF use there.

To generally fix the situation, we've recently added new helper variants
bpf_probe_read_{user,kernel}() and bpf_probe_read_{user,kernel}_str().
For details on them, see 6ae08ae3dea2 ("bpf: Add probe_read_{user, kernel}
and probe_read_{user,kernel}_str helpers").

Given bpf_probe_read{,str}() have been around for ~5 years by now, there
are plenty of users at least on x86 still relying on them today, so we
cannot remove them entirely w/o breaking the BPF tracing ecosystem.

However, their use should be restricted to archs with non-overlapping
address ranges where they are working in their current form. Therefore,
move this behind a CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE and
have x86, arm64, arm select it (other archs supporting it can follow-up
on it as well).

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Brendan Gregg <brendan.d.gregg@gmail.com>
Cc: Christoph Hellwig <hch@lst.de>
---
 arch/arm/Kconfig         | 1 +
 arch/arm64/Kconfig       | 1 +
 arch/x86/Kconfig         | 1 +
 init/Kconfig             | 3 +++
 kernel/trace/bpf_trace.c | 6 ++++--
 5 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 66a04f6f4775..c77c93c485a0 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -12,6 +12,7 @@ config ARM
 	select ARCH_HAS_KEEPINITRD
 	select ARCH_HAS_KCOV
 	select ARCH_HAS_MEMBARRIER_SYNC_CORE
+	select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
 	select ARCH_HAS_PTE_SPECIAL if ARM_LPAE
 	select ARCH_HAS_PHYS_TO_DMA
 	select ARCH_HAS_SETUP_DMA_OPS
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 40fb05d96c60..5d513f461957 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -20,6 +20,7 @@ config ARM64
 	select ARCH_HAS_KCOV
 	select ARCH_HAS_KEEPINITRD
 	select ARCH_HAS_MEMBARRIER_SYNC_CORE
+	select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
 	select ARCH_HAS_PTE_DEVMAP
 	select ARCH_HAS_PTE_SPECIAL
 	select ARCH_HAS_SETUP_DMA_OPS
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 1197b5596d5a..2d3f963fd6f1 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -68,6 +68,7 @@ config X86
 	select ARCH_HAS_KCOV			if X86_64
 	select ARCH_HAS_MEM_ENCRYPT
 	select ARCH_HAS_MEMBARRIER_SYNC_CORE
+	select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
 	select ARCH_HAS_PMEM_API		if X86_64
 	select ARCH_HAS_PTE_DEVMAP		if X86_64
 	select ARCH_HAS_PTE_SPECIAL
diff --git a/init/Kconfig b/init/Kconfig
index 9e22ee8fbd75..6fd13a051342 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -2279,6 +2279,9 @@ config ASN1
 
 source "kernel/Kconfig.locks"
 
+config ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
+	bool
+
 config ARCH_HAS_SYNC_CORE_BEFORE_USERMODE
 	bool
 
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index ca1796747a77..b83bdaa31c7b 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -825,14 +825,16 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_probe_read_user_proto;
 	case BPF_FUNC_probe_read_kernel:
 		return &bpf_probe_read_kernel_proto;
-	case BPF_FUNC_probe_read:
-		return &bpf_probe_read_compat_proto;
 	case BPF_FUNC_probe_read_user_str:
 		return &bpf_probe_read_user_str_proto;
 	case BPF_FUNC_probe_read_kernel_str:
 		return &bpf_probe_read_kernel_str_proto;
+#ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
+	case BPF_FUNC_probe_read:
+		return &bpf_probe_read_compat_proto;
 	case BPF_FUNC_probe_read_str:
 		return &bpf_probe_read_compat_str_proto;
+#endif
 #ifdef CONFIG_CGROUPS
 	case BPF_FUNC_get_current_cgroup_id:
 		return &bpf_get_current_cgroup_id_proto;
-- 
2.21.0

[PATCH bpf 2/3] bpf: add bpf_probe_read_{user, kernel}_str() to do_refine_retval_range

[Daniel Borkmann]

Given bpf_probe_read{,str}() BPF helpers are now only available under
CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE, we need to add the drop-in
replacements of bpf_probe_read_{kernel,user}_str() to do_refine_retval_range()
as well to avoid hitting the same issue as in 849fa50662fbc ("bpf/verifier:
refine retval R0 state for bpf_get_stack helper").

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Yonghong Song <yhs@fb.com>
---
 kernel/bpf/verifier.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index fa1d8245b925..ac922b0d3b00 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4340,7 +4340,9 @@ static void do_refine_retval_range(struct bpf_reg_state *regs, int ret_type,
 
 	if (ret_type != RET_INTEGER ||
 	    (func_id != BPF_FUNC_get_stack &&
-	     func_id != BPF_FUNC_probe_read_str))
+	     func_id != BPF_FUNC_probe_read_str &&
+	     func_id != BPF_FUNC_probe_read_kernel_str &&
+	     func_id != BPF_FUNC_probe_read_user_str))
 		return;
 
 	ret_reg->smax_value = meta->msize_max_value;
-- 
2.21.0

[PATCH bpf 3/3] bpf: restrict bpf_trace_printk()'s %s usage and add %psK, %psU specifier

[Daniel Borkmann]

Usage of plain %s conversion specifier in bpf_trace_printk() suffers from the
very same issue as bpf_probe_read{,str}() helpers, that is, it is broken on
archs with overlapping address ranges.

While the helpers have been addressed through work in 6ae08ae3dea2 ("bpf: Add
probe_read_{user, kernel} and probe_read_{user, kernel}_str helpers"), we need
an option for bpf_trace_printk() as well to fix it.

Similarly as with the helpers, force users to make an explicit choice by adding
%psK and %psU specifier to bpf_trace_printk() which will then pick the corresponding
strncpy_from_unsafe*() variant to perform the access under KERNEL_DS or USER_DS.

Existing %s for legacy users is still kept working for archs where it is not
broken and therefore gated through CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE.

Fixes: 8d3b7dce8622 ("bpf: add support for %s specifier to bpf_trace_printk()")
Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Reported-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Brendan Gregg <brendan.d.gregg@gmail.com>
Cc: Christoph Hellwig <hch@lst.de>
---
 Documentation/core-api/printk-formats.rst | 14 ++++
 kernel/trace/bpf_trace.c                  | 92 +++++++++++++++--------
 lib/vsprintf.c                            |  7 +-
 3 files changed, 81 insertions(+), 32 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 8ebe46b1af39..76b5f4f265cb 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -112,6 +112,20 @@ used when printing stack backtraces. The specifier takes into
 consideration the effect of compiler optimisations which may occur
 when tail-calls are used and marked with the noreturn GCC attribute.
 
+Probed Strings from BPF
+-----------------------
+
+::
+
+	%psK	kernel_string
+	%psU	user_string
+
+The ``sK`` and ``sU`` specifiers are used for printing a string from probed
+memory. From regular vsnprintf(), they are equivalent to ``%s``, however,
+when used out of BPF's bpf_trace_printk() it reads a string of up to 64 bytes
+in memory without faulting. For ``K`` specifier, the string is probed out of
+kernel memory whereas for ``U`` specifier, it is probed out of user memory.
+
 Kernel Pointers
 ---------------
 
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index b83bdaa31c7b..9eef2075ea18 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -323,17 +323,15 @@ static const struct bpf_func_proto *bpf_get_probe_write_proto(void)
 
 /*
  * Only limited trace_printk() conversion specifiers allowed:
- * %d %i %u %x %ld %li %lu %lx %lld %lli %llu %llx %p %s
+ * %d %i %u %x %ld %li %lu %lx %lld %lli %llu %llx %p %psK %psU %s
  */
 BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, fmt_size, u64, arg1,
 	   u64, arg2, u64, arg3)
 {
+	int i, mod[3] = {}, fmt_cnt = 0;
+	void *unsafe_ptr = NULL;
 	bool str_seen = false;
-	int mod[3] = {};
-	int fmt_cnt = 0;
-	u64 unsafe_addr;
 	char buf[64];
-	int i;
 
 	/*
 	 * bpf_check()->check_func_arg()->check_stack_boundary()
@@ -359,40 +357,71 @@ BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, fmt_size, u64, arg1,
 		if (fmt[i] == 'l') {
 			mod[fmt_cnt]++;
 			i++;
-		} else if (fmt[i] == 'p' || fmt[i] == 's') {
+		} else if (fmt[i] == 'p') {
 			mod[fmt_cnt]++;
+			if (fmt[i + 1] == 's' &&
+			    (fmt[i + 2] == 'K' ||
+			     fmt[i + 2] == 'U')) {
+				i += 2;
+				goto fmt_str;
+			}
+
 			/* disallow any further format extensions */
 			if (fmt[i + 1] != 0 &&
 			    !isspace(fmt[i + 1]) &&
 			    !ispunct(fmt[i + 1]))
 				return -EINVAL;
-			fmt_cnt++;
-			if (fmt[i] == 's') {
-				if (str_seen)
-					/* allow only one '%s' per fmt string */
-					return -EINVAL;
-				str_seen = true;
-
-				switch (fmt_cnt) {
-				case 1:
-					unsafe_addr = arg1;
-					arg1 = (long) buf;
-					break;
-				case 2:
-					unsafe_addr = arg2;
-					arg2 = (long) buf;
-					break;
-				case 3:
-					unsafe_addr = arg3;
-					arg3 = (long) buf;
-					break;
-				}
-				buf[0] = 0;
-				strncpy_from_unsafe(buf,
-						    (void *) (long) unsafe_addr,
+
+			goto fmt_next;
+		} else if (fmt[i] == 's') {
+			mod[fmt_cnt]++;
+fmt_str:
+			if (str_seen)
+				/* allow only one '%s' per fmt string */
+				return -EINVAL;
+			str_seen = true;
+
+			if (fmt[i + 1] != 0 &&
+			    !isspace(fmt[i + 1]) &&
+			    !ispunct(fmt[i + 1]))
+				return -EINVAL;
+
+			switch (fmt_cnt) {
+			case 1:
+				unsafe_ptr = (void *)(long)arg1;
+				arg1 = (long)buf;
+				break;
+			case 2:
+				unsafe_ptr = (void *)(long)arg2;
+				arg2 = (long)buf;
+				break;
+			case 3:
+				unsafe_ptr = (void *)(long)arg3;
+				arg3 = (long)buf;
+				break;
+			}
+
+			buf[0] = 0;
+
+			switch (fmt[i]) {
+			default:
+				return -EOPNOTSUPP;
+#ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
+			case 's':
+				/* Fallthrough */
+#endif
+			case 'K':
+				strncpy_from_unsafe(buf, unsafe_ptr,
 						    sizeof(buf));
+				break;
+			case 'U':
+				strncpy_from_unsafe_user(buf,
+						(__force void __user *)unsafe_ptr,
+						sizeof(buf));
+				break;
 			}
-			continue;
+
+			goto fmt_next;
 		}
 
 		if (fmt[i] == 'l') {
@@ -403,6 +432,7 @@ BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, fmt_size, u64, arg1,
 		if (fmt[i] != 'i' && fmt[i] != 'd' &&
 		    fmt[i] != 'u' && fmt[i] != 'x')
 			return -EINVAL;
+fmt_next:
 		fmt_cnt++;
 	}
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 7c488a1ce318..06161925225b 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2168,6 +2168,8 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
  *		f full name
  *		P node name, including a possible unit address
  * - 'x' For printing the address. Equivalent to "%lx".
+ * - 's[KU]' For printing a string, used in bpf_trace_printk(). For non-BPF
+ *           context this is equivalent to "%s".
  *
  * ** When making changes please also update:
  *	Documentation/core-api/printk-formats.rst
@@ -2180,8 +2182,11 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	      struct printf_spec spec)
 {
 	switch (*fmt) {
-	case 'S':
 	case 's':
+		if (fmt[1] == 'K' || fmt[1] == 'U')
+			return string(buf, end, ptr, spec);
+		/* Fallthrough */
+	case 'S':
 		ptr = dereference_symbol_descriptor(ptr);
 		/* Fallthrough */
 	case 'B':
-- 
2.21.0

RE: [PATCH bpf 2/3] bpf: add bpf_probe_read_{user, kernel}_str() to do_refine_retval_range

[John Fastabend]

Daniel Borkmann wrote:
> Given bpf_probe_read{,str}() BPF helpers are now only available under
> CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE, we need to add the drop-in
> replacements of bpf_probe_read_{kernel,user}_str() to do_refine_retval_range()
> as well to avoid hitting the same issue as in 849fa50662fbc ("bpf/verifier:
> refine retval R0 state for bpf_get_stack helper").
> 
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Cc: Yonghong Song <yhs@fb.com>
> ---

Acked-by: John Fastabend <john.fastabend@gmail.com>

Re: [PATCH bpf 0/3] Restrict bpf_probe_read{,str}() and bpf_trace_printk()'s %s

[Christoph Hellwig]

On Thu, May 14, 2020 at 06:16:04PM +0200, Daniel Borkmann wrote:
> Small set of fixes in order to restrict BPF helpers for tracing which are
> broken on archs with overlapping address ranges as per discussion in [0].
> I've targetted this for -bpf tree so they can be routed as fixes. Thanks!

Does that mean you are targeting them for 5.7?

Re: [PATCH bpf 2/3] bpf: add bpf_probe_read_{user, kernel}_str() to do_refine_retval_range

[Yonghong Song]

On 5/14/20 9:16 AM, Daniel Borkmann wrote:
> Given bpf_probe_read{,str}() BPF helpers are now only available under
> CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE, we need to add the drop-in
> replacements of bpf_probe_read_{kernel,user}_str() to do_refine_retval_range()
> as well to avoid hitting the same issue as in 849fa50662fbc ("bpf/verifier:
> refine retval R0 state for bpf_get_stack helper").
> 
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Cc: Yonghong Song <yhs@fb.com>

LGTM.
Acked-by: Yonghong Song <yhs@fb.com>

Re: [PATCH bpf 3/3] bpf: restrict bpf_trace_printk()'s %s usage and add %psK, %psU specifier

[Yonghong Song]

On 5/14/20 9:16 AM, Daniel Borkmann wrote:
> Usage of plain %s conversion specifier in bpf_trace_printk() suffers from the
> very same issue as bpf_probe_read{,str}() helpers, that is, it is broken on
> archs with overlapping address ranges.
> 
> While the helpers have been addressed through work in 6ae08ae3dea2 ("bpf: Add
> probe_read_{user, kernel} and probe_read_{user, kernel}_str helpers"), we need
> an option for bpf_trace_printk() as well to fix it.
> 
> Similarly as with the helpers, force users to make an explicit choice by adding
> %psK and %psU specifier to bpf_trace_printk() which will then pick the corresponding
> strncpy_from_unsafe*() variant to perform the access under KERNEL_DS or USER_DS.

In bpf_trace_printk(), we only print strings.

In bpf-next bpf_iter bpf_seq_printf() helper, introduced by
commit 492e639f0c22 ("bpf: Add bpf_seq_printf and bpf_seq_write 
helpers"), print strings and ip addresses %p{i,I}{4,6}.

Alan in
https://lore.kernel.org/bpf/alpine.LRH.2.21.2005141738050.23867@localhost/T
proposed BTF based type printing with a new format specifier
%pT, which potentially will be used in bpf_trace_printk() and 
bpf_seq_printf().

In the future, we may want to support more %p<...> format in these 
helpers. I am wondering whether we can have generic way so we only need 
to change lib/vsprintf.c once.

Maybe using %pk<...> to specify the kernel address and %pu<...> to
specify user address space. In the above example, we will have
%pks, %pus, %pki4 or %pui4, etc. Does this make sense?

> 
> Existing %s for legacy users is still kept working for archs where it is not
> broken and therefore gated through CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE.
> 
> Fixes: 8d3b7dce8622 ("bpf: add support for %s specifier to bpf_trace_printk()")
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Reported-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Brendan Gregg <brendan.d.gregg@gmail.com>
> Cc: Christoph Hellwig <hch@lst.de>
> ---
>   Documentation/core-api/printk-formats.rst | 14 ++++
>   kernel/trace/bpf_trace.c                  | 92 +++++++++++++++--------
>   lib/vsprintf.c                            |  7 +-
>   3 files changed, 81 insertions(+), 32 deletions(-)
> 
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index 8ebe46b1af39..76b5f4f265cb 100644
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -112,6 +112,20 @@ used when printing stack backtraces. The specifier takes into
>   consideration the effect of compiler optimisations which may occur
>   when tail-calls are used and marked with the noreturn GCC attribute.
>   
> +Probed Strings from BPF
> +-----------------------
> +
> +::
> +
> +	%psK	kernel_string
> +	%psU	user_string
> +
> +The ``sK`` and ``sU`` specifiers are used for printing a string from probed
> +memory. From regular vsnprintf(), they are equivalent to ``%s``, however,
> +when used out of BPF's bpf_trace_printk() it reads a string of up to 64 bytes
> +in memory without faulting. For ``K`` specifier, the string is probed out of
> +kernel memory whereas for ``U`` specifier, it is probed out of user memory.
> +
>   Kernel Pointers
>   ---------------
>   
[...]

Re: [PATCH bpf 1/3] bpf: restrict bpf_probe_read{,str}() only to archs where they work

[Linus Torvalds]

On Thu, May 14, 2020 at 9:18 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> However, their use should be restricted to archs with non-overlapping
> address ranges where they are working in their current form. Therefore,
> move this behind a CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE and
> have x86, arm64, arm select it (other archs supporting it can follow-up
> on it as well).

Ack, looks sane to me.

               Linus

Re: [PATCH bpf 0/3] Restrict bpf_probe_read{,str}() and bpf_trace_printk()'s %s

[Daniel Borkmann]

On 5/14/20 6:58 PM, Christoph Hellwig wrote:
> On Thu, May 14, 2020 at 06:16:04PM +0200, Daniel Borkmann wrote:
>> Small set of fixes in order to restrict BPF helpers for tracing which are
>> broken on archs with overlapping address ranges as per discussion in [0].
>> I've targetted this for -bpf tree so they can be routed as fixes. Thanks!
> 
> Does that mean you are targeting them for 5.7?

Yes, it would make most sense to me based on the discussion we had in the
other thread. If there is concern wrt latency we could route these to DaveM's
net tree in a timely manner (e.g. still tonight or so).

Thanks,
Daniel

Re: [PATCH bpf 0/3] Restrict bpf_probe_read{,str}() and bpf_trace_printk()'s %s

[Christoph Hellwig]

On Thu, May 14, 2020 at 09:54:06PM +0200, Daniel Borkmann wrote:
> On 5/14/20 6:58 PM, Christoph Hellwig wrote:
>> On Thu, May 14, 2020 at 06:16:04PM +0200, Daniel Borkmann wrote:
>>> Small set of fixes in order to restrict BPF helpers for tracing which are
>>> broken on archs with overlapping address ranges as per discussion in [0].
>>> I've targetted this for -bpf tree so they can be routed as fixes. Thanks!
>>
>> Does that mean you are targeting them for 5.7?
>
> Yes, it would make most sense to me based on the discussion we had in the
> other thread. If there is concern wrt latency we could route these to DaveM's
> net tree in a timely manner (e.g. still tonight or so).

I don't think we should rush this too much.  I just want to make sure
it either goes into 5.7 or that we have a coordinated tree that I can
base the maccess series on.

Re: [PATCH bpf 3/3] bpf: restrict bpf_trace_printk()'s %s usage and add %psK, %psU specifier

[Daniel Borkmann]

On 5/14/20 8:10 PM, Yonghong Song wrote:
> On 5/14/20 9:16 AM, Daniel Borkmann wrote:
>> Usage of plain %s conversion specifier in bpf_trace_printk() suffers from the
>> very same issue as bpf_probe_read{,str}() helpers, that is, it is broken on
>> archs with overlapping address ranges.
>>
>> While the helpers have been addressed through work in 6ae08ae3dea2 ("bpf: Add
>> probe_read_{user, kernel} and probe_read_{user, kernel}_str helpers"), we need
>> an option for bpf_trace_printk() as well to fix it.
>>
>> Similarly as with the helpers, force users to make an explicit choice by adding
>> %psK and %psU specifier to bpf_trace_printk() which will then pick the corresponding
>> strncpy_from_unsafe*() variant to perform the access under KERNEL_DS or USER_DS.
> 
> In bpf_trace_printk(), we only print strings.

Right ...

> In bpf-next bpf_iter bpf_seq_printf() helper, introduced by
> commit 492e639f0c22 ("bpf: Add bpf_seq_printf and bpf_seq_write helpers"), print strings and ip addresses %p{i,I}{4,6}.

... and here only kernel buffers.

> Alan in
> https://lore.kernel.org/bpf/alpine.LRH.2.21.2005141738050.23867@localhost/T
> proposed BTF based type printing with a new format specifier
> %pT, which potentially will be used in bpf_trace_printk() and bpf_seq_printf().
> 
> In the future, we may want to support more %p<...> format in these helpers. I am wondering whether we can have generic way so we only need to change lib/vsprintf.c once.
> 
> Maybe using %pk<...> to specify the kernel address and %pu<...> to
> specify user address space. In the above example, we will have
> %pks, %pus, %pki4 or %pui4, etc. Does this make sense?

Ah, right, once bpf merges back into bpf-next, we should consolidate these. I didn't want
to add the strncpy_from_unsafe*() right into lib/vsprintf.c since then we'd open it up to
all possible call-sites whereas it's really just needed out of bpf_trace_printk() for the
fix. I think it probably might make sense to add a generic lightweight layer to consolidate
all the bpf-related printk handling where we can statically specify a config e.g. as flags
of allowed specifiers which then internally takes care of checking the fmt specifiers for
sanity and does the probe read handling before passing down into lower layers. Thinking of
the case of adding %p{i,I}{4,6} to bpf_trace_printk(), for example, and assuming we'd had
a case where it needs to be probed out of both, then %pk<...> and %pu<...> modifier feels
reasonable indeed. I'll do a v2 to implement %pks and %pus instead.

Thanks,
Daniel

Re: [PATCH bpf 0/3] Restrict bpf_probe_read{,str}() and bpf_trace_printk()'s %s

[Daniel Borkmann]

On 5/14/20 9:58 PM, Christoph Hellwig wrote:
> On Thu, May 14, 2020 at 09:54:06PM +0200, Daniel Borkmann wrote:
>> On 5/14/20 6:58 PM, Christoph Hellwig wrote:
>>> On Thu, May 14, 2020 at 06:16:04PM +0200, Daniel Borkmann wrote:
>>>> Small set of fixes in order to restrict BPF helpers for tracing which are
>>>> broken on archs with overlapping address ranges as per discussion in [0].
>>>> I've targetted this for -bpf tree so they can be routed as fixes. Thanks!
>>>
>>> Does that mean you are targeting them for 5.7?
>>
>> Yes, it would make most sense to me based on the discussion we had in the
>> other thread. If there is concern wrt latency we could route these to DaveM's
>> net tree in a timely manner (e.g. still tonight or so).
> 
> I don't think we should rush this too much.  I just want to make sure
> it either goes into 5.7 or that we have a coordinated tree that I can
> base the maccess series on.

Yep, makes sense, we'll target 5.7 for it.

Thanks,
Daniel

Re: [PATCH bpf 1/3] bpf: restrict bpf_probe_read{,str}() only to archs where they work

[Masami Hiramatsu]

On Thu, 14 May 2020 18:16:05 +0200
Daniel Borkmann <daniel@iogearbox.net> wrote:

> Given the legacy bpf_probe_read{,str}() BPF helpers are broken on archs
> with overlapping address ranges, we should really take the next step to
> disable them from BPF use there.
> 
> To generally fix the situation, we've recently added new helper variants
> bpf_probe_read_{user,kernel}() and bpf_probe_read_{user,kernel}_str().
> For details on them, see 6ae08ae3dea2 ("bpf: Add probe_read_{user, kernel}
> and probe_read_{user,kernel}_str helpers").
> 
> Given bpf_probe_read{,str}() have been around for ~5 years by now, there
> are plenty of users at least on x86 still relying on them today, so we
> cannot remove them entirely w/o breaking the BPF tracing ecosystem.
> 
> However, their use should be restricted to archs with non-overlapping
> address ranges where they are working in their current form. Therefore,
> move this behind a CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE and
> have x86, arm64, arm select it (other archs supporting it can follow-up
> on it as well).
> 
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Brendan Gregg <brendan.d.gregg@gmail.com>
> Cc: Christoph Hellwig <hch@lst.de>

Thanks for the config! Looks good to me.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>


> ---
>  arch/arm/Kconfig         | 1 +
>  arch/arm64/Kconfig       | 1 +
>  arch/x86/Kconfig         | 1 +
>  init/Kconfig             | 3 +++
>  kernel/trace/bpf_trace.c | 6 ++++--
>  5 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 66a04f6f4775..c77c93c485a0 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -12,6 +12,7 @@ config ARM
>  	select ARCH_HAS_KEEPINITRD
>  	select ARCH_HAS_KCOV
>  	select ARCH_HAS_MEMBARRIER_SYNC_CORE
> +	select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
>  	select ARCH_HAS_PTE_SPECIAL if ARM_LPAE
>  	select ARCH_HAS_PHYS_TO_DMA
>  	select ARCH_HAS_SETUP_DMA_OPS
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 40fb05d96c60..5d513f461957 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -20,6 +20,7 @@ config ARM64
>  	select ARCH_HAS_KCOV
>  	select ARCH_HAS_KEEPINITRD
>  	select ARCH_HAS_MEMBARRIER_SYNC_CORE
> +	select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
>  	select ARCH_HAS_PTE_DEVMAP
>  	select ARCH_HAS_PTE_SPECIAL
>  	select ARCH_HAS_SETUP_DMA_OPS
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 1197b5596d5a..2d3f963fd6f1 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -68,6 +68,7 @@ config X86
>  	select ARCH_HAS_KCOV			if X86_64
>  	select ARCH_HAS_MEM_ENCRYPT
>  	select ARCH_HAS_MEMBARRIER_SYNC_CORE
> +	select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
>  	select ARCH_HAS_PMEM_API		if X86_64
>  	select ARCH_HAS_PTE_DEVMAP		if X86_64
>  	select ARCH_HAS_PTE_SPECIAL
> diff --git a/init/Kconfig b/init/Kconfig
> index 9e22ee8fbd75..6fd13a051342 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -2279,6 +2279,9 @@ config ASN1
>  
>  source "kernel/Kconfig.locks"
>  
> +config ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
> +	bool
> +
>  config ARCH_HAS_SYNC_CORE_BEFORE_USERMODE
>  	bool
>  
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index ca1796747a77..b83bdaa31c7b 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -825,14 +825,16 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>  		return &bpf_probe_read_user_proto;
>  	case BPF_FUNC_probe_read_kernel:
>  		return &bpf_probe_read_kernel_proto;
> -	case BPF_FUNC_probe_read:
> -		return &bpf_probe_read_compat_proto;
>  	case BPF_FUNC_probe_read_user_str:
>  		return &bpf_probe_read_user_str_proto;
>  	case BPF_FUNC_probe_read_kernel_str:
>  		return &bpf_probe_read_kernel_str_proto;
> +#ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
> +	case BPF_FUNC_probe_read:
> +		return &bpf_probe_read_compat_proto;
>  	case BPF_FUNC_probe_read_str:
>  		return &bpf_probe_read_compat_str_proto;
> +#endif
>  #ifdef CONFIG_CGROUPS
>  	case BPF_FUNC_get_current_cgroup_id:
>  		return &bpf_get_current_cgroup_id_proto;
> -- 
> 2.21.0
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>