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: <20230108162222.146d136f@rorschach.local.home>
Date:   Sun, 8 Jan 2023 16:22:22 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Quanfa Fu <quanfafu@...il.com>
Cc:     rostedt@...dmis.or, mhiramat@...nel.org,
        linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH] tracing/eprobe: Replace kzalloc with kmalloc

On Sat,  7 Jan 2023 11:43:35 +0800
Quanfa Fu <quanfafu@...il.com> wrote:

> Since this memory will be filled soon below, I feel that there is

kzalloc() is also used as a safety measure to make sure nothing is
accidentally exposed. I rather keep it for safety. Just because it
doesn't need to be here doesn't mean it should be removed. There is no
benefit to making this kmalloc(), as this is far from a fast path.

> no need for a memory of all zeros here. 'snprintf' does not return
> negative num according to ISO C99, so I feel that no judgment is
> needed here.

Also, it's best to remove "feelings" from change logs. Code updates are
not made due to how one feels about something (at least it shouldn't
be), but about having technical reasons for doing so. I do agree
there's no reason to check snprintf() from returning negative, as
looking at its implementation, there is no negative return. Thus, the
change log should be:

 "No need to check for negative return value from snprintf() as the
 code does not return negative values."

> 
> No functional change intended.

And this does have functional changes. If the output of a compiler is
different for a function, then that's a functional change. What we
consider non functional changes is if functions get moved around, or
possibly code in a function is moved into a helper function where the
compiler *should* end up with the same assembly.

Changing what malloc is called is definitely a functional change.

> 
> Signed-off-by: Quanfa Fu <quanfafu@...il.com>
> ---
>  kernel/trace/trace_eprobe.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> index 352b65e2b910..cd1d271a74e7 100644
> --- a/kernel/trace/trace_eprobe.c
> +++ b/kernel/trace/trace_eprobe.c
> @@ -917,15 +917,13 @@ static int trace_eprobe_parse_filter(struct trace_eprobe *ep, int argc, const ch
>  	for (i = 0; i < argc; i++)
>  		len += strlen(argv[i]) + 1;
>  
> -	ep->filter_str = kzalloc(len, GFP_KERNEL);
> +	ep->filter_str = kmalloc(len, GFP_KERNEL);
>  	if (!ep->filter_str)
>  		return -ENOMEM;
>  
>  	p = ep->filter_str;
>  	for (i = 0; i < argc; i++) {
>  		ret = snprintf(p, len, "%s ", argv[i]);

I wonder if this should be a strncat() instead?

> -		if (ret < 0)
> -			goto error;
>  		if (ret > len) {
>  			ret = -E2BIG;
>  			goto error;

	for (i = 0; i < arcc, i++)
		strncat(ep->filter_str, argv[i], len);

I mean, before this code we have that loop already determining what len
is, do we really need to check if it is going to be -E2BIG?

-- Steve

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ