Re: [PATCH 20/21] selftests/exec: test binfmt_misc loader substitution

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing NULL check for the map pointer returned by `bpf_object__find_map_by_name` before passing it to `bpf_map__attach_struct_ops`.
- [Medium] An unhandled `read()` error on `/proc/<pid>/auxv` leads to a massive out-of-bounds array read due to implicit type conversion on 32-bit architectures.
--

--- Patch [20]: [PATCH 20/21] selftests/exec: test binfmt_misc loader substitution ---
commit 7fb50e082768e7f213f616cf84985ff4feb60ccd
Author: Christian Brauner <brauner@kernel.org>
Subject: selftests/exec: test binfmt_misc loader substitution

This commit exercises the 'L' flag end to end by running a payload as the main
image with a copy of the system loader substituted for its PT_INTERP. It also
introduces a bpf-side BPF_BINPRM_LOADER test path.

> diff --git a/tools/testing/selftests/exec/binfmt_misc_bpf.c b/tools/testing/selftests/exec/binfmt_misc_bpf.c
> index 483c46e134107..926db50d39d54 100644
> --- a/tools/testing/selftests/exec/binfmt_misc_bpf.c
> +++ b/tools/testing/selftests/exec/binfmt_misc_bpf.c

[ ... ]

> +static int bpf_case_start(struct bpf_case *c, const char *objfile,
> +			  const char *handler, const char *entry)
>  {
> -	struct bpf_object *obj;
> -	struct bpf_map *map;
> -	struct bpf_link *link;
> -	int ret = -1;
> +	c->obj = NULL;
> +	c->link = NULL;
> +	c->entry = entry;
>  
> -	obj = bpf_object__open_file(objfile, NULL);
> -	if (!obj || libbpf_get_error(obj)) {
> +	c->obj = bpf_object__open_file(objfile, NULL);
> +	if (!c->obj || libbpf_get_error(c->obj)) {
>  		fprintf(stderr, "open %s failed\n", objfile);
> +		c->obj = NULL;
>  		return -1;
>  	}
> -	if (bpf_object__load(obj)) {
> +	if (bpf_object__load(c->obj)) {
>  		fprintf(stderr, "load %s failed (check dmesg for the verifier log)\n",
>  			objfile);
> -		goto close;
> -	}
> -	map = bpf_object__find_map_by_name(obj, handler);
> -	if (!map) {
> -		fprintf(stderr, "no struct_ops map '%s' in %s\n", handler, objfile);
> -		goto close;
> +		goto fail;
>  	}
> -	link = bpf_map__attach_struct_ops(map);
> -	if (!link || libbpf_get_error(link)) {
> +	c->link = bpf_map__attach_struct_ops(
> +			bpf_object__find_map_by_name(c->obj, handler));
> +	if (!c->link || libbpf_get_error(c->link)) {

[Severity: Medium]
Is it possible for bpf_object__find_map_by_name() to return NULL here if the 
map isn't found?

If it does return NULL, passing it directly to bpf_map__attach_struct_ops()
will cause a segmentation fault because the attach API unconditionally 
dereferences the map pointer to check its type. The original code included a
check for a NULL map before attempting to attach.

>  		fprintf(stderr, "attach struct_ops '%s' failed\n", handler);
> -		goto close;
> +		c->link = NULL;
> +		goto fail;
>  	}

[ ... ]

> diff --git a/tools/testing/selftests/exec/binfmt_misc_loader.c b/tools/testing/selftests/exec/binfmt_misc_loader.c
> new file mode 100644
> index 0000000000000..97fa0247ab53e
> --- /dev/null
> +++ b/tools/testing/selftests/exec/binfmt_misc_loader.c

[ ... ]

> +static int ptrace_probe(const char *target)
> +{
> +	unsigned long auxv[2 * 64], base = 0, entry = 0, at_flags = 0;
> +	unsigned long start_code = 0, end_code = 0;
> +	int status, fd, execfd_seen = 0, failed = 0;
> +	char path[64], buf[PATH_MAX];
> +	ssize_t n;
> +	pid_t pid;
> +	int i;

[ ... ]

> +	snprintf(path, sizeof(path), "/proc/%d/auxv", pid);
> +	fd = open(path, O_RDONLY);
> +	if (fd < 0) {
> +		failed = 1;
> +		n = 0;
> +	} else {
> +		n = read(fd, auxv, sizeof(auxv));
> +		close(fd);
> +	}
> +	for (i = 0; i + 1 < (int)(n / sizeof(unsigned long)); i += 2) {

[Severity: Medium]
What happens if the read() call fails and returns -1?

If n is set to -1, the loop condition implicitly converts it to an unsigned
size_t during the division: n / sizeof(unsigned long). On 32-bit platforms,
this division results in a massive positive integer (0x3FFFFFFF). 

Because this is cast to an int, the loop will iterate far past the end of the
auxv array, leading to an out-of-bounds memory read. Should this check if n
is greater than 0 before proceeding to the loop?

> +		switch (auxv[i]) {
> +		case AT_BASE:
> +			base = auxv[i + 1];
> +			break;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-work-bpf-binfmt_misc-ptinterp-v1-0-ddb76c9a508e@kernel.org?part=20