Re: [PATCH v7 5/8] mm/util: Fix possible race condition in kstrdup()
From: Kees Cook <kees@kernel.org>
Date: 2024-09-28 21:14:27
Also in:
bpf, dri-devel, linux-fsdevel, linux-mm, linux-security-module, linux-trace-kernel, selinux
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
quoted hunk ↗ jump to hunk
Signed-off-by: Yafang Shao <redacted> Cc: Andrew Morton <akpm@linux-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