linux-kernel - Re: [PATCH 16/20] tracing/probe: Manual replacement of the deprecated strlcpy() with return values

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: <20210222124936.03103585@gandalf.local.home>
Date:   Mon, 22 Feb 2021 12:49:36 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Romain Perier <romain.perier@...il.com>
Cc:     Kees Cook <keescook@...omium.org>,
        kernel-hardening@...ts.openwall.com,
        Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 16/20] tracing/probe: Manual replacement of the
 deprecated strlcpy() with return values

On Mon, 22 Feb 2021 16:12:27 +0100
Romain Perier <romain.perier@...il.com> wrote:

> The strlcpy() reads the entire source buffer first, it is dangerous if
> the source buffer lenght is unbounded or possibility non NULL-terminated.
> It can lead to linear read overflows, crashes, etc...
> 
> As recommended in the deprecated interfaces [1], it should be replaced
> by strscpy.
> 
> This commit replaces all calls to strlcpy that handle the return values
> by the corresponding strscpy calls with new handling of the return
> values (as it is quite different between the two functions).
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> 
> Signed-off-by: Romain Perier <romain.perier@...il.com>
> ---
>  kernel/trace/trace_uprobe.c |   11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 3cf7128e1ad3..f9583afdb735 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -154,12 +154,11 @@ fetch_store_string(unsigned long addr, void *dest, void *base)
>  	u8 *dst = get_loc_data(dest, base);
>  	void __user *src = (void __force __user *) addr;
>  
> -	if (unlikely(!maxlen))
> -		return -ENOMEM;

Don't remove the above. You just broke the else side.

> -
> -	if (addr == FETCH_TOKEN_COMM)
> -		ret = strlcpy(dst, current->comm, maxlen);
> -	else
> +	if (addr == FETCH_TOKEN_COMM) {
> +		ret = strscpy(dst, current->comm, maxlen);
> +		if (ret == -E2BIG)
> +			return -ENOMEM;

I'm not sure the above is what we want. current->comm is always nul
terminated, and not only that, it will never be bigger than TASK_COMM_LEN.
If the "dst" location is smaller than comm (maxlen < TASK_COMM_LEN), it is
still OK to copy a partial string. It should not return -ENOMEM which looks
to be what happens with this patch.

In other words, it looks like this patch breaks the current code in more
ways than one.

-- Steve


> +	} else
>  		ret = strncpy_from_user(dst, src, maxlen);
>  	if (ret >= 0) {
>  		if (ret == maxlen)