Re: [PATCH] exec: Make sure task->comm is always NUL-terminated

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 1706 bytes --]

Edited down to just the end result:

On Fri, 29 Nov 2024 at 20:49, Kees Cook <kees@kernel.org> wrote:
>
>  void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
>  {
>         size_t len = min(strlen(buf), sizeof(tsk->comm) - 1);
>
>         trace_task_rename(tsk, buf);
>         memcpy(tsk->comm, buf, len);
>         memset(&tsk->comm[len], 0, sizeof(tsk->comm) - len);
>         perf_event_comm(tsk, exec);
>  }

I actually don't think that's super-safe either. Yeah, it works in
practice, and the last byte is certainly always going to be 0, but it
might not be reliably padded.

Why? It walks over the source twice. First at strlen() time, then at
memcpy. So if the source isn't stable, the end result might have odd
results with NUL characters in the middle.

And strscpy() really was *supposed* to be safe even in this case, and
I thought it was until I looked closer.

But I think strscpy() can be saved.

Something (UNTESTED!) like the attached I think does the right thing.
I added a couple of "READ_ONCE()" things to make it really super-clear
that strscpy() reads the source exactly once, and to not allow any
compiler re-materialization of the reads (although I think that when I
asked people, it turns out neither gcc nor clang rematerialize memory
accesses, so that READ_ONCE is likely more a documentation ad
theoretical thing than a real thing).

And yes, we could make the word-at-a-time case also know about masking
the last word, but it's kind of annoying and depends on byte ordering.

Hmm? I don't think your version is wrong, but I also think we'd be
better off making our 'strscpy()' infrastructure explicitly safe wrt
unstable source strings.

          Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1168 bytes --]

 lib/string.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/lib/string.c b/lib/string.c
index 76327b51e36f..a2a678e45389 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -137,7 +137,7 @@ ssize_t sized_strscpy(char *dest, const char *src, size_t count)
 	if (IS_ENABLED(CONFIG_KMSAN))
 		max = 0;
 
-	while (max >= sizeof(unsigned long)) {
+	while (max > sizeof(unsigned long)) {
 		unsigned long c, data;
 
 		c = read_word_at_a_time(src+res);
@@ -153,10 +153,10 @@ ssize_t sized_strscpy(char *dest, const char *src, size_t count)
 		max -= sizeof(unsigned long);
 	}
 
-	while (count) {
+	while (count > 0) {
 		char c;
 
-		c = src[res];
+		c = READ_ONCE(src[res]);
 		dest[res] = c;
 		if (!c)
 			return res;
@@ -164,11 +164,11 @@ ssize_t sized_strscpy(char *dest, const char *src, size_t count)
 		count--;
 	}
 
-	/* Hit buffer length without finding a NUL; force NUL-termination. */
-	if (res)
-		dest[res-1] = '\0';
+	/* Final byte - force NUL termination */
+	dest[res] = 0;
 
-	return -E2BIG;
+	/* Return -E2BIG if the source continued.. */
+	return READ_ONCE(src[res]) ? -E2BIG : res;
 }
 EXPORT_SYMBOL(sized_strscpy);