Re: [PATCH v3 bpf-next 7/7] selftests/bpf: Add a test verifying inline information

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] bpf: null pointer dereference in test_btf_inline() on error path
- [Medium] bpf: invalid pointer dereference in test_btf_inline() due to missing NULL check
- [Medium] bpf: uninitialized variable usage in module_get_base_addr()
- [Low] bpf: inverted assertion logic in test_btf_inline() breaks failure-abort pattern
--

commit 0f45ec42b3956956fce6d91dc0397c28034b638d
Author: Alan Maguire <alan.maguire@oracle.com>

selftests/bpf: Add a test verifying inline information

This commit adds a selftest for bpf_testmod to verify that inline information
is correctly generated and matches expectations for an always-inline function.
It validates the LOCSEC descriptor offset and skips the test if pahole does
not encode inline information.

> diff --git a/tools/testing/selftests/bpf/prog_tests/btf_inline.c b/tools/testing/selftests/bpf/prog_tests/btf_inline.c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/btf_inline.c
[ ... ]
> +void test_btf_inline(void)
> +{
[ ... ]
> +	inline_btf = btf__parse_split(BTF_SYSFS_DIR "/bpf_testmod" BTF_INLINE_SUFFIX,
> +				      btf);
> +	err = libbpf_get_error(inline_btf);
> +	/* pahole may not have inline BTF feature support. */
> +	if (err == -ENOENT) {
> +		skip = true;
> +		goto out;
> +	}
> +	locsec_id = btf__find_by_name_kind(inline_btf, "inline.text", BTF_KIND_LOCSEC);

[Severity: Medium]
What happens if btf__parse_split() fails with an error other than -ENOENT,
such as -ENOMEM?

If inline_btf is an error pointer or NULL, the condition checking for -ENOENT
evaluates to false, and the invalid inline_btf pointer is passed directly to
btf__find_by_name_kind().

Can this cause a null pointer dereference when libbpf attempts to access the
BTF struct?

[ ... ]
> +	t = btf__type_by_id(inline_btf, l->loc_proto);
> +	if (!ASSERT_OK_PTR(t, "loc_proto_ptr"))
> +		goto out;
> +	if (!ASSERT_EQ(btf_vlen(t), 1, "loc_proto_one_param"))
> +		goto out;
> +	p = btf_loc_proto_params(t);
> +	t = btf__type_by_id(inline_btf, *p);
> +	lp = btf_loc_param(t);
> +	if (!ASSERT_EQ(lp->flags, BTF_LOC_PARAM_REG, "param_is_reg"))

[Severity: Medium]
Does the second call to btf__type_by_id() require a check to ensure 't' is
not NULL before calculating the location param?

If *p is an invalid type ID, t would be NULL, and passing it to
btf_loc_param() calculates an offset from a NULL pointer. Could this cause a
segmentation fault when lp->flags is accessed?

> +		goto out;
> +	if (!ASSERT_GT(l->offset, caller_addr, "inline_gt_caller"))
> +		goto out;
> +	/* simple sanity test to roughly ensure inline site still in function */
> +	if (ASSERT_LT(l->offset, caller_addr + 256, "inline_in_caller"))
> +		goto out;

[Severity: Low]
Is the logic inverted on this ASSERT_LT() check?

The other assertions use the ! operator to jump to 'out' on failure. Because
ASSERT_LT() returns true when the check succeeds, it looks like this will jump
to 'out' on success instead of on failure. While this might be benign right
now as the last statement, will it bypass any future checks appended here?


> diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c
> --- a/tools/testing/selftests/bpf/trace_helpers.c
> +++ b/tools/testing/selftests/bpf/trace_helpers.c
[ ... ]
> +long module_get_base_addr(const char *module)
> +{
[ ... ]
> +	while (fscanf(f, "%s %*s %*s %*s %*s 0x%llx", name, &addr) > 0) {
> +		if (strcmp(module, name) == 0) {
> +			base_addr = addr;
> +			break;
> +		}
> +	}

[Severity: Medium]
Can fscanf() return 1 if it successfully parses the module name but fails to
parse the address field?

If the line format in /proc/modules is unexpected or malformed, and fscanf()
returns 1, the check > 0 will pass, but the addr variable will remain
uninitialized. If the module name matches, could this return uninitialized
stack memory as the base address? Would it be safer to check that fscanf
returns exactly 2?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261003194323.817885-1-alan.maguire@oracle.com?part=7