Re: [PATCH v3 bpf-next 4/4] selftest: bpf: Add test for hwtstamp proxy.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The function `check_tstamp()` unconditionally returns -1 even upon successful validation, which causes the test driver to silently skip the RX phase of the test.
- [Medium] An uninitialized stack buffer `buf_cmsg` is passed to the kernel via `sendmsg()`, passing stack padding into the kernel and risking Memory Sanitizer (MSAN) failures.
--

commit cbe313a245c9f97c0ad306889154ce4bfae090f0
Author: Kuniyuki Iwashima <kuniyu@google.com>

selftest: bpf: Add test for hwtstamp proxy.

This selftest simulates the hardware timestamp proxy scenario using two UDP
sockets. It receives and injects encapsulated packets via a GENEVE device to
verify hardware timestamp behavior.

> diff --git a/tools/testing/selftests/bpf/prog_tests/proxy_hwtstamp.c b/tools/testing/selftests/bpf/prog_tests/proxy_hwtstamp.c
> new file mode 100644
> index 0000000000000..175d8be74804d
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/proxy_hwtstamp.c

[ ... ]

> @@ -335,11 +335,11 @@ static int check_tstamp(struct proxy_hwtstamp_test_case *test_case, bool tx)
>  	ret = memcmp(buf_msg, test_case->app_payload, sizeof(test_case->app_payload));
>  	ASSERT_OK(ret, "memcmp");
>  
>  	ret = -1;
>  
>  	for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) {

[Severity: Medium]
Is ret intentionally left as -1 here? 

If it isn't updated to 0 upon success, won't this cause the function to always
return -1?

Looking at run_test(), the RX test phase is guarded by checking this return
value:

run_test() {
    ...
    ret = test_proxy_hwtstamp_tx(test_case);
    if (!ret)
        test_proxy_hwtstamp_rx(test_case);

Since test_proxy_hwtstamp_tx() returns the -1 from check_tstamp(), doesn't
this unconditionally skip the RX test phase without actually failing the test?

[ ... ]

> @@ -364,22 +364,22 @@ static int test_proxy_hwtstamp_tx(struct proxy_hwtstamp_test_case *test_case)
>  {
>  	char h_source_dummy[ETH_HLEN] = {0xFF, 0xEE, 0xDD, 0xCC, 0xBB, 0xAA};
>  	char buf_cmsg[CMSG_SPACE(sizeof(u32))];
>  	struct proxy_header *phdr;
>  	struct msghdr msg = {};
>  	struct iovec iov = {};
>  	struct cmsghdr *cmsg;
>  	int ret;
>  
>  	memset(test_case->app_payload, 0xAB, sizeof(test_case->app_payload));
>  	iov.iov_base = test_case->app_payload;
>  	iov.iov_len = sizeof(test_case->app_payload);
>  
>  	msg.msg_iov = &iov;
>  	msg.msg_iovlen = 1;
>  	msg.msg_control = buf_cmsg;
>  	msg.msg_controllen = sizeof(buf_cmsg);
>  
>  	cmsg = CMSG_FIRSTHDR(&msg);
>  	cmsg->cmsg_level = SOL_SOCKET;
>  	cmsg->cmsg_type = SCM_TS_OPT_ID;
>  	cmsg->cmsg_len = CMSG_LEN(sizeof(u32));
>  	*(u32 *)CMSG_DATA(cmsg) = TSKEY;
>  
>  	ret = sendmsg(test_case->app_fd, &msg, 0);

[Severity: Medium]
Can this pass uninitialized stack memory to the kernel?

The buf_cmsg array is allocated on the stack without being zero-initialized.
While the cmsghdr structure assignments initialize CMSG_LEN(sizeof(u32)) bytes,
does this leave the remaining alignment padding bytes uninitialized?

If so, passing it to sendmsg() could leak uninitialized stack padding into the
kernel and trigger Memory Sanitizer (MSAN) warnings in continuous integration
environments.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260613224823.2399535-1-kuniyu@google.com?part=4