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  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]
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,
> 
> 

Powered by blists - more mailing lists