| Message ID | 20240817025624.13157-6-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Improve the copy of task comm | expand |
Hi Yafang, On Sat, Aug 17, 2024 at 10:56:21AM GMT, 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. > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > --- > 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'; > + } I would compact the above to: len = strlen(s); buf = kmalloc_track_caller(len + 1, gfp); if (buf) strcpy(mempcpy(buf, s, len), ""); It allows _FORTIFY_SOURCE to track the copy of the NUL, and also uses less screen. It also has less moving parts. (You'd need to write a mempcpy() for the kernel, but that's as easy as the following:) #define mempcpy(d, s, n) (memcpy(d, s, n) + n) In shadow utils, I did a global replacement of all buf[...] = '\0'; by strcpy(..., "");. It ends up being optimized by the compiler to the same code (at least in the experiments I did). Have a lovely day! Alex > return buf; > } > EXPORT_SYMBOL(kstrdup); > -- > 2.43.5 >
On Sat, 17 Aug 2024 at 01:48, Alejandro Colomar <alx@kernel.org> wrote: > > I would compact the above to: > > len = strlen(s); > buf = kmalloc_track_caller(len + 1, gfp); > if (buf) > strcpy(mempcpy(buf, s, len), ""); No, we're not doing this kind of horror. If _FORTIFY_SOURCE has problems with a simple "memcpy and add NUL", then _FORTIFY_SOURCE needs to be fixed. We don't replace a "buf[len] = 0" with strcpy(,""). Yes, compilers may simplify it, but dammit, it's an unreadable incomprehensible mess to humans, and humans still matter a LOT more. Linus
Hi Linus, On Sat, Aug 17, 2024 at 09:26:21AM GMT, Linus Torvalds wrote: > On Sat, 17 Aug 2024 at 01:48, Alejandro Colomar <alx@kernel.org> wrote: > > > > I would compact the above to: > > > > len = strlen(s); > > buf = kmalloc_track_caller(len + 1, gfp); > > if (buf) > > strcpy(mempcpy(buf, s, len), ""); > > No, we're not doing this kind of horror. Ok. > If _FORTIFY_SOURCE has problems with a simple "memcpy and add NUL", > then _FORTIFY_SOURCE needs to be fixed. _FORTIFY_SOURCE works (AFAIK) by replacing the usual string calls by oneis that do some extra work to learn the real size of the buffers. This means that for _FORTIFY_SOURCE to work, you need to actually call a function. Since the "add NUL" is not done in a function call, it's unprotected (except that sanitizers may protect it via other means). Here's the fortified version of strcpy(3) in the kernel: $ grepc -h -B15 strcpy ./include/linux/fortify-string.h /** * strcpy - Copy a string into another string buffer * * @p: pointer to destination of copy * @q: pointer to NUL-terminated source string to copy * * Do not use this function. While FORTIFY_SOURCE tries to avoid * overflows, this is only possible when the sizes of @q and @p are * known to the compiler. Prefer strscpy(), though note its different * return values for detecting truncation. * * Returns @p. * */ /* Defined after fortified strlen to reuse it. */ __FORTIFY_INLINE __diagnose_as(__builtin_strcpy, 1, 2) char *strcpy(char * const POS p, const char * const POS q) { const size_t p_size = __member_size(p); const size_t q_size = __member_size(q); size_t size; /* If neither buffer size is known, immediately give up. */ if (__builtin_constant_p(p_size) && __builtin_constant_p(q_size) && p_size == SIZE_MAX && q_size == SIZE_MAX) return __underlying_strcpy(p, q); size = strlen(q) + 1; /* Compile-time check for const size overflow. */ if (__compiletime_lessthan(p_size, size)) __write_overflow(); /* Run-time check for dynamic size overflow. */ if (p_size < size) fortify_panic(FORTIFY_FUNC_strcpy, FORTIFY_WRITE, p_size, size, p); __underlying_memcpy(p, q, size); return p; } > We don't replace a "buf[len] = 0" with strcpy(,""). Yes, compilers may > simplify it, but dammit, it's an unreadable incomprehensible mess to > humans, and humans still matter a LOT more. I understand. While I don't consider it unreadable anymore (I guess I got used to it), it felt strange at first. > > Linus Have a lovely day! Alex
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);
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 | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)