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@xxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> ---
> 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
