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

On Thu, Sep 26, 2024 at 7:44 PM 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

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