Re: [PATCH v8 1/4] kallsyms: embed source file:line info in kernel stack traces

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Use of `snprintf` violates `sprint_symbol` API contract and causes trace buffer inflation.
--

--- Patch [1]: [PATCH v8 1/4] kallsyms: embed source file:line info in kernel stack traces ---
commit 8a14a0edefef69e5b032ac47662777d287f71164
Author: Sasha Levin <sashal@kernel.org>

kallsyms: embed source file:line info in kernel stack traces

Add CONFIG_KALLSYMS_LINEINFO, which embeds a compact address-to-line
lookup table in the kernel image so stack traces directly print source
file and line number information.

> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c

[ ... ]

> @@ -497,6 +567,23 @@ static int __sprint_symbol(char *buffer, unsigned long address,
>  		len += sprintf(buffer + len, "]");
>  	}
>  
> +	/*
> +	 * Append "(file:line)" only for stack-backtrace consumers.  Plain
> +	 * sprint_symbol() backs %ps, and many existing format strings tack
> +	 * literal "()" after %ps to indicate a function call ("foo()
> +	 * replaced with bar()"); appending lineinfo there would produce a
> +	 * confusing "foo (file:line)()".
> +	 */
> +	if (add_lineinfo && IS_ENABLED(CONFIG_KALLSYMS_LINEINFO) && !modname) {
> +		const char *li_file;
> +		unsigned int li_line;
> +
> +		if (kallsyms_lookup_lineinfo(address, sym_start,
> +					     &li_file, &li_line))
> +			len += snprintf(buffer + len, KSYM_SYMBOL_LEN - len,
> +					" (%s:%u)", li_file, li_line);

[Severity: Medium]
Is it safe to use snprintf() here instead of scnprintf()?

The API contract for sprint_symbol() specifies it returns the number of bytes
stored. If the source file path is very long and truncation occurs, snprintf()
will return the un-truncated length.

This can cause caller logic errors. For example, if a kprobe triggers on a
symbol with a long source path, fetch_store_symstrlen() will allocate buffer
space based on this inflated return value. When fetch_store_symstring() writes
the truncated string, the tail of the allocated ring buffer remains
uninitialized. Future callers attempting pointer advancement via
buf += sprint_symbol(...) could also be exposed to out-of-bounds writes.

Could this be changed to use scnprintf() to ensure it returns the actual
number of bytes written?

> +	}
> +
>  	return len;
>  }

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