On Thu, Sep 26, 2024 at 7:44 PM Yafang Shao <laoar.shao@xxxxxxxxx> 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
