diff mbox

[RFC,2/2] bpf: Initialise mod[] in bpf_trace_printk

Message ID 598DDF88.6020809@iogearbox.net
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Daniel Borkmann Aug. 11, 2017, 4:47 p.m. UTC
Hi James,

On 08/09/2017 10:34 PM, Daniel Borkmann wrote:
> On 08/09/2017 09:39 AM, James Hogan wrote:
> [...]
>> time (but please consider looking at the other patch which is certainly
>> a more real issue).
>
> Sorry for the delay, started looking into that and whether we
> have some other options, I'll get back to you on this.

Could we solve this more generically (as in: originally intended) in
the sense that we don't need to trust the gcc va_list handling; I feel
this is relying on an implementation detail? Perhaps something like
below poc patch?

Thanks again,
Daniel

 From 71f16544d455abb6bb82f7253c17c14d2a395e91 Mon Sep 17 00:00:00 2001
Message-Id: <71f16544d455abb6bb82f7253c17c14d2a395e91.1502469361.git.daniel@iogearbox.net>
From: Daniel Borkmann <daniel@iogearbox.net>
Date: Fri, 11 Aug 2017 15:56:32 +0200
Subject: [PATCH] bpf: fix bpf_trace_printk on 32 bit

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
  kernel/trace/bpf_trace.c | 31 +++++++++++++++++++++++++++----
  1 file changed, 27 insertions(+), 4 deletions(-)

Comments

James Hogan Aug. 14, 2017, 12:25 p.m. UTC | #1
On Fri, Aug 11, 2017 at 06:47:04PM +0200, Daniel Borkmann wrote:
> Hi James,
> 
> On 08/09/2017 10:34 PM, Daniel Borkmann wrote:
> > On 08/09/2017 09:39 AM, James Hogan wrote:
> > [...]
> >> time (but please consider looking at the other patch which is certainly
> >> a more real issue).
> >
> > Sorry for the delay, started looking into that and whether we
> > have some other options, I'll get back to you on this.
> 
> Could we solve this more generically (as in: originally intended) in
> the sense that we don't need to trust the gcc va_list handling; I feel
> this is relying on an implementation detail? Perhaps something like
> below poc patch?

Well it works on MIPS32 and MIPS64 with tracex5.

Tested-by: James Hogan <james.hogan@imgtec.com>

Cheers
James

> 
> Thanks again,
> Daniel
> 
>  From 71f16544d455abb6bb82f7253c17c14d2a395e91 Mon Sep 17 00:00:00 2001
> Message-Id: <71f16544d455abb6bb82f7253c17c14d2a395e91.1502469361.git.daniel@iogearbox.net>
> From: Daniel Borkmann <daniel@iogearbox.net>
> Date: Fri, 11 Aug 2017 15:56:32 +0200
> Subject: [PATCH] bpf: fix bpf_trace_printk on 32 bit
> 
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
>   kernel/trace/bpf_trace.c | 31 +++++++++++++++++++++++++++----
>   1 file changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 3738519..d4cb36f 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -204,10 +204,33 @@ static const struct bpf_func_proto *bpf_get_probe_write_proto(void)
>   		fmt_cnt++;
>   	}
> 
> -	return __trace_printk(1/* fake ip will not be printed */, fmt,
> -			      mod[0] == 2 ? arg1 : mod[0] == 1 ? (long) arg1 : (u32) arg1,
> -			      mod[1] == 2 ? arg2 : mod[1] == 1 ? (long) arg2 : (u32) arg2,
> -			      mod[2] == 2 ? arg3 : mod[2] == 1 ? (long) arg3 : (u32) arg3);
> +#define __BPF_TP_EMIT()	__BPF_ARG3_TP()
> +#define __BPF_TP(...)							\
> +	__trace_printk(1 /* fake ip will not be printed */,		\
> +		       fmt, ##__VA_ARGS__)
> +
> +#define __BPF_ARG1_TP(...)						\
> +	((mod[0] == 2 || (mod[0] == 1 && __BITS_PER_LONG == 64))	\
> +	  ? __BPF_TP(arg1, ##__VA_ARGS__)				\
> +	  : ((mod[0] == 1 || (mod[0] == 0 && __BITS_PER_LONG == 32))	\
> +	      ? __BPF_TP((long)arg1, ##__VA_ARGS__)			\
> +	      : __BPF_TP((u32)arg1, ##__VA_ARGS__)))
> +
> +#define __BPF_ARG2_TP(...)						\
> +	((mod[1] == 2 || (mod[1] == 1 && __BITS_PER_LONG == 64))	\
> +	  ? __BPF_ARG1_TP(arg2, ##__VA_ARGS__)				\
> +	  : ((mod[1] == 1 || (mod[1] == 0 && __BITS_PER_LONG == 32))	\
> +	      ? __BPF_ARG1_TP((long)arg2, ##__VA_ARGS__)		\
> +	      : __BPF_ARG1_TP((u32)arg2, ##__VA_ARGS__)))
> +
> +#define __BPF_ARG3_TP(...)						\
> +	((mod[2] == 2 || (mod[2] == 1 && __BITS_PER_LONG == 64))	\
> +	  ? __BPF_ARG2_TP(arg3, ##__VA_ARGS__)				\
> +	  : ((mod[2] == 1 || (mod[2] == 0 && __BITS_PER_LONG == 32))	\
> +	      ? __BPF_ARG2_TP((long)arg3, ##__VA_ARGS__)		\
> +	      : __BPF_ARG2_TP((u32)arg3, ##__VA_ARGS__)))
> +
> +	return __BPF_TP_EMIT();
>   }
> 
>   static const struct bpf_func_proto bpf_trace_printk_proto = {
> -- 
> 1.9.3
David Laight Aug. 14, 2017, 12:44 p.m. UTC | #2
From: Daniel Borkmann

> Sent: 11 August 2017 17:47

> On 08/09/2017 10:34 PM, Daniel Borkmann wrote:

> > On 08/09/2017 09:39 AM, James Hogan wrote:

> > [...]

> >> time (but please consider looking at the other patch which is certainly

> >> a more real issue).

> >

> > Sorry for the delay, started looking into that and whether we

> > have some other options, I'll get back to you on this.

> 

> Could we solve this more generically (as in: originally intended) in

> the sense that we don't need to trust the gcc va_list handling; I feel

> this is relying on an implementation detail? Perhaps something like

> below poc patch?


That patch still has 'cond ? arg : cond1 ? (long)arg : (u32)arg' so
probably has the same warning as the original version.

The va_list handling is defined by the relevant ABI, not gcc.

It is ok on x86-64 because all 32bit values are extended to 64bits
before being passed as arguments (either in registers, or on the stack).
Nothing in the C language requires that, so other 64bit architectures
could pass 32bit values in 4 bytes of stack.

	David
diff mbox

Patch

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 3738519..d4cb36f 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -204,10 +204,33 @@  static const struct bpf_func_proto *bpf_get_probe_write_proto(void)
  		fmt_cnt++;
  	}

-	return __trace_printk(1/* fake ip will not be printed */, fmt,
-			      mod[0] == 2 ? arg1 : mod[0] == 1 ? (long) arg1 : (u32) arg1,
-			      mod[1] == 2 ? arg2 : mod[1] == 1 ? (long) arg2 : (u32) arg2,
-			      mod[2] == 2 ? arg3 : mod[2] == 1 ? (long) arg3 : (u32) arg3);
+#define __BPF_TP_EMIT()	__BPF_ARG3_TP()
+#define __BPF_TP(...)							\
+	__trace_printk(1 /* fake ip will not be printed */,		\
+		       fmt, ##__VA_ARGS__)
+
+#define __BPF_ARG1_TP(...)						\
+	((mod[0] == 2 || (mod[0] == 1 && __BITS_PER_LONG == 64))	\
+	  ? __BPF_TP(arg1, ##__VA_ARGS__)				\
+	  : ((mod[0] == 1 || (mod[0] == 0 && __BITS_PER_LONG == 32))	\
+	      ? __BPF_TP((long)arg1, ##__VA_ARGS__)			\
+	      : __BPF_TP((u32)arg1, ##__VA_ARGS__)))
+
+#define __BPF_ARG2_TP(...)						\
+	((mod[1] == 2 || (mod[1] == 1 && __BITS_PER_LONG == 64))	\
+	  ? __BPF_ARG1_TP(arg2, ##__VA_ARGS__)				\
+	  : ((mod[1] == 1 || (mod[1] == 0 && __BITS_PER_LONG == 32))	\
+	      ? __BPF_ARG1_TP((long)arg2, ##__VA_ARGS__)		\
+	      : __BPF_ARG1_TP((u32)arg2, ##__VA_ARGS__)))
+
+#define __BPF_ARG3_TP(...)						\
+	((mod[2] == 2 || (mod[2] == 1 && __BITS_PER_LONG == 64))	\
+	  ? __BPF_ARG2_TP(arg3, ##__VA_ARGS__)				\
+	  : ((mod[2] == 1 || (mod[2] == 0 && __BITS_PER_LONG == 32))	\
+	      ? __BPF_ARG2_TP((long)arg3, ##__VA_ARGS__)		\
+	      : __BPF_ARG2_TP((u32)arg3, ##__VA_ARGS__)))
+
+	return __BPF_TP_EMIT();
  }

  static const struct bpf_func_proto bpf_trace_printk_proto = {