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 <[email protected]> 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/[email protected]?part=7
