netdev - Re: [PATCH v7 5/8] mm/util: Fix possible race condition in kstrdup()

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: <202409281411.3C42A3703C@keescook>
Date: Sat, 28 Sep 2024 14:14:27 -0700
From: Kees Cook <kees@...nel.org>
To: Yafang Shao <laoar.shao@...il.com>
Cc: akpm@...ux-foundation.org, torvalds@...ux-foundation.org,
	alx@...nel.org, justinstitt@...gle.com, ebiederm@...ssion.com,
	alexei.starovoitov@...il.com, rostedt@...dmis.org,
	catalin.marinas@....com, penguin-kernel@...ove.sakura.ne.jp,
	linux-mm@...ck.org, linux-fsdevel@...r.kernel.org,
	linux-trace-kernel@...r.kernel.org, audit@...r.kernel.org,
	linux-security-module@...r.kernel.org, selinux@...r.kernel.org,
	bpf@...r.kernel.org, netdev@...r.kernel.org,
	dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v7 5/8] mm/util: Fix possible race condition in kstrdup()

On Sat, Aug 17, 2024 at 10:56:21AM +0800, Yafang Shao wrote:
> In kstrdup(), it is critical to ensure that the dest string is always
> NUL-terminated. However, potential race condidtion can occur between a
> writer and a reader.
> 
> Consider the following scenario involving task->comm:
> 
>     reader                    writer
> 
>   len = strlen(s) + 1;
>                              strlcpy(tsk->comm, buf, sizeof(tsk->comm));
>   memcpy(buf, s, len);
> 
> In this case, there is a race condition between the reader and the
> writer. The reader calculate the length of the string `s` based on the
> old value of task->comm. However, during the memcpy(), the string `s`
> might be updated by the writer to a new value of task->comm.
> 
> If the new task->comm is larger than the old one, the `buf` might not be
> NUL-terminated. This can lead to undefined behavior and potential
> security vulnerabilities.
> 
> Let's fix it by explicitly adding a NUL-terminator.

So, I'm fine with adding this generally, but I'm not sure we can
construct these helpers to be universally safe against the strings
changing out from under them. This situation is distinct to the 'comm'
member, so I'd like to focus on helpers around 'comm' access behaving in
a reliable fashion.

-Kees

> 
> Signed-off-by: Yafang Shao <laoar.shao@...il.com>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> ---
>  mm/util.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/util.c b/mm/util.c
> index 983baf2bd675..4542d8a800d9 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -62,8 +62,14 @@ char *kstrdup(const char *s, gfp_t gfp)
>  
>  	len = strlen(s) + 1;
>  	buf = kmalloc_track_caller(len, gfp);
> -	if (buf)
> +	if (buf) {
>  		memcpy(buf, s, len);
> +		/* During memcpy(), the string might be updated to a new value,
> +		 * which could be longer than the string when strlen() is
> +		 * called. Therefore, we need to add a null termimator.
> +		 */
> +		buf[len - 1] = '\0';
> +	}
>  	return buf;
>  }
>  EXPORT_SYMBOL(kstrdup);
> -- 
> 2.43.5
> 

-- 
Kees Cook