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 <[email protected]> > Cc: Andrew Morton <[email protected]> > --- > 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
>
--
<https://www.alejandro-colomar.es/>
signature.asc
Description: PGP signature
