Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] lib/string_kunit: uninitialized stack read in string_copy_task_comm() --
commit 0d3a01595d2cc8a60f5d8a8716e5935db351758d Author: André Almeida <[email protected]> lib/string_kunit: Add test for copy_task_comm() This commit adds a KUnit test case for copy_task_comm() in lib/tests/string_kunit.c to verify copying a task command name into buffers of equal, smaller, and larger sizes. > diff --git a/lib/tests/string_kunit.c b/lib/tests/string_kunit.c > index 0819ace5b0275..66c3398f23174 100644 > --- a/lib/tests/string_kunit.c > +++ b/lib/tests/string_kunit.c > @@ -881,6 +881,43 @@ static void string_bench_strrchr(struct kunit *test) > STRING_BENCH_BUF(test, buf, len, strrchr, buf, '\0'); > } > > +#define TASK_NAME "task_name" > +#define TASK_NAME_LEN 9 > +#define TASK_MAX_LEN TASK_COMM_LEN > +#define SMALLER_LEN (TASK_NAME_LEN - 3) > +#define BIGGER_LEN (TASK_MAX_LEN + 3) > + > +static void string_copy_task_comm(struct kunit *test) > +{ > + char str[TASK_MAX_LEN] = TASK_NAME, copy[TASK_MAX_LEN], > + smaller_buf[SMALLER_LEN], bigger_buf[BIGGER_LEN]; > + static struct task_struct task, *tsk = &task; > + int len1, len2, i; > + > + /* set and get task name */ > + strscpy_pad(tsk->comm, str); > + copy_task_comm(copy, tsk, TASK_COMM_LEN); > + > + len1 = strlen(str); > + len2 = strlen(copy); > + > + KUNIT_ASSERT_EQ(test, len1, len2); > + KUNIT_ASSERT_EQ(test, len2, TASK_NAME_LEN); > + KUNIT_ASSERT_EQ(test, copy[len2], '\0'); > + KUNIT_ASSERT_TRUE(test, !strcmp(str, copy)); > + > + /* copy to a smaller dst buffer */ > + copy_task_comm(smaller_buf, tsk, sizeof(smaller_buf)); > + KUNIT_ASSERT_TRUE(test, !strncmp(str, smaller_buf, SMALLER_LEN - 1)); > + KUNIT_ASSERT_EQ(test, smaller_buf[SMALLER_LEN - 1], '\0'); > + > + /* copy to a bigger dst buffer */ > + copy_task_comm(bigger_buf, tsk, sizeof(bigger_buf)); > + KUNIT_ASSERT_TRUE(test, !strncmp(str, bigger_buf, TASK_NAME_LEN)); > + for (i = TASK_NAME_LEN; i < BIGGER_LEN; i++) > + KUNIT_ASSERT_EQ(test, bigger_buf[i], '\0'); > +} [Severity: Medium] Does this loop in string_copy_task_comm() read uninitialized stack memory at the end of bigger_buf[]? In string_copy_task_comm(), bigger_buf is declared on the stack without an initializer and sized to BIGGER_LEN (TASK_MAX_LEN + 3, where TASK_MAX_LEN is TASK_COMM_LEN). However, copy_task_comm() in include/linux/sched.h clamps the copy length to TASK_COMM_LEN and does not zero-pad dst beyond _len: include/linux/sched.h:copy_task_comm() { const char *_src = tsk->comm; size_t _len = min(len, TASK_COMM_LEN); if (!_len) return; memcpy(dst, _src, _len); dst[_len - 1] = '\0'; } Because copy_task_comm() only writes TASK_MAX_LEN bytes into bigger_buf, the trailing 3 bytes from TASK_MAX_LEN to BIGGER_LEN - 1 are never written. When the loop iterates i up to BIGGER_LEN, won't KUNIT_ASSERT_EQ() read those 3 uninitialized stack bytes and fail the test (or trigger a KMSAN report) when stack variables are not zero-initialized? Note that in the later commit de6fdb508944 ("sched: Extend task command name with TASK_COMM_EXT_LEN"), tsk->comm and the clamp in copy_task_comm() are increased to TASK_COMM_EXT_LEN (64) while TASK_MAX_LEN here remains TASK_COMM_LEN (16). That masks the uninitialized read only because bigger_buf (19 bytes) ceases to be larger than tsk->comm (64 bytes), which leaves this commit broken during bisection and means the bigger_buf test case is no longer testing a buffer larger than tsk->comm after that commit. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
