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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Thu, Sep 26, 2024 at 7:44 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> In kstrdup(), it is critical to ensure that the dest string is always
> NUL-terminated. However, potential race condidtion can occur between a

condition

> 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

calculates

> 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.

memcpy() is not atomic AFAIK, meaning that the new string can be also
shorter and when memcpy() already copied past the new NUL. I would
amend the explanation to include this as well.

...

> +               /* 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.

/*
 * The wrong comment style. Besides that a typo
 * in the word 'terminator'. Please, run codespell on your changes.
 * Also use the same form: NUL-terminator when you are talking
 * about '\0' and not NULL.
 */

> +                */


-- 
With Best Regards,
Andy Shevchenko