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

Commit Message

Yafang Shao Aug. 17, 2024, 2:56 a.m. UTC

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 <[e-mail]>
Cc: Andrew Morton <[e-mail]>
---
 mm/util.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Alejandro Colomar Aug. 17, 2024, 8:48 a.m. UTC | #1

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 <[e-mail]>
> Cc: Andrew Morton <[e-mail]>
> ---
>  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
>

Linus Torvalds Aug. 17, 2024, 4:26 p.m. UTC | #2

On Sat, 17 Aug 2024 at 01:48, Alejandro Colomar <[e-mail]> 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

Alejandro Colomar Aug. 17, 2024, 5:03 p.m. UTC | #3

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 <[e-mail]> 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

Andy Shevchenko Sept. 26, 2024, 5:35 p.m. UTC | #4

On Thu, Sep 26, 2024 at 7:44 PM Yafang Shao <[e-mail]> 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.
 */

> +                */

Patch

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);