Message-ID: <20250603211547.32aed8e9@gandalf.local.home>
Date: Tue, 3 Jun 2025 21:15:47 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Sasha Levin <sashal@...nel.org>
Cc: patches@...ts.linux.dev, stable@...r.kernel.org, Masami Hiramatsu
 <mhiramat@...nel.org>, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH AUTOSEL 6.15 070/118] tracing: Only return an adjusted
 address if it matches the kernel address

On Tue,  3 Jun 2025 20:50:01 -0400
Sasha Levin <sashal@...nel.org> wrote:

> From: Steven Rostedt <rostedt@...dmis.org>
> 
> [ Upstream commit 00d872dd541cdf22230510201a1baf58f0147db9 ]
> 
> The trace_adjust_address() will take a given address and examine the
> persistent ring buffer to see if the address matches a module that is
> listed there. If it does not, it will just adjust the value to the core
> kernel delta. But if the address was for something that was not part of
> the core kernel text or data it should not be adjusted.
> 
> Check the result of the adjustment and only return the adjustment if it
> lands in the current kernel text or data. If not, return the original
> address.
> 
> Cc: Masami Hiramatsu <mhiramat@...nel.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
> Link: https://lore.kernel.org/20250506102300.0ba2f9e0@gandalf.local.home
> Signed-off-by: Steven Rostedt (Google) <rostedt@...dmis.org>
> Signed-off-by: Sasha Levin <sashal@...nel.org>
> ---
> 

I guess the following blurb is new.

> **YES** This commit should be backported to stable kernel trees based on

Hmm, I'm not so sure the analysis is correct.

> the following comprehensive analysis: ## Security Analysis ### 1.
> **Critical Security Fix** The commit addresses a significant security
> vulnerability in the `trace_adjust_address()` function. The change on
> lines 6148-6149 adds crucial validation: ```c raddr = addr +
> tr->text_delta; return __is_kernel(raddr) || is_kernel_core_data(raddr)
> || is_kernel_rodata(raddr) ? raddr : addr; ``` **Before the fix**: The
> function would blindly return `addr + tr->text_delta` without validating
> whether the resulting address falls within legitimate kernel memory

If you look at the code, it will return the address regardless if it is
within the kernel memory or not.

This is called when reading addresses that are in the persistent ring
buffer from a previous boot.

Before the "fix":

  It would always add the text_delta to the address.

The issue without that is that it could be adjusting a pointer that was to
allocated memory. It makes no sense to do this. The reason for doing this
adjustment is because a lot of reads of addresses use "%pS", and we care
only about getting a proper kallsyms of the address.

Thus what is done is:

		raddr = addr + tr->text_delta;
		return __is_kernel(raddr) || is_kernel_core_data(raddr) ||
			is_kernel_rodata(raddr) ? raddr : addr;

Which does the adjustment, and if it falls into kernel memory or data
return that adjustment, otherwise return the original address. The reason
is that by returning the adjusted memory, it may fall into a module that we
do not want to print kallsyms for.

> regions. **After the fix**: The function validates that adjusted
> addresses only point to valid kernel sections (text, core data, or read-
> only data). If the adjusted address doesn't fall within these legitimate
> regions, it returns the original address unchanged. ### 2. **KASLR
> Protection** This fix is particularly important for KASLR (Kernel
> Address Space Layout Randomization) security: - **Information Disclosure

It doesn't risk any KASLR information. All addresses used by
trace_adjust_address() is from a pointer that existed in a previous boot.
The adjustment is pretty meaningless if it's not in kernel text or data.

> Risk**: Without validation, the function could return addresses pointing
> to arbitrary memory locations, potentially leaking kernel address layout
> information - **KASLR Bypass**: Invalid address adjustments could reveal
> the kernel's memory layout, defeating KASLR protections - **Cross-boot
> attacks**: The persistent ring buffer functionality could be exploited
> to extract address information from previous boot sessions ### 3.
> **Memory Safety** The validation prevents potential memory corruption
> scenarios: - **Out-of-bounds access**: Ensures addresses used for symbol
> resolution and string dereferencing are within valid kernel regions -
> **Kernel crashes**: Prevents dereferencing of invalid addresses that
> could cause kernel panics - **Information leakage**: Stops potential
> disclosure of arbitrary kernel memory contents ### 4. **Limited Risk of
> Regression** This is a defensive fix that only affects the tracing
> subsystem: - **Minimal impact**: Only changes behavior when addresses
> would be invalid anyway - **Backwards compatible**: Legitimate addresses
> continue to work as before - **Contained scope**: Limited to persistent
> ring buffer functionality ### 5. **Alignment with Stable Tree Criteria**
> The commit meets all the criteria for stable tree backporting: - ✅
> **Important bug fix**: Addresses a security vulnerability - ✅ **Small
> and contained**: Only adds validation logic, doesn't change architecture
> - ✅ **Minimal regression risk**: Defensive change that only affects
> invalid cases - ✅ **Clear benefit**: Improves kernel security without
> breaking functionality - ✅ **Non-feature**: Pure security fix, not
> adding new functionality ### 6. **Real-world Impact** This vulnerability
> could be exploited in environments where: - Persistent tracing is
> enabled across reboots - Attackers have access to trace output - Systems

Yes persistent tracing is enabled across reboots and the address is from a
previous boot. It does return the actual address of the current boot to use
with %pS when it was on the kernel text or data address. When it isn't
(likely a module address) the adjustment is meaningless and may give bad
trace output at most.

If an attacker has access to trace output KASLR is already lost, as
function tracing records raw addresses and exposes everything KASLR, which
is why reading these files is a privilege operation.

I won't argue against backporting, but I just wanted to state this analysis
may not be correct.

-- Steve


> rely on KASLR for security The fix ensures that the tracing subsystem
> cannot be used as a vector for kernel address space information
> disclosure or memory corruption attacks. **Conclusion**: This is a clear
> security fix that should be backported to maintain the security
> integrity of stable kernel releases.
> 
>  kernel/trace/trace.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 5b8db27fb6ef3..01572ef79802f 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -6032,6 +6032,7 @@ unsigned long trace_adjust_address(struct trace_array *tr, unsigned long addr)
>  	struct trace_module_delta *module_delta;
>  	struct trace_scratch *tscratch;
>  	struct trace_mod_entry *entry;
> +	unsigned long raddr;
>  	int idx = 0, nr_entries;
>  
>  	/* If we don't have last boot delta, return the address */
> @@ -6045,7 +6046,9 @@ unsigned long trace_adjust_address(struct trace_array *tr, unsigned long addr)
>  	module_delta = READ_ONCE(tr->module_delta);
>  	if (!module_delta || !tscratch->nr_entries ||
>  	    tscratch->entries[0].mod_addr > addr) {
> -		return addr + tr->text_delta;
> +		raddr = addr + tr->text_delta;
> +		return __is_kernel(raddr) || is_kernel_core_data(raddr) ||
> +			is_kernel_rodata(raddr) ? raddr : addr;
>  	}
>  
>  	/* Note that entries must be sorted. */

Powered by blists - more mailing lists