linux-kernel - Re: [PATCH] tracing: Check for NULL field_name in __synth_event_add

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20230219205608.4664c310@rorschach.local.home>
Date:   Sun, 19 Feb 2023 20:56:08 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Tom Zanussi <zanussi@...nel.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Linux Trace Kernel <linux-trace-kernel@...r.kernel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>, ionut_n2001@...oo.com
Subject: Re: [PATCH] tracing: Check for NULL field_name in
 __synth_event_add_val()

On Sun, 19 Feb 2023 15:46:24 -0600
Tom Zanussi <zanussi@...nel.org> wrote:


> No, because this code just above it makes sure you can't mix add_name
> with add_next.  Once add_name is set it will return -EINVAL if
> field_name is ever null after that, and add_name will never be changed
> once set:
> 
>        /* can't mix add_next_synth_val() with add_synth_val() */
>         if (field_name) {
>                 if (trace_state->add_next) {
>                         ret = -EINVAL;
>                         goto out;
>                 }
>                 trace_state->add_name = true;
>         } else {
>                 if (trace_state->add_name) {
>                         ret = -EINVAL;
>                         goto out;
>                 }
>                 trace_state->add_next = true;
>         }
> 
> 
> >  kernel/trace/trace_events_synth.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/kernel/trace/trace_events_synth.c
> > b/kernel/trace/trace_events_synth.c
> > index 70bddb25d9c0..fa28c1da06d2 100644
> > --- a/kernel/trace/trace_events_synth.c
> > +++ b/kernel/trace/trace_events_synth.c
> > @@ -1982,6 +1982,10 @@ static int __synth_event_add_val(const char
> > *field_name, u64 val,
> >  
> >         event = trace_state->event;
> >         if (trace_state->add_name) {
> > +               if (!field_name) {
> > +                       ret = -EINVAL;
> > +                       goto out;
> > +               }  
> 
> So if add_name is set here, it must also mean that field_name can't be
> null, because of the above.
> 
> >                 for (i = 0; i < event->n_fields; i++) {
> >                         field = event->fields[i];
> >                         if (strcmp(field->name, field_name) == 0)  
> 
> And if field_name can't be null, then I don't see how this strcmp could
> fail due to a null field_name.
> 
> So I don't see the need for this patch.  The bugzilla shows a compiler
> warning when using -Wnonnull - could this just be a spurious gcc
> warning?

Thanks, I should have caught that (I was even looking for that logic,
but still missed it). That's what I get for writing patches while jet-lagged :-p

-- Steve