linux-kernel - Re: [PATCH v3 4/4] tracing/probes: Fix to record 0-length data_loc in fetch_store

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: <20230710181601.438b79a8@gandalf.local.home>
Date:   Mon, 10 Jul 2023 18:16:01 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     "Masami Hiramatsu (Google)" <mhiramat@...nel.org>
Cc:     Dan Carpenter <dan.carpenter@...aro.org>,
        linux-trace-kernel@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 4/4] tracing/probes: Fix to record 0-length data_loc
 in fetch_store_string*() if fails

On Sat,  8 Jul 2023 11:48:58 +0900
"Masami Hiramatsu (Google)" <mhiramat@...nel.org> wrote:

> --- a/kernel/trace/trace_probe_kernel.h
> +++ b/kernel/trace/trace_probe_kernel.h
> @@ -55,8 +55,7 @@ fetch_store_string_user(unsigned long addr, void *dest, void *base)
>  	__dest = get_loc_data(dest, base);
>  
>  	ret = strncpy_from_user_nofault(__dest, uaddr, maxlen);
> -	if (ret >= 0)
> -		*(u32 *)dest = make_data_loc(ret, __dest - base);
> +	*(u32 *)dest = make_data_loc((ret >= 0) ? ret : 0, __dest - base);
>  
>  	return ret;
>  }
> @@ -87,8 +86,7 @@ fetch_store_string(unsigned long addr, void *dest, void *base)
>  	 * probing.
>  	 */
>  	ret = strncpy_from_kernel_nofault(__dest, (void *)addr, maxlen);
> -	if (ret >= 0)
> -		*(u32 *)dest = make_data_loc(ret, __dest - base);
> +	*(u32 *)dest = make_data_loc((ret >= 0) ? ret : 0, __dest - base);

The above is a complex line, and not something that I think should be cut
and pasted between two different locations.

I know you took out the set_data_loc() helper, but really it should have
stayed, and have used that to update this code in the two places it
affected, instead of making the changes in those two locations.

That is, patch 3 could have had kept.

static nokprobe_inline void set_data_loc(int ret, void *dest, void *__dest, void *base)
{
	if (ret >= 0)
		*(u32 *)dest = make_data_loc(ret, __dest - base);
}

And this patch could have been:

static nokprobe_inline void set_data_loc(int ret, void *dest, void *__dest, void *base)
{
	*(u32 *)dest = make_data_loc(ret, __dest - base);
}

That would keep the complexity down in this changes set.

-- Steve


>  
>  	return ret;
>  }