| Message ID | 20240613023044.45873-6-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | Improve the copy of task comm | expand |
On Thu, 13 Jun 2024 10:30:39 +0800 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 > 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. The concept sounds a little strange. If some code takes a copy of a string while some other code is altering it, yes, the result will be a mess. This is why get_task_comm() exists, and why it uses locking. I get that "your copy is a mess" is less serious than "your string isn't null-terminated" but still. Whichever outcome we get, the calling code is buggy and should be fixed. Are there any other problematic scenarios we're defending against here? > > --- a/mm/util.c > +++ b/mm/util.c > @@ -60,8 +60,10 @@ 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); > + buf[len - 1] = '\0'; > + } > return buf; > } Now I'll start receiving patches to remove this again. Let's have a code comment please. And kstrdup() is now looking awfully similar to kstrndup(). Perhaps there's a way to reduce duplication?
On Thu, 13 Jun 2024 at 14:14, Andrew Morton <akpm@linux-foundation.org> wrote: > > The concept sounds a little strange. If some code takes a copy of a > string while some other code is altering it, yes, the result will be a > mess. This is why get_task_comm() exists, and why it uses locking. The thing is, get_task_comm() is terminally broken. Nobody sane uses it, and sometimes it's literally _because_ it uses locking. Let's look at the numbers: - 39 uses of get_task_comm() - 2 uses of __get_task_comm() because the locking doesn't work - 447 uses of raw "current->comm" - 112 uses of raw 'ta*sk->comm' (and possibly IOW, we need to just accept the fact that nobody actually wants to use "get_task_comm()". It's a broken interface. It's inconvenient, and the locking makes it worse. Now, I'm not convinced that kstrdup() is what anybody should use should, but of the 600 "raw" uses of ->comm, four of them do seem to be kstrdup. Not great, I think they could be removed, but they are examples of people doing this. And I think it *would* be good to have the guarantee that yes, the kstrdup() result is always a proper string, even if it's used for unstable sources. Who knows what other unstable sources exist? I do suspect that most of the raw uses of 'xyz->comm' is for printouts. And I think we would be better with a '%pTSK' vsnprintf() format thing for that. Sadly, I don't think coccinelle can do the kinds of transforms that involve printf format strings. And no, a printk() string still couldn't use the locking version. Linus
On Fri, Jun 14, 2024 at 5:14 AM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Thu, 13 Jun 2024 10:30:39 +0800 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 > > 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. > > The concept sounds a little strange. If some code takes a copy of a > string while some other code is altering it, yes, the result will be a > mess. This is why get_task_comm() exists, and why it uses locking. > > I get that "your copy is a mess" is less serious than "your string > isn't null-terminated" but still. Whichever outcome we get, the > calling code is buggy and should be fixed. > > Are there any other problematic scenarios we're defending against here? > > > > > --- a/mm/util.c > > +++ b/mm/util.c > > @@ -60,8 +60,10 @@ 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); > > + buf[len - 1] = '\0'; > > + } > > return buf; > > } > > Now I'll start receiving patches to remove this again. Let's have a > code comment please. I will add a comment for it. > > And kstrdup() is now looking awfully similar to kstrndup(). Perhaps > there's a way to reduce duplication? Yes, I believe we can add a common helper for them : static char *__kstrndup(const char *s, size_t max, gfp_t gfp)
On Fri, Jun 14, 2024 at 6:18 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Thu, 13 Jun 2024 at 14:14, Andrew Morton <akpm@linux-foundation.org> wrote: > > > > The concept sounds a little strange. If some code takes a copy of a > > string while some other code is altering it, yes, the result will be a > > mess. This is why get_task_comm() exists, and why it uses locking. > > The thing is, get_task_comm() is terminally broken. > > Nobody sane uses it, and sometimes it's literally _because_ it uses locking. > > Let's look at the numbers: > > - 39 uses of get_task_comm() > > - 2 uses of __get_task_comm() because the locking doesn't work > > - 447 uses of raw "current->comm" > > - 112 uses of raw 'ta*sk->comm' (and possibly > > IOW, we need to just accept the fact that nobody actually wants to use > "get_task_comm()". It's a broken interface. It's inconvenient, and the > locking makes it worse. > > Now, I'm not convinced that kstrdup() is what anybody should use > should, but of the 600 "raw" uses of ->comm, four of them do seem to > be kstrdup. > > Not great, I think they could be removed, but they are examples of > people doing this. And I think it *would* be good to have the > guarantee that yes, the kstrdup() result is always a proper string, > even if it's used for unstable sources. Who knows what other unstable > sources exist? > > I do suspect that most of the raw uses of 'xyz->comm' is for > printouts. And I think we would be better with a '%pTSK' vsnprintf() > format thing for that. I will implement this change in the next step if no one else handles it. > > Sadly, I don't think coccinelle can do the kinds of transforms that > involve printf format strings. Yes, we need to carefully check them one by one. > > And no, a printk() string still couldn't use the locking version. > > Linus
diff --git a/mm/util.c b/mm/util.c index c9e519e6811f..3b383f790208 100644 --- a/mm/util.c +++ b/mm/util.c @@ -60,8 +60,10 @@ 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); + buf[len - 1] = '\0'; + } return buf; } EXPORT_SYMBOL(kstrdup);
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. Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Cc: Andrew Morton <akpm@linux-foundation.org> --- mm/util.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)