linux-kernel - Re: [PATCH v5 2/5] tracing: Rework synthetic event command parsing

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: <48d21108d635e707ef135cac7fa0eaabc1cf5e9a.camel@kernel.org>
Date:   Wed, 23 Dec 2020 17:17:27 -0600
From:   Tom Zanussi <zanussi@...nel.org>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     rostedt@...dmis.org, axelrasmussen@...gle.com,
        dan.carpenter@...cle.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/5] tracing: Rework synthetic event command parsing

Hi Masami,

On Tue, 2020-12-22 at 21:42 +0900, Masami Hiramatsu wrote:
> Hi Tom,
> 
> On Mon, 21 Dec 2020 15:44:28 -0600
> Tom Zanussi <zanussi@...nel.org> wrote:
> 
> 
> > @@ -656,7 +651,6 @@ static struct synth_field
> > *parse_synth_field(int argc, const char **argv,
> >  
> >  	size = synth_field_size(field->type);
> >  	if (size < 0) {
> > -		synth_err(SYNTH_ERR_INVALID_TYPE, errpos(field_type));
> 
> Why did you remove this error message?

It wasn't actually removed - it was just moved into the next patch, so
is still there.  I'll move it back here to avoid confusion.

> 
> [..]
> > @@ -1228,26 +1189,47 @@ static int __create_synth_event(int argc,
> > const char *name, const char **argv)
> >  		goto out;
> >  	}
> >  
> > -	for (i = 0; i < argc - 1; i++) {
> > -		if (strcmp(argv[i], ";") == 0)
> > -			continue;
> > +	tmp_fields = saved_fields = kstrdup(raw_fields, GFP_KERNEL);
> > +	if (!tmp_fields) {
> > +		ret = -ENOMEM;
> > +		goto out;
> > +	}
> > +
> > +	while ((field_str = strsep(&tmp_fields, ";")) != NULL) {
> >  		if (n_fields == SYNTH_FIELDS_MAX) {
> >  			synth_err(SYNTH_ERR_TOO_MANY_FIELDS, 0);
> >  			ret = -EINVAL;
> >  			goto err;
> >  		}
> >  
> > -		field = parse_synth_field(argc - i, &argv[i],
> > &consumed);
> > +		argv = argv_split(GFP_KERNEL, field_str, &argc);
> > +		if (!argv) {
> > +			ret = -ENOMEM;
> > +			goto err;
> > +		}
> > +
> > +		if (!argc)
> > +			continue;
> > +
> > +		field = parse_synth_field(argc, argv, &consumed);
> >  		if (IS_ERR(field)) {
> > +			argv_free(argv);
> >  			ret = PTR_ERR(field);
> >  			goto err;
> >  		}
> > +
> > +		argv_free(argv);
> > +
> > +		if (consumed < argc) {
> > +			ret = -EINVAL;
> > +			goto err;
> > +		}
> 
> You can check the consumed < argc in parse_synth_field(), unless
> you keep the backward compatibility - I think you can add an
> inner loop for it, something like
> 
> while ((field_str = strsep(&tmp_fields, ";")) != NULL) {
>     argv = argv_split(...);
>     consumed = 0;
>     while (argc > consumed) {
>         // increment consumed in parse_synth_field()
>         field = parse_synth_field(argc - consumed, argv + consumed,
> &consumed);
>         if (IS_ERR(field)) {...}
> 
>         fields[n_fields++] = field;
>         if (n_fields == SYNTH_FIELDS_MAX) {...}
>      }
> 
>     argv_free(argv);
> }
> 
> what would you think?

Hmm, not sure this helps - there's only supposed to be one field per
field_str and consumed returns either 2 or 3 depending on the field. 
consumed is only used to detect whether there were unused words and if
so flag an error, rather than loop around to try to get another field.

> 
> > +
> >  		fields[n_fields++] = field;
> > -		i += consumed - 1;
> >  	}
> >  
> > -	if (i < argc && strcmp(argv[i], ";") != 0) {
> > -		synth_err(SYNTH_ERR_INVALID_FIELD, errpos(argv[i]));
> > +	if (n_fields == 0) {
> > +		synth_err(SYNTH_ERR_CMD_INCOMPLETE, 0);
> >  		ret = -EINVAL;
> >  		goto err;
> >  	}
> > @@ -1266,6 +1248,8 @@ static int __create_synth_event(int argc,
> > const char *name, const char **argv)
> >   out:
> >  	mutex_unlock(&event_mutex);
> >  
> > +	kfree(saved_fields);
> > +
> >  	return ret;
> >   err:
> >  	for (i = 0; i < n_fields; i++)
> > @@ -1385,29 +1369,35 @@ EXPORT_SYMBOL_GPL(synth_event_delete);
> >  
> >  static int create_or_delete_synth_event(const char *raw_command)
> >  {
> > -	char **argv, *name = NULL;
> > -	int argc = 0, ret = 0;
> > +	char *name = NULL, *fields, *p;
> > +	int ret = 0;
> >  
> > -	argv = argv_split(GFP_KERNEL, raw_command, &argc);
> > -	if (!argv)
> > -		return -ENOMEM;
> > +	raw_command = skip_spaces(raw_command);
> > +	if (raw_command[0] == '\0')
> > +		return ret;
> >  
> > -	if (!argc)
> > -		goto free;
> > +	last_cmd_set(raw_command);
> >  
> > -	name = argv[0];
> > +	p = strpbrk(raw_command, " \t");
> > +	if (!p)
> > +		return -EINVAL;
> 
> Hmm, this may drop the ability to delete an event with "!name",
> it always requires some spaces after the name.
> 

Yes, good point, will fix that and also add a test case for just !name.

Thanks,

Tom

> Thank you,
> 
>