Re: [PATCH 2/2] kallsyms: add BTF-based deep parameter rendering in oops dumps

[sashiko-bot@kernel.org]

Sashiko AI review found 7 potential issue(s):
- [High] Unsynchronized static buffers cause data corruption and KASAN out-of-bounds reads in concurrent WARN() context.
- [High] `dwarf_attr_integrate` incorrectly skips function definitions, breaking paraminfo generation.
- [High] Out-of-bounds array read in BTF string coalescing logic.
- [High] Stack buffer overflow risk due to truncated `sym_name` buffer.
- [Medium] `extract_struct_name` silently fails on `const` and `volatile` qualified struct pointers.
- [Medium] Spurious parameter info shown for module functions due to 32-bit truncation hash collisions.
- [Low] Unresolved symbol link errors if KUnit test is built as a module.
--

commit dc1b9fb23e8dd955bb821b603ed59ecf1ae219f5
Author: Sasha Levin <sashal@kernel.org>

kallsyms: add BTF-based deep parameter rendering in oops dumps

This patch adds BTF-based deep rendering for struct pointer parameters in oops
and WARN dumps. It introduces paraminfo_btf_show_ptr() to safely format BPF
data and updates the kallsyms logic to invoke this rendering for pointer
types.

> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -586,13 +586,13 @@ void kallsyms_show_paraminfo(struct pt_regs *regs)
>  	const u8 *data;
>  	unsigned int num_params, i;
>  	unsigned long ip, fault_addr;
> -	char sym_name[KSYM_NAME_LEN];
> +	char sym_name[128];
>  	unsigned long sym_size, sym_offset;

KSYM_NAME_LEN can be configured to larger values than 128 (e.g., 512 for
Rust symbol support via CONFIG_KALLSYMS_MAX_NAME_LEN). If this buffer is
passed to kallsyms APIs that write up to KSYM_NAME_LEN bytes, could this
cause a stack buffer overflow during an oops dump?

[ ... ]

> @@ -611,14 +611,23 @@ void kallsyms_show_paraminfo(struct pt_regs *regs)
>  		return;
>  
>  	/*
> -	 * Verify the IP is within a reasonable range of the function
> -	 * start.  paraminfo_func_addrs[] contains function start offsets;
> -	 * check that we're not too far past the start.  Use kallsyms to
> -	 * verify we're in the right function.
> +	 * Verify the paraminfo entry actually matches the function
> +	 * containing the IP.  Without this, if the faulting function
> +	 * has no paraminfo, the binary search silently returns the
> +	 * preceding function's entry — showing wrong parameter info.
>  	 */
>  	if (!kallsyms_lookup_size_offset(ip, &sym_size, &sym_offset))
>  		return;
>  
> +	{
> +		unsigned int func_start_offset;
> +
> +		func_start_offset = (unsigned int)(ip - sym_offset -
> +						   (unsigned long)_text);
> +		if (paraminfo_func_addrs[func_idx] != func_start_offset)
> +			return;
> +	}
> +

For an instruction pointer within a kernel module, the memory distance
from the core _text section can exceed 4GB on 64-bit architectures where
modules reside in vmalloc space.

Will casting this distance to a 32-bit integer truncate the upper bits? If
the truncated value coincidentally matches a valid entry in
paraminfo_func_addrs, could this incorrectly display parameter info for a
completely unrelated vmlinux function?

> diff --git a/kernel/kallsyms_paraminfo_btf.c b/kernel/kallsyms_paraminfo_btf.c
> --- /dev/null
> +++ b/kernel/kallsyms_paraminfo_btf.c
> @@ -0,0 +1,267 @@
[ ... ]
> +static const char *extract_struct_name(const char *type_str, bool *is_union,
> +				       char *name_buf, size_t bufsz)
> +{
> +	const char *p, *end;
> +
> +	*is_union = false;
> +
> +	/* Must end with " *" to be a pointer */
> +	end = type_str + strlen(type_str);
> +	if (end - type_str < 3 || end[-1] != '*' || end[-2] != ' ')
> +		return NULL;
> +
> +	if (!strncmp(type_str, "struct ", 7)) {
> +		p = type_str + 7;
> +	} else if (!strncmp(type_str, "union ", 6)) {

The extract_struct_name() function checks for "struct " at the very
beginning of the string. Since gen_paraminfo.c generates strings with
qualifiers for pointers like "const struct file *", won't this strict
comparison fail on qualified strings and silently bypass BTF rendering?

[ ... ]

> +void paraminfo_btf_show_ptr(unsigned long ptr_val, const char *type_str)
> +{
> +	static char buf[BTF_SHOW_BUF_LEN];
> +	char sname[64];

[ ... ]

> +#define MAX_COALESCED 8
> +		{
> +			static char coalesced[MAX_COALESCED][128];
> +			int ci = 0, i;

The comment states this is safe because oops context is effectively
single-threaded. However, since this function is also invoked during
WARN() dumps, and WARN() does not stop other CPUs, couldn't multiple
CPUs hit warnings and execute this code simultaneously?

Will concurrent lockless modifications to these static arrays cause data
races, garbled output, and potential out-of-bounds memory accesses if
null terminators are overwritten mid-read?

[ ... ]

> +				/* Gather chars from subsequent lines */
> +				for (j = i + 1; j < nlines &&
> +				     spos < (int)sizeof(coalesced[0]) - 2; j++) {
> +					char *s = lines[j];
> +
> +					if (s[0] == '\'' && s[2] == '\'' &&
> +					    (s[3] == '\0' || s[3] == ',')) {
> +						coalesced[ci][spos++] = s[1];
> +						lines[j] = "";

This evaluates s[2] and s[3] without validating the string's length. If
the parsed string is a single character (e.g., s[0] == '\'' and
s[1] == '\0'), won't this read past the null terminator?

If this short string is located at the very end of the buf array, will
evaluating s[2] read past the global array's bounds and trigger a KASAN
out-of-bounds read?

> diff --git a/lib/tests/paraminfo_kunit.c b/lib/tests/paraminfo_kunit.c
> --- a/lib/tests/paraminfo_kunit.c
> +++ b/lib/tests/paraminfo_kunit.c
> @@ -7,7 +7,8 @@
>   * Verifies that the paraminfo tables correctly map function addresses
>   * to their parameter names and types.
>   *
> - * Build with: CONFIG_PARAMINFO_KUNIT_TEST=m (or =y)
> + * Build with: CONFIG_PARAMINFO_KUNIT_TEST=y (must be built-in; paraminfo
> + * tables are vmlinux-only, so module test functions won't be found)
>   */

This isn't a bug, but the comment notes this test must be built-in
because it relies on unexported vmlinux-only symbols.

If there is no corresponding Kconfig dependency enforced, could
configuring the test as a module cause the build to fail with undefined
reference errors during modpost?

> diff --git a/scripts/gen_paraminfo.c b/scripts/gen_paraminfo.c
> --- a/scripts/gen_paraminfo.c
> +++ b/scripts/gen_paraminfo.c
> @@ -341,6 +384,12 @@ static void process_dwarf(Dwarf *dwarf, unsigned long long text_addr)
[ ... ]
>  			/* Skip declarations (no body) */
> -			if (dwarf_attr(&child, DW_AT_declaration, &attr))
> +			if (dwarf_attr_integrate(&child, DW_AT_declaration, &attr))
>  				continue;

For a function definition that was prototyped elsewhere, the definition
DIE contains a DW_AT_specification attribute pointing to its declaration
DIE.

Because dwarf_attr_integrate resolves attributes through the
specification chain, won't it locate DW_AT_declaration on the
declaration DIE and evaluate to true?

Could this inadvertently cause the script to skip almost all exported
kernel function definitions?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260323164858.1939248-1-sashal@kernel.org?part=2