linux-kernel - Re: [PATCH v4 1/3] tracing: eprobe: remove duplicate is_good

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <e5501570657870e3cdbbce591b0def973f5a20b6.camel@kernel.org>
Date:   Mon, 20 Jun 2022 13:38:50 -0500
From:   Tom Zanussi <zanussi@...nel.org>
To:     Linyu Yuan <quic_linyyuan@...cinc.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Ingo Molnar <mingo@...hat.com>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/3] tracing: eprobe: remove duplicate is_good_name()
 operation

Hi Linyu,

On Tue, 2022-06-14 at 08:48 +0800, Linyu Yuan wrote:
> hi Tom,
> 
> On 6/14/2022 5:01 AM, Tom Zanussi wrote:
> > Hi Linhu,
> > 
> > On Thu, 2022-06-02 at 20:10 +0800, Linyu Yuan wrote:
> > > traceprobe_parse_event_name() already validate group and event
> > > name,
> > > there is no need to call is_good_name() after it.
> > > 
> > > Signed-off-by: Linyu Yuan <quic_linyyuan@...cinc.com>
> > > ---
> > > v2: drop v1 change as it is NACK.
> > >      add it to remove duplicate is_good_name().
> > > v3: move it as first patch.
> > > v4: no change
> > > 
> > >   kernel/trace/trace_eprobe.c | 4 ----
> > >   1 file changed, 4 deletions(-)
> > > 
> > > diff --git a/kernel/trace/trace_eprobe.c
> > > b/kernel/trace/trace_eprobe.c
> > > index 7d44785..17d64e3 100644
> > > --- a/kernel/trace/trace_eprobe.c
> > > +++ b/kernel/trace/trace_eprobe.c
> > > @@ -878,16 +878,12 @@ static int __trace_eprobe_create(int argc,
> > > const char *argv[])
> > >                  sanitize_event_name(buf1);
> > >                  event = buf1;
> > >          }
> > > -       if (!is_good_name(event) || !is_good_name(group))
> > > -               goto parse_error;
> > traceprobe_parse_event_name() is only called if (event).  In the
> > !event case, wouldn't the is_good_name() checks still be needed
> > (since
> > in that case buf1 is assigned to event)?
> 
> when user input no  event name, it will generate event name from
> second  
> SYSTEM.EVENT,
> 
> and it will validate with following traceprobe_parse_event_name().
> 
> 

Right, but that happens in your second patch '[PATCH v5 2/3] tracing:
auto generate event name when create a group of events', not this one.
 
So if you apply only this patch, the !event case will assign event but
it will remain unchecked when used later in this function.

It would make more sense to remove this check in patch 2/3 along with
the code that does the generating...

> (
> 
> if you agree, i will send a new version to update a minor issue in 
> second patch,
> 
> 
>          sys_event = argv[1];
> -       ret = traceprobe_parse_event_name(&sys_event, &sys_name,
> buf2,
> -                                         sys_event - argv[1]);
> -       if (ret || !sys_name)
> +       ret = traceprobe_parse_event_name(&sys_event, &sys_name,
> buf2, 0);
> +       if (!sys_event || !sys_name)
>                  goto parse_error;
> 
> )
> 
> > 
> > >   
> > >          sys_event = argv[1];
> > >          ret = traceprobe_parse_event_name(&sys_event, &sys_name,
> > > buf2,
> > >                                            sys_event - argv[1]);
> > >          if (ret || !sys_name)
> > >                  goto parse_error;
> > > -       if (!is_good_name(sys_event) || !is_good_name(sys_name))
> > > -               goto parse_error;
> > I agree this one isn't needed.

But keep this one in this patch, since it's useful on its own as a
standalone cleanup regardless of whether or not patch 2/3 gets merged.

Tom

> > Thanks,
> > 
> > Tom
> > 
> > >   
> > >          mutex_lock(&event_mutex);
> > >          event_call = find_and_get_event(sys_name, sys_event);