| Message ID | 20170807222514.24292-3-james.hogan@imgtec.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On 08/08/2017 12:25 AM, James Hogan wrote: > In bpf_trace_printk(), the elements in mod[] are left uninitialised, but > they are then incremented to track the width of the formats. Zero > initialise the array just in case the memory contains non-zero values on > entry. > > Fixes: 9c959c863f82 ("tracing: Allow BPF programs to call bpf_trace_printk()") > Signed-off-by: James Hogan <james.hogan@imgtec.com> > Cc: Alexei Starovoitov <ast@kernel.org> > Cc: Daniel Borkmann <daniel@iogearbox.net> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: netdev@vger.kernel.org > --- > When I checked (on MIPS32), the elements tended to have the value zero > anyway (does BPF zero the stack or something clever?), so this is a > purely theoretical fix. > --- > kernel/trace/bpf_trace.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 32dcbe1b48f2..86a52857d941 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -129,7 +129,7 @@ BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, fmt_size, u64, arg1, > u64, arg2, u64, arg3) > { > bool str_seen = false; > - int mod[3] = {}; > + int mod[3] = { 0, 0, 0 }; I'm probably missing something, but is the behavior of gcc wrt above initializers different on mips (it zeroes just fine on x86 at least)? If yes, we'd probably need a cocci script to also check rest of the kernel given this is used in a number of places. Hm, could you elaborate? > int fmt_cnt = 0; > u64 unsafe_addr; > char buf[64]; >
From: Daniel Borkmann <daniel@iogearbox.net> Date: Tue, 08 Aug 2017 10:46:52 +0200 > On 08/08/2017 12:25 AM, James Hogan wrote: >> In bpf_trace_printk(), the elements in mod[] are left uninitialised, >> but >> they are then incremented to track the width of the formats. Zero >> initialise the array just in case the memory contains non-zero values >> on >> entry. >> >> Fixes: 9c959c863f82 ("tracing: Allow BPF programs to call >> bpf_trace_printk()") >> Signed-off-by: James Hogan <james.hogan@imgtec.com> >> Cc: Alexei Starovoitov <ast@kernel.org> >> Cc: Daniel Borkmann <daniel@iogearbox.net> >> Cc: Steven Rostedt <rostedt@goodmis.org> >> Cc: Ingo Molnar <mingo@redhat.com> >> Cc: netdev@vger.kernel.org >> --- >> When I checked (on MIPS32), the elements tended to have the value zero >> anyway (does BPF zero the stack or something clever?), so this is a >> purely theoretical fix. >> --- >> kernel/trace/bpf_trace.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c >> index 32dcbe1b48f2..86a52857d941 100644 >> --- a/kernel/trace/bpf_trace.c >> +++ b/kernel/trace/bpf_trace.c >> @@ -129,7 +129,7 @@ BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, >> fmt_size, u64, arg1, >> u64, arg2, u64, arg3) >> { >> bool str_seen = false; >> - int mod[3] = {}; >> + int mod[3] = { 0, 0, 0 }; > > I'm probably missing something, but is the behavior of gcc wrt > above initializers different on mips (it zeroes just fine on x86 > at least)? If yes, we'd probably need a cocci script to also check > rest of the kernel given this is used in a number of places. Hm, > could you elaborate? This change is not necessary at all. An empty initializer must clear the whole object to zero. "theoretical" fix indeed... :-(
On 8 August 2017 17:48:57 BST, David Miller <davem@davemloft.net> wrote: >From: Daniel Borkmann <daniel@iogearbox.net> >Date: Tue, 08 Aug 2017 10:46:52 +0200 > >> On 08/08/2017 12:25 AM, James Hogan wrote: >>> In bpf_trace_printk(), the elements in mod[] are left uninitialised, >>> but >>> they are then incremented to track the width of the formats. Zero >>> initialise the array just in case the memory contains non-zero >values >>> on >>> entry. >>> >>> Fixes: 9c959c863f82 ("tracing: Allow BPF programs to call >>> bpf_trace_printk()") >>> Signed-off-by: James Hogan <james.hogan@imgtec.com> >>> Cc: Alexei Starovoitov <ast@kernel.org> >>> Cc: Daniel Borkmann <daniel@iogearbox.net> >>> Cc: Steven Rostedt <rostedt@goodmis.org> >>> Cc: Ingo Molnar <mingo@redhat.com> >>> Cc: netdev@vger.kernel.org >>> --- >>> When I checked (on MIPS32), the elements tended to have the value >zero >>> anyway (does BPF zero the stack or something clever?), so this is a >>> purely theoretical fix. >>> --- >>> kernel/trace/bpf_trace.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c >>> index 32dcbe1b48f2..86a52857d941 100644 >>> --- a/kernel/trace/bpf_trace.c >>> +++ b/kernel/trace/bpf_trace.c >>> @@ -129,7 +129,7 @@ BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, >>> fmt_size, u64, arg1, >>> u64, arg2, u64, arg3) >>> { >>> bool str_seen = false; >>> - int mod[3] = {}; >>> + int mod[3] = { 0, 0, 0 }; >> >> I'm probably missing something, but is the behavior of gcc wrt >> above initializers different on mips (it zeroes just fine on x86 >> at least)? If yes, we'd probably need a cocci script to also check >> rest of the kernel given this is used in a number of places. Hm, >> could you elaborate? > >This change is not necessary at all. > >An empty initializer must clear the whole object to zero. > >"theoretical" fix indeed... :-( cool, i hadn't realised unmentioned elements in an initialiser are always zeroed, even when non-global/static, so had interpreted the whole array as uninitialised. learn something new every day :-) sorry for the noise. cheers James
From: James Hogan <james.hogan@imgtec.com> Date: Tue, 08 Aug 2017 22:20:05 +0100 > cool, i hadn't realised unmentioned elements in an initialiser are > always zeroed, even when non-global/static, so had interpreted the > whole array as uninitialised. learn something new every day :-) > sorry for the noise. You didn't have to know in the first place, you could have simply compiled the code into assembler by running: make kernel/trace/bpf_trace.s and seen for yourself before putting all of this time and effort into this patch and discussion. If you don't know what the compiler does, simply look!
On Tue, Aug 08, 2017 at 02:54:33PM -0700, David Miller wrote: > From: James Hogan <james.hogan@imgtec.com> > Date: Tue, 08 Aug 2017 22:20:05 +0100 > > > cool, i hadn't realised unmentioned elements in an initialiser are > > always zeroed, even when non-global/static, so had interpreted the > > whole array as uninitialised. learn something new every day :-) > > sorry for the noise. > > You didn't have to know in the first place, you could have simply > compiled the code into assembler by running: > > make kernel/trace/bpf_trace.s > > and seen for yourself before putting all of this time and effort into > this patch and discussion. > > If you don't know what the compiler does, simply look! Well, thats the danger of wrongly thinking I already knew what it did in this case. Anyway like I said, I'm sorry for the noise and wasting your time (but please consider looking at the other patch which is certainly a more real issue). Thanks James
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. Thanks, Daniel
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 32dcbe1b48f2..86a52857d941 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -129,7 +129,7 @@ BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, fmt_size, u64, arg1, u64, arg2, u64, arg3) { bool str_seen = false; - int mod[3] = {}; + int mod[3] = { 0, 0, 0 }; int fmt_cnt = 0; u64 unsafe_addr; char buf[64];
In bpf_trace_printk(), the elements in mod[] are left uninitialised, but they are then incremented to track the width of the formats. Zero initialise the array just in case the memory contains non-zero values on entry. Fixes: 9c959c863f82 ("tracing: Allow BPF programs to call bpf_trace_printk()") Signed-off-by: James Hogan <james.hogan@imgtec.com> Cc: Alexei Starovoitov <ast@kernel.org> Cc: Daniel Borkmann <daniel@iogearbox.net> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Ingo Molnar <mingo@redhat.com> Cc: netdev@vger.kernel.org --- When I checked (on MIPS32), the elements tended to have the value zero anyway (does BPF zero the stack or something clever?), so this is a purely theoretical fix. --- kernel/trace/bpf_trace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)