| From: Yafang Shao <laoar.shao@gmail.com> |
| Subject: mm/util: fix possible race condition in kstrdup() |
| Date: Mon, 7 Oct 2024 22:49:09 +0800 |
| |
| In kstrdup(), it is critical to ensure that the dest string is always |
| NUL-terminated. However, potential race condition 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 calculates 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 after the memcpy. It |
| is worth noting that memcpy() is not atomic, so the new string can be |
| shorter when memcpy() already copied past the new NUL. |
| |
| Link: https://lkml.kernel.org/r/20241007144911.27693-6-laoar.shao@gmail.com |
| Signed-off-by: Yafang Shao <laoar.shao@gmail.com> |
| Cc: Alejandro Colomar <alx@kernel.org> |
| Cc: Andy Shevchenko <andy.shevchenko@gmail.com> |
| Cc: Alexander Viro <viro@zeniv.linux.org.uk> |
| Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> |
| Cc: Catalin Marinas <catalin.marinas@arm.com> |
| Cc: Christian Brauner <brauner@kernel.org> |
| Cc: Daniel Vetter <daniel.vetter@ffwll.ch> |
| Cc: David Airlie <airlied@gmail.com> |
| Cc: Eric Biederman <ebiederm@xmission.com> |
| Cc: Eric Paris <eparis@redhat.com> |
| Cc: James Morris <jmorris@namei.org> |
| Cc: Jan Kara <jack@suse.cz> |
| Cc: Justin Stitt <justinstitt@google.com> |
| Cc: Kees Cook <keescook@chromium.org> |
| Cc: Linus Torvalds <torvalds@linux-foundation.org> |
| Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> |
| Cc: Matthew Wilcox <willy@infradead.org> |
| Cc: Matus Jokay <matus.jokay@stuba.sk> |
| Cc: Maxime Ripard <mripard@kernel.org> |
| Cc: Ondrej Mosnacek <omosnace@redhat.com> |
| Cc: Paul Moore <paul@paul-moore.com> |
| Cc: Quentin Monnet <qmo@kernel.org> |
| Cc: "Serge E. Hallyn" <serge@hallyn.com> |
| Cc: Simon Horman <horms@kernel.org> |
| Cc: Stephen Smalley <stephen.smalley.work@gmail.com> |
| Cc: Steven Rostedt (Google) <rostedt@goodmis.org> |
| Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> |
| Cc: Thomas Zimmermann <tzimmermann@suse.de> |
| Signed-off-by: Andrew Morton <akpm@linux-foundation.org> |
| --- |
| |
| mm/util.c | 9 ++++++++- |
| 1 file changed, 8 insertions(+), 1 deletion(-) |
| |
| --- a/mm/util.c~mm-util-fix-possible-race-condition-in-kstrdup |
| +++ a/mm/util.c |
| @@ -62,8 +62,15 @@ 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 NUL terminator. |
| + */ |
| + buf[len - 1] = '\0'; |
| + } |
| return buf; |
| } |
| EXPORT_SYMBOL(kstrdup); |
| _ |