Re: [PATCH bpf-next 10/11] selftests/bpf: Add tests for bpf keyring in signed loader

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] A ternary operator logic error causes a NULL pointer dereference if `strstr` returns `NULL` in `bpf_keyring_lookup`.
--

--- Patch [10]: [PATCH bpf-next 10/11] selftests/bpf: Add tests for bpf keyring in signed loader ---
commit c9f1ea97a7848e9a19ec91707712827e46792acd
Author: Daniel Borkmann <[email protected]>

selftests/bpf: Add tests for bpf keyring in signed loader

This commit adds selftests for the bpf keyring in the signed loader. It
verifies the keyring through its lifecycle, including key generation,
enrollment, and testing loading restrictions based on the keyring state.

> diff --git a/tools/testing/selftests/bpf/prog_tests/signed_loader.c b/tools/testing/selftests/bpf/prog_tests/signed_loader.c
> --- a/tools/testing/selftests/bpf/prog_tests/signed_loader.c
> +++ b/tools/testing/selftests/bpf/prog_tests/signed_loader.c

[ ... ]

> +static int bpf_keyring_lookup(int *nr_keys)
> +{
> +	char line[512], type[32], desc[64];
> +	int serial = -ENOENT;
> +	FILE *f;
> +
> +	f = fopen("/proc/keys", "r");
> +	if (!f)
> +		return -errno;
> +
> +	while (fgets(line, sizeof(line), f)) {
> +		unsigned int hex;
> +		char *sum;
> +
> +		if (sscanf(line, "%x %*s %*s %*s %*s %*s %*s %31s %63s",
> +			   &hex, type, desc) != 3)
> +			continue;
> +		if (strcmp(type, "keyring") || strcmp(desc, ".bpf:"))
> +			continue;
> +
> +		serial = (int)hex;
> +		if (nr_keys) {
> +			sum = strstr(line, ".bpf: ");
> +			*nr_keys = (sum && !strncmp(sum + 6, "empty", 5)) ?
> +				   0 : atoi(sum + 6);

[Severity: Medium]
Could this ternary expression result in a NULL pointer dereference?

If strstr does not find ".bpf: ", sum will be NULL. In that case, the
ternary condition (sum && !strncmp(sum + 6, "empty", 5)) evaluates to
false. The execution will then fall through to the false branch,
calling atoi(sum + 6).

Since sum is NULL, this leads to a dereference of NULL + 6, which
would crash the selftest binary with a segmentation fault.

This situation could occur if the keyring description is parsed as
.bpf: by sscanf above, but the line does not contain the exact
substring ".bpf: " (with the trailing space).

> +		}
> +		break;
> +	}
> +	fclose(f);
> +	return serial;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10