| Message ID | 20250327114911.2c713511@gandalf.local.home (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tracing: Verify event formats that have "%*p.." | expand |
On 3/27/25 08:49, Steven Rostedt wrote: > From: Steven Rostedt <rostedt@goodmis.org> > > The trace event verifier checks the formats of trace events to make sure > that they do not point at memory that is not in the trace event itself or > in data that will never be freed. If an event references data that was > allocated when the event triggered and that same data is freed before the > event is read, then the kernel can crash by reading freed memory. > > The verifier runs at boot up (or module load) and scans the print formats > of the events and checks their arguments to make sure that dereferenced > pointers are safe. If the format uses "%*p.." the verifier will ignore it, > and that could be dangerous. Cover this case as well. > > Also add to the sample code a use case of "%*pbl". > > Link: https://urldefense.com/v3/__https://lore.kernel.org/all/bcba4d76-2c3f-4d11-baf0-02905db953dd@oracle.com/__;!!ACWV5N9M2RV99hQ!LTIVO1_O9wY2hBAnpNF5zcB1EFlC77zOnu4oVcM4DoD77p5ZO_m1LFZdPVJPj4spzye4JntXHOqOVxirPQ$ > Thanks Steve, a minor style issue below, otherwise LGTM. I have tested it, it works. Reviewed-by: Libo Chen <libo.chen@oracle.com> Tested-by: Libo Chen <libo.chen@oracle.com> -- Libo > Reported-by: Libo Chen <libo.chen@oracle.com> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > --- > kernel/trace/trace_events.c | 7 +++++++ > samples/trace_events/trace-events-sample.h | 8 ++++++-- > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c > index 7b3ef1d26167..34e7b4af9f88 100644 > --- a/kernel/trace/trace_events.c > +++ b/kernel/trace/trace_events.c > @@ -470,6 +470,7 @@ static void test_event_printk(struct trace_event_call *call) > case '%': > continue; > case 'p': > + do_pointer: > /* Find dereferencing fields */ > switch (fmt[i + 1]) { > case 'B': case 'R': case 'r': > @@ -498,6 +499,12 @@ static void test_event_printk(struct trace_event_call *call) > continue; > if (fmt[i + j] == '*') { > star = true; > + /* Handle %*pbl case */ > + if (!j && fmt[i + 1] == 'p') { > + arg++; > + i++; > + goto do_pointer; > + } > continue; > } > if ((fmt[i + j] == 's')) { > diff --git a/samples/trace_events/trace-events-sample.h b/samples/trace_events/trace-events-sample.h > index 999f78d380ae..0622c9e3f2be 100644 > --- a/samples/trace_events/trace-events-sample.h > +++ b/samples/trace_events/trace-events-sample.h > @@ -319,7 +319,8 @@ TRACE_EVENT(foo_bar, > __assign_cpumask(cpum, cpumask_bits(mask)); > ), > > - TP_printk("foo %s %d %s %s %s %s %s %s (%s) (%s) %s", __entry->foo, __entry->bar, > + TP_printk("foo %s %d %s %s %s %s %s %s (%s) (%s) %s [%d] %*pbl", > + __entry->foo, __entry->bar, > > /* > * Notice here the use of some helper functions. This includes: > @@ -370,7 +371,10 @@ TRACE_EVENT(foo_bar, > > __get_str(str), __get_str(lstr), > __get_bitmask(cpus), __get_cpumask(cpum), > - __get_str(vstr)) > + __get_str(vstr), > + __get_dynamic_array_len(cpus), > + __get_dynamic_array_len(cpus), > + __get_dynamic_array(cpus)) __get_dynamic_array_len() calls don't seem to be aligned with the ones above > ); > > /*
On Thu, 27 Mar 2025 11:32:23 -0700 Libo Chen <libo.chen@oracle.com> wrote: > On 3/27/25 08:49, Steven Rostedt wrote: > > From: Steven Rostedt <rostedt@goodmis.org> > > > > The trace event verifier checks the formats of trace events to make sure > > that they do not point at memory that is not in the trace event itself or > > in data that will never be freed. If an event references data that was > > allocated when the event triggered and that same data is freed before the > > event is read, then the kernel can crash by reading freed memory. > > > > The verifier runs at boot up (or module load) and scans the print formats > > of the events and checks their arguments to make sure that dereferenced > > pointers are safe. If the format uses "%*p.." the verifier will ignore it, > > and that could be dangerous. Cover this case as well. > > > > Also add to the sample code a use case of "%*pbl". > > > > Link: https://urldefense.com/v3/__https://lore.kernel.org/all/bcba4d76-2c3f-4d11-baf0-02905db953dd@oracle.com/__;!!ACWV5N9M2RV99hQ!LTIVO1_O9wY2hBAnpNF5zcB1EFlC77zOnu4oVcM4DoD77p5ZO_m1LFZdPVJPj4spzye4JntXHOqOVxirPQ$ > > > Thanks Steve, a minor style issue below, otherwise LGTM. I have tested it, it works. > > Reviewed-by: Libo Chen <libo.chen@oracle.com> > Tested-by: Libo Chen <libo.chen@oracle.com> Thanks for testing. > > /* > > * Notice here the use of some helper functions. This includes: > > @@ -370,7 +371,10 @@ TRACE_EVENT(foo_bar, > > > > __get_str(str), __get_str(lstr), > > __get_bitmask(cpus), __get_cpumask(cpum), > > - __get_str(vstr)) > > + __get_str(vstr), > > + __get_dynamic_array_len(cpus), > > + __get_dynamic_array_len(cpus), > > + __get_dynamic_array(cpus)) > > __get_dynamic_array_len() calls don't seem to be aligned with the ones above > Ah yeah. I'll update. Note, this isn't going to go into the kernel until at least 6.16. -- Steve
On 3/27/25 11:38, Steven Rostedt wrote: > On Thu, 27 Mar 2025 11:32:23 -0700 > Libo Chen <libo.chen@oracle.com> wrote: > >> On 3/27/25 08:49, Steven Rostedt wrote: >>> From: Steven Rostedt <rostedt@goodmis.org> >>> >>> The trace event verifier checks the formats of trace events to make sure >>> that they do not point at memory that is not in the trace event itself or >>> in data that will never be freed. If an event references data that was >>> allocated when the event triggered and that same data is freed before the >>> event is read, then the kernel can crash by reading freed memory. >>> >>> The verifier runs at boot up (or module load) and scans the print formats >>> of the events and checks their arguments to make sure that dereferenced >>> pointers are safe. If the format uses "%*p.." the verifier will ignore it, >>> and that could be dangerous. Cover this case as well. >>> >>> Also add to the sample code a use case of "%*pbl". >>> >>> Link: https://urldefense.com/v3/__https://lore.kernel.org/all/bcba4d76-2c3f-4d11-baf0-02905db953dd@oracle.com/__;!!ACWV5N9M2RV99hQ!LTIVO1_O9wY2hBAnpNF5zcB1EFlC77zOnu4oVcM4DoD77p5ZO_m1LFZdPVJPj4spzye4JntXHOqOVxirPQ$ >>> >> Thanks Steve, a minor style issue below, otherwise LGTM. I have tested it, it works. >> >> Reviewed-by: Libo Chen <libo.chen@oracle.com> >> Tested-by: Libo Chen <libo.chen@oracle.com> > > Thanks for testing. > >>> /* >>> * Notice here the use of some helper functions. This includes: >>> @@ -370,7 +371,10 @@ TRACE_EVENT(foo_bar, >>> >>> __get_str(str), __get_str(lstr), >>> __get_bitmask(cpus), __get_cpumask(cpum), >>> - __get_str(vstr)) >>> + __get_str(vstr), >>> + __get_dynamic_array_len(cpus), >>> + __get_dynamic_array_len(cpus), >>> + __get_dynamic_array(cpus)) >> >> __get_dynamic_array_len() calls don't seem to be aligned with the ones above >> > > Ah yeah. I'll update. Note, this isn't going to go into the kernel until at > least 6.16. > Okay thanks for the note. I will stick to %lx for now and change it to %*pbl once this gets in. Libo > -- Steve
On Thu, 27 Mar 2025 14:00:44 -0700 Libo Chen <libo.chen@oracle.com> wrote: > > Ah yeah. I'll update. Note, this isn't going to go into the kernel until at > > least 6.16. > > > > Okay thanks for the note. I will stick to %lx for now and change it to %*pbl > once this gets in. This shouldn't be a blocker for you. Do you get any errors? Hmm, if this causes bugs (perhaps you have arguments after the %*pbl) then I can add this as a fix and not an enhancement. -- Steve
On 3/27/25 14:20, Steven Rostedt wrote: > On Thu, 27 Mar 2025 14:00:44 -0700 > Libo Chen <libo.chen@oracle.com> wrote: > >>> Ah yeah. I'll update. Note, this isn't going to go into the kernel until at >>> least 6.16. >>> >> >> Okay thanks for the note. I will stick to %lx for now and change it to %*pbl >> once this gets in. > > This shouldn't be a blocker for you. Do you get any errors? > > Hmm, if this causes bugs (perhaps you have arguments after the %*pbl) then > I can add this as a fix and not an enhancement. Not sure if I will call it an error. But without this patch, I will just get "nil" when using %*p so this information is lost here. If I go ahead with %*pbl, nodemask value will not show up in the trace until this one is merged. Libo > > -- Steve
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c index 7b3ef1d26167..34e7b4af9f88 100644 --- a/kernel/trace/trace_events.c +++ b/kernel/trace/trace_events.c @@ -470,6 +470,7 @@ static void test_event_printk(struct trace_event_call *call) case '%': continue; case 'p': + do_pointer: /* Find dereferencing fields */ switch (fmt[i + 1]) { case 'B': case 'R': case 'r': @@ -498,6 +499,12 @@ static void test_event_printk(struct trace_event_call *call) continue; if (fmt[i + j] == '*') { star = true; + /* Handle %*pbl case */ + if (!j && fmt[i + 1] == 'p') { + arg++; + i++; + goto do_pointer; + } continue; } if ((fmt[i + j] == 's')) { diff --git a/samples/trace_events/trace-events-sample.h b/samples/trace_events/trace-events-sample.h index 999f78d380ae..0622c9e3f2be 100644 --- a/samples/trace_events/trace-events-sample.h +++ b/samples/trace_events/trace-events-sample.h @@ -319,7 +319,8 @@ TRACE_EVENT(foo_bar, __assign_cpumask(cpum, cpumask_bits(mask)); ), - TP_printk("foo %s %d %s %s %s %s %s %s (%s) (%s) %s", __entry->foo, __entry->bar, + TP_printk("foo %s %d %s %s %s %s %s %s (%s) (%s) %s [%d] %*pbl", + __entry->foo, __entry->bar, /* * Notice here the use of some helper functions. This includes: @@ -370,7 +371,10 @@ TRACE_EVENT(foo_bar, __get_str(str), __get_str(lstr), __get_bitmask(cpus), __get_cpumask(cpum), - __get_str(vstr)) + __get_str(vstr), + __get_dynamic_array_len(cpus), + __get_dynamic_array_len(cpus), + __get_dynamic_array(cpus)) ); /*