Re: [PATCH] pstore/ftrace: Factor KASLR offset in the core kernel instruction addresses

[Kees Cook <kees@kernel.org>]

On Fri, Mar 13, 2026 at 05:00:22PM -0300, Guilherme G. Piccoli wrote:
> The pstore ftrace frontend works by purely collecting the
> instruction address, saving it on the persistent area through
> the backend and when the log is read, on next boot for example,
> the address is then resolved by using the regular printk symbol
> lookup (%pS for example).
> 
> Problem: if we are running a relocatable kernel with KASLR enabled,
> this is a recipe for failure in the symbol resolution on next boots,
> since the addresses are offset'ed by the KASLR address. So, naturally
> the way to go is factor the KASLR address out of instruction address
> collection, and adding the fresh offset when resolving the symbol
> on future boots.
> 
> Problem #2: modules also have varying addresses that float based
> on module base address and potentially the module ordering in
> memory, meaning factoring KASLR offset for them is useless.
> So, let's hereby only take KASLR offset into account for core
> kernel addresses, leaving module ones as is.
> 
> And we have yet a 3rd complexity: not necessarily the check range
> for core kernel addresses holds true on future boots, since the
> module base address will vary. With that, the choice was to mark
> the addresses as being core vs module based on its MSB. And with
> that...
> 
> ...we have the 4th challenge here: for some "simple" architectures,
> the CPU number is saved bit-encoded on the instruction pointer, to
> allow bigger timestamps - this is set through the PSTORE_CPU_IN_IP
> define for such architectures. Hence, the approach here is to skip
> such architectures (at least in a first moment).
> 
> TL;DR: let's factor KASLR offsets on pstore/ftrace for core kernel
> addresses, leaving module addresses out and also leaving the
> architectures that define PSTORE_CPU_IN_IP.
> 
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> ---
> 
> 
> Hi folks, first of all thanks in advance for reviews and comments!
> 
> I was testing a pstore/ftrace patch the other day and noticed
> the lack of the KASLR support. But to my surprise, it was not
> as easy to fix up as I expected heh
> 
> Main reason is the obvious thing with modules: the way to
> go, I think, is to somehow save the module name (or some other
> id?) and the instruction offset inside such module, to then
> resolve that in next boot, when printing. But that would require
> more intrusive changes in the way pstore/ftrace saves the IP
> (which is quite simple now), leading to some potentially
> meaningful perf overhead.
> 
> Hence, I've decided to just mess with core kernel addresses
> so far, lemme know WDYT - should I somehow pursue fixing
> modules addr resolution as well? Or doesn't worth the changes?
> Any ideas on how to approach that? I noticed that currently,
> modules' symbols are sometimes resolved fine, sometimes they're
> bogus but point to the module at least (not some other random
> code), but eventually they are just nonsense addresses.
> 
> Regarding the choice of using the MSB to store if an addr is core
> kernel or module, well this was also a choice taking into account
> simplicity and performance, lemme know please if it's no good and
> any suggestions on how to better do it, I can easily re-implement!
> Thanks again,
> 
> Guilherme
> 
> 
>  fs/pstore/ftrace.c   | 33 +++++++++++++++++++++++++++++++--
>  fs/pstore/inode.c    |  6 ++++--
>  fs/pstore/internal.h |  2 ++
>  3 files changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/pstore/ftrace.c b/fs/pstore/ftrace.c
> index 13db123beac1..58f8204b23af 100644
> --- a/fs/pstore/ftrace.c
> +++ b/fs/pstore/ftrace.c
> @@ -18,10 +18,35 @@
>  #include <linux/cache.h>
>  #include <linux/slab.h>
>  #include <asm/barrier.h>
> +#include <asm/setup.h>
>  #include "internal.h"
>  
>  /* This doesn't need to be atomic: speed is chosen over correctness here. */
>  static u64 pstore_ftrace_stamp;
> +unsigned long kaslr_off;

This should at least be "static", but why have it sitting in the data
segment at all, only to be scraped out by attackers with a arbitrary read
primitives? Can we just call kaslr_offset() directly as needed instead
(it's already an inline)?

-Kees

> +
> +static inline unsigned long adjust_ip(unsigned long ip)
> +{
> +#ifndef PSTORE_CPU_IN_IP
> +	if (core_kernel_text(ip))
> +		return ip - kaslr_off;
> +
> +	__clear_bit(BITS_PER_LONG - 1, &ip);
> +#endif
> +	return ip;
> +}
> +
> +inline unsigned long decode_ip(unsigned long ip)
> +{
> +#ifndef PSTORE_CPU_IN_IP
> +	if (test_bit(BITS_PER_LONG - 1, &ip))
> +		return ip + kaslr_off;
> +
> +	__set_bit(BITS_PER_LONG - 1, &ip);
> +
> +#endif
> +	return ip;
> +}
>  
>  static void notrace pstore_ftrace_call(unsigned long ip,
>  				       unsigned long parent_ip,
> @@ -47,8 +72,8 @@ static void notrace pstore_ftrace_call(unsigned long ip,
>  
>  	local_irq_save(flags);
>  
> -	rec.ip = ip;
> -	rec.parent_ip = parent_ip;
> +	rec.ip = adjust_ip(ip);
> +	rec.parent_ip = adjust_ip(parent_ip);
>  	pstore_ftrace_write_timestamp(&rec, pstore_ftrace_stamp++);
>  	pstore_ftrace_encode_cpu(&rec, raw_smp_processor_id());
>  	psinfo->write(&record);
> @@ -132,6 +157,10 @@ void pstore_register_ftrace(void)
>  	if (!psinfo->write)
>  		return;
>  
> +#ifdef CONFIG_RANDOMIZE_BASE
> +	kaslr_off = kaslr_offset();
> +#endif
> +
>  	pstore_ftrace_dir = debugfs_create_dir("pstore", NULL);
>  
>  	pstore_set_ftrace_enabled(record_ftrace);
> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
> index 83fa0bb3435a..62e678f3527d 100644
> --- a/fs/pstore/inode.c
> +++ b/fs/pstore/inode.c
> @@ -105,17 +105,19 @@ static int pstore_ftrace_seq_show(struct seq_file *s, void *v)
>  	struct pstore_private *ps = s->private;
>  	struct pstore_ftrace_seq_data *data = v;
>  	struct pstore_ftrace_record *rec;
> +	unsigned long ip, parent_ip;
>  
>  	if (!data)
>  		return 0;
>  
>  	rec = (struct pstore_ftrace_record *)(ps->record->buf + data->off);
>  
> +	ip = decode_ip(rec->ip);
> +	parent_ip = decode_ip(rec->parent_ip);
>  	seq_printf(s, "CPU:%d ts:%llu %08lx  %08lx  %ps <- %pS\n",
>  		   pstore_ftrace_decode_cpu(rec),
>  		   pstore_ftrace_read_timestamp(rec),
> -		   rec->ip, rec->parent_ip, (void *)rec->ip,
> -		   (void *)rec->parent_ip);
> +		   ip, parent_ip, (void *)ip, (void *)parent_ip);
>  
>  	return 0;
>  }
> diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
> index a0fc51196910..079284120db9 100644
> --- a/fs/pstore/internal.h
> +++ b/fs/pstore/internal.h
> @@ -9,6 +9,7 @@
>  extern unsigned int kmsg_bytes;
>  
>  #ifdef CONFIG_PSTORE_FTRACE
> +extern unsigned long decode_ip(unsigned long ip);
>  extern void pstore_register_ftrace(void);
>  extern void pstore_unregister_ftrace(void);
>  ssize_t pstore_ftrace_combine_log(char **dest_log, size_t *dest_log_size,
> @@ -16,6 +17,7 @@ ssize_t pstore_ftrace_combine_log(char **dest_log, size_t *dest_log_size,
>  #else
>  static inline void pstore_register_ftrace(void) {}
>  static inline void pstore_unregister_ftrace(void) {}
> +static inline unsigned long decode_ip(unsigned long ip) { return ip; }
>  static inline ssize_t
>  pstore_ftrace_combine_log(char **dest_log, size_t *dest_log_size,
>  			  const char *src_log, size_t src_log_size)
> -- 
> 2.50.1
> 
> 

-- 
Kees Cook