Re: [PATCH v2 05/10] mm/util: Fix possible race condition in kstrdup()
From: Andrew Morton <akpm@linux-foundation.org>
Date: 2024-06-13 21:14:37
Also in:
bpf, dri-devel, linux-fsdevel, linux-mm, linux-security-module, linux-trace-kernel, selinux
On Thu, 13 Jun 2024 10:30:39 +0800 Yafang Shao [off-list ref] 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.The concept sounds a little strange. If some code takes a copy of a string while some other code is altering it, yes, the result will be a mess. This is why get_task_comm() exists, and why it uses locking. I get that "your copy is a mess" is less serious than "your string isn't null-terminated" but still. Whichever outcome we get, the calling code is buggy and should be fixed. Are there any other problematic scenarios we're defending against here?
quoted hunk ↗ jump to hunk
--- a/mm/util.c +++ b/mm/util.c@@ -60,8 +60,10 @@ 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); + buf[len - 1] = '\0'; + } return buf; }
Now I'll start receiving patches to remove this again. Let's have a code comment please. And kstrdup() is now looking awfully similar to kstrndup(). Perhaps there's a way to reduce duplication?