Re: [PATCH v8 3/6] lib/string_kunit: Add test for copy_task_comm()

[sashiko-bot@kernel.org]

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 <andrealmeid@igalia.com>

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/20261030-tonyk-long_name-v8-0-c9b030332254@igalia.com?part=3