Re: [PATCH bpf-next v5 2/2] selftests/bpf: Add memcg async reclaim test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Insecure temporary file creation (Time-of-Check to Time-of-Use symlink attack)

Pre-existing issues:
- [High] Missing negative bounds check for enum in BPF memory controller kfuncs
--

commit d790568f8e67742a221eaf1bb57e732d1d035f32
Author: Hui Zhu <zhuhui@kylinos.cn>
selftests/bpf: Add memcg async reclaim test

This commit adds the memcg_async_reclaim selftest to verify that
BPF-driven async proactive reclaim mitigates refault-induced slowdown
under memory pressure.

[Severity: High]
This is a pre-existing issue, but there is a missing negative bounds check
for enum arguments in the BPF memory controller kfuncs.

In mm/bpf_memcontrol.c:bpf_mem_cgroup_memory_events():

    if (unlikely(event >= MEMCG_NR_MEMORY_EVENTS))
        return (unsigned long)-1;

And similarly in mm/memcontrol.c:memcg_vm_event_item_valid():

    if (idx >= NR_VM_EVENT_ITEMS)
        return false;

Because enum types in C are signed integers, these signed comparisons allow
negative values to bypass the bounds check. A BPF program calling these
kfuncs with a negative integer value can bypass the check, leading directly
to an out-of-bounds kernel memory read. Since the BPF verifier permits this
(the enum parameter is not constrained by __const), could this allow
information disclosure for users with CAP_BPF?

> diff --git a/tools/testing/selftests/bpf/prog_tests/memcg_async_reclaim.c b/tools/testing/selftests/bpf/prog_tests/memcg_async_reclaim.c
> new file mode 100644
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/memcg_async_reclaim.c
[ ... ]
> +static int write_file(const char *filename)
> +{
> +	int ret = -1;
> +	size_t written = 0;
> +	char *buffer;
> +	FILE *fp;
> +
> +	fp = fopen(filename, "wb");
> +	if (!fp)
> +		goto out;
[ ... ]
> +static int
> +run_high_low_workload(double *high_elapsed, double *low_elapsed, int read_times)
> +{
> +	char high_data_file[PATH_MAX];
> +	char low_data_file[PATH_MAX];
> +	char high_time_file[PATH_MAX];
> +	char low_time_file[PATH_MAX];
> +	const char *dir = workload_files_dir();
> +	pid_t high_pid = -1, low_pid = -1;
> +	pid_t wait_ret;
> +	int fd, status;
> +	int ret = -1;
> +
> +	snprintf(high_data_file, sizeof(high_data_file),
> +		 "%s/memcg_async_high_data_XXXXXX", dir);
> +	snprintf(low_data_file, sizeof(low_data_file),
> +		 "%s/memcg_async_low_data_XXXXXX", dir);
> +	snprintf(high_time_file, sizeof(high_time_file),
> +		 "%s/memcg_async_high_time_XXXXXX", dir);
> +	snprintf(low_time_file, sizeof(low_time_file),
> +		 "%s/memcg_async_low_time_XXXXXX", dir);
> +
> +	fd = mkstemp(high_data_file);
> +	if (!ASSERT_GE(fd, 0, "mkstemp"))
> +		goto cleanup;
> +	close(fd);

[Severity: Medium]
Is there a potential Time-of-Check to Time-of-Use (TOCTOU) symlink
vulnerability here? 

The test uses mkstemp() to securely create temporary files, but immediately
closes the file descriptor. Later, the child process opens the file by name
in write_file() using fopen(filename, "wb").

If the test is run in a shared temporary directory (like /tmp), could a local
attacker replace the temporary file with a symlink to another file on the
system during the window between close() and fopen()? Since the test process
typically runs as root, this would follow the symlink and overwrite the
target file.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1787826402.git.zhuhui@kylinos.cn?part=2