[PATCH 0/6] kallsyms: Prevent invalid access when showing module buildid

[PATCH 0/6] kallsyms: Prevent invalid access when showing module buildid

[Petr Mladek]

We have seen nested crashes in __sprint_symbol(), see below. They seems
to be caused by invalid pointer to "buildid".

I made an audit of __sprint_symbol() and found several situations
when the buildid might be wrong:

  + bpf_address_lookup() does not set @modbuildid

  + ftrace_mod_address_lookup() does not set @modbuildid

  + __sprint_symbol() does not take rcu_read_lock and
    the related struct module might get removed before
    mod->build_id is printed.

This patchset solves these problems:

  + 1st, 2nd patches are preparatory
  + 3rd, 4th, 6th patches fix the above problems
  + 5th patch cleans up a suspicious initialization code.

This is the backtrace, we have seen. But it is not really important.
The problems fixed by the patchset are obvious:

  crash64> bt [62/2029]
  PID: 136151 TASK: ffff9f6c981d4000 CPU: 367 COMMAND: "btrfs"
  #0 [ffffbdb687635c28] machine_kexec at ffffffffb4c845b3
  #1 [ffffbdb687635c80] __crash_kexec at ffffffffb4d86a6a
  #2 [ffffbdb687635d08] hex_string at ffffffffb51b3b61
  #3 [ffffbdb687635d40] crash_kexec at ffffffffb4d87964
  #4 [ffffbdb687635d50] oops_end at ffffffffb4c41fc8
  #5 [ffffbdb687635d70] do_trap at ffffffffb4c3e49a
  #6 [ffffbdb687635db8] do_error_trap at ffffffffb4c3e6a4
  #7 [ffffbdb687635df8] exc_stack_segment at ffffffffb5666b33
  #8 [ffffbdb687635e20] asm_exc_stack_segment at ffffffffb5800cf9
  #9 [ffffbdb687635ea8] hex_string at ffffffffb51b3b61
  #10 [ffffbdb687635ef8] vsnprintf at ffffffffb51b7291
  #11 [ffffbdb687635f50] sprintf at ffffffffb51b7541
  #12 [ffffbdb687635fb8] __sprint_symbol at ffffffffb4d849d6
  #13 [ffffbdb687636018] symbol_string at ffffffffb51b4588
  #14 [ffffbdb687636168] vsnprintf at ffffffffb51b7291
  #15 [ffffbdb6876361c0] vscnprintf at ffffffffb51b73b9
  #16 [ffffbdb6876361d0] printk_sprint at ffffffffb4d2ae82
  #17 [ffffbdb6876361f8] vprintk_store at ffffffffb4d2d06d
  #18 [ffffbdb6876362c8] vprintk_emit at ffffffffb4d2d1bf
  #19 [ffffbdb687636308] printk at ffffffffb565e5ce
  #20 [ffffbdb687636370] show_trace_log_lvl at ffffffffb4c42374
  #21 [ffffbdb687636478] __die_body at ffffffffb4c426ca
  #22 [ffffbdb687636498] die at ffffffffb4c42778
  #23 [ffffbdb6876364c0] do_trap at ffffffffb4c3e49a
  #24 [ffffbdb687636508] do_error_trap at ffffffffb4c3e6a4
  #25 [ffffbdb687636548] exc_stack_segment at ffffffffb5666b33
  #26 [ffffbdb687636570] asm_exc_stack_segment at ffffffffb5800cf9
  #27 [ffffbdb6876365f8] hex_string at ffffffffb51b3b61
  #28 [ffffbdb687636648] vsnprintf at ffffffffb51b7291
  #29 [ffffbdb6876366a0] sprintf at ffffffffb51b7541
  #30 [ffffbdb687636708] __sprint_symbol at ffffffffb4d849d6
  #31 [ffffbdb687636768] symbol_string at ffffffffb51b4588
  #32 [ffffbdb6876368b8] vsnprintf at ffffffffb51b7291
  #33 [ffffbdb687636910] vscnprintf at ffffffffb51b73b9
  #34 [ffffbdb687636920] printk_sprint at ffffffffb4d2ae82
  #35 [ffffbdb687636948] vprintk_store at ffffffffb4d2d06d
  #36 [ffffbdb687636a18] vprintk_emit at ffffffffb4d2d1bf
  #37 [ffffbdb687636a58] printk at ffffffffb565e5ce
  #38 [ffffbdb687636ac0] show_trace_log_lvl at ffffffffb4c42374
  #39 [ffffbdb687636bc8] __die_body at ffffffffb4c426ca
  #40 [ffffbdb687636be8] die at ffffffffb4c42778
  #41 [ffffbdb687636c10] do_trap at ffffffffb4c3e49a
  #42 [ffffbdb687636c58] do_error_trap at ffffffffb4c3e6a4
  #43 [ffffbdb687636c98] exc_stack_segment at ffffffffb5666b33
  #44 [ffffbdb687636cc0] asm_exc_stack_segment at ffffffffb5800cf9
  #45 [ffffbdb687636d48] hex_string at ffffffffb51b3b61
  #46 [ffffbdb687636d98] vsnprintf at ffffffffb51b7291
  #47 [ffffbdb687636df0] sprintf at ffffffffb51b7541
  #48 [ffffbdb687636e58] __sprint_symbol at ffffffffb4d849d6
  #49 [ffffbdb687636eb8] symbol_string at ffffffffb51b4588
  #50 [ffffbdb687637008] vsnprintf at ffffffffb51b7291
  #51 [ffffbdb687637060] vscnprintf at ffffffffb51b73b9
  #52 [ffffbdb687637070] printk_sprint at ffffffffb4d2ae82
  #53 [ffffbdb687637098] vprintk_store at ffffffffb4d2d06d
  #54 [ffffbdb687637168] vprintk_emit at ffffffffb4d2d1bf
  #55 [ffffbdb6876371a8] printk at ffffffffb565e5ce
  #56 [ffffbdb687637210] show_trace_log_lvl at ffffffffb4c42374
  #57 [ffffbdb687637318] __die_body at ffffffffb4c426ca
  #58 [ffffbdb687637338] die at ffffffffb4c42778
  #59 [ffffbdb687637360] do_trap at ffffffffb4c3e49a
  #60 [ffffbdb6876373a8] do_error_trap at ffffffffb4c3e6a4
  #61 [ffffbdb6876373e8] exc_stack_segment at ffffffffb5666b33
  #62 [ffffbdb687637410] asm_exc_stack_segment at ffffffffb5800cf9
  #63 [ffffbdb687637498] hex_string at ffffffffb51b3b61
  #64 [ffffbdb6876374e8] vsnprintf at ffffffffb51b7291
  #65 [ffffbdb687637540] sprintf at ffffffffb51b7541
  #66 [ffffbdb6876375a8] __sprint_symbol at ffffffffb4d849d6
  #67 [ffffbdb687637608] symbol_string at ffffffffb51b4588
  #68 [ffffbdb687637758] vsnprintf at ffffffffb51b7291
  #69 [ffffbdb6876377b0] vscnprintf at ffffffffb51b73b9
  #70 [ffffbdb6876377c0] printk_sprint at ffffffffb4d2ae82
  #71 [ffffbdb6876377e8] vprintk_store at ffffffffb4d2d06d
  #72 [ffffbdb6876378b8] vprintk_emit at ffffffffb4d2d1bf
  #73 [ffffbdb6876378f8] printk at ffffffffb565e5ce
  #74 [ffffbdb687637960] show_trace_log_lvl at ffffffffb4c42374
  #75 [ffffbdb687637a68] __warn at ffffffffb4cb0d4d
  #76 [ffffbdb687637aa0] report_bug at ffffffffb51a73fb
  #77 [ffffbdb687637ad8] handle_bug at ffffffffb5666817
  #78 [ffffbdb687637ae8] exc_invalid_op at ffffffffb56669d3
  #79 [ffffbdb687637b00] asm_exc_invalid_op at ffffffffb5800e0d
  [exception RIP: btrfs_ioctl_send+0x26e]
  RIP: ffffffffc06070ce RSP: ffffbdb687637bb8 RFLAGS: 00010282
  RAX: ffff9f6e50160380 RBX: ffff9f8eda64f200 RCX: 0000000000000000
  RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
  RBP: 0000000000000000 R8: 000000000000000a R9: ffff9f6c9e1c5b20
  R10: 0000000000000075 R11: 0000000000000004 R12: ffff9f6d43a24000
  R13: 0000000000000001 R14: 0000000000000000 R15: ffff9a2d65644d30
  ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
  #80 [ffffbdb687637c78] _btrfs_ioctl_send at ffffffffc05c31d4 [btrfs]
  #81 [ffffbdb687637ce8] btrfs_ioctl at ffffffffc05c80c4 [btrfs]
  #82 [ffffbdb687637df8] __x64_sys_ioctl at ffffffffb4f776df
  #83 [ffffbdb687637e38] do_syscall_64 at ffffffffb56663f8
  RIP: 00007fbd339164a7 RSP: 00007ffde6a19888 RFLAGS: 00000246
  RAX: ffffffffffffffda RBX: 0000000000000fe2 RCX: 00007fbd339164a7
  RDX: 00007ffde6a19980 RSI: 0000000040489426 RDI: 0000000000000022
  RBP: 00007ffde6a1ab80 R8: 00007ffde6a19980 R9: 00007fbd33808700
  R10: 00007fbd338089d0 R11: 0000000000000246 R12: 0000000000000022
  R13: 0000000000000001 R14: 0000000000000001 R15: 00005585428002b0
  ORIG_RAX: 0000000000000010 CS: 0033 SS: 002b

Petr Mladek (6):
  module: Add helper function for reading module_buildid()
  kallsyms: Cleanup code for appending the module buildid
  kallsyms/bpf: Set module buildid in bpf_address_lookup()
  kallsyms/ftrace: Set module buildid in ftrace_mod_address_lookup()
  kallsyms: Clean up @namebuf initialization in
    kallsyms_lookup_buildid()
  kallsyms: Prevent module removal when printing module name and buildid

 include/linux/filter.h   | 15 +++++++---
 include/linux/ftrace.h   |  6 ++--
 include/linux/module.h   |  9 ++++++
 kernel/kallsyms.c        | 60 ++++++++++++++++++++++++++++++----------
 kernel/module/kallsyms.c |  9 ++----
 kernel/trace/ftrace.c    |  5 +++-
 6 files changed, 76 insertions(+), 28 deletions(-)

-- 
2.51.1

[PATCH 1/6] module: Add helper function for reading module_buildid()

[Petr Mladek]

Add a helper function for reading the optional "build_id" member
of struct module. It is going to be used also in
ftrace_mod_address_lookup().

Use "#ifdef" instead of "#if IS_ENABLED()" to match the declaration
of the optional field in struct module.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 include/linux/module.h   | 9 +++++++++
 kernel/module/kallsyms.c | 9 ++-------
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index e135cc79acee..4decae2b1675 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -747,6 +747,15 @@ static inline void __module_get(struct module *module)
 	__mod ? __mod->name : "kernel";		\
 })
 
+static inline const unsigned char *module_buildid(struct module *mod)
+{
+#ifdef CONFIG_STACKTRACE_BUILD_ID
+	return mod->build_id;
+#else
+	return NULL;
+#endif
+}
+
 /* Dereference module function descriptor */
 void *dereference_module_function_descriptor(struct module *mod, void *ptr);
 
diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
index 00a60796327c..0fc11e45df9b 100644
--- a/kernel/module/kallsyms.c
+++ b/kernel/module/kallsyms.c
@@ -334,13 +334,8 @@ int module_address_lookup(unsigned long addr,
 	if (mod) {
 		if (modname)
 			*modname = mod->name;
-		if (modbuildid) {
-#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
-			*modbuildid = mod->build_id;
-#else
-			*modbuildid = NULL;
-#endif
-		}
+		if (modbuildid)
+			*modbuildid = module_buildid(mod);
 
 		sym = find_kallsyms_symbol(mod, addr, size, offset);
 
-- 
2.51.1

[PATCH 2/6] kallsyms: Cleanup code for appending the module buildid

[Petr Mladek]

Put the code for appending the optional "buildid" into a helper
function, It makes __sprint_symbol() better readable.

Also print a warning when the "modname" is set and the "buildid" isn't.
It might catch a situation when some lookup function in
kallsyms_lookup_buildid() does not handle the "buildid".

Use pr_*_once() to avoid an infinite recursion when the function
is called from printk(). The recursion is rather theoretical but
better be on the safe side.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/kallsyms.c | 42 +++++++++++++++++++++++++++++++++---------
 1 file changed, 33 insertions(+), 9 deletions(-)

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 1e7635864124..9455e3bb07fc 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -423,6 +423,37 @@ int lookup_symbol_name(unsigned long addr, char *symname)
 	return lookup_module_symbol_name(addr, symname);
 }
 
+#ifdef CONFIG_STACKTRACE_BUILD_ID
+
+static int append_buildid(char *buffer,  const char *modname,
+			  const unsigned char *buildid)
+{
+	if (!modname)
+		return 0;
+
+	if (!buildid) {
+		pr_warn_once("Undefined buildid for the module %s\n", modname);
+		return 0;
+	}
+
+	/* build ID should match length of sprintf */
+#ifdef CONFIG_MODULES
+	static_assert(sizeof(typeof_member(struct module, build_id)) == 20);
+#endif
+
+	return sprintf(buffer, " %20phN", buildid);
+}
+
+#else /* CONFIG_STACKTRACE_BUILD_ID */
+
+static int append_buildid(char *buffer,   const char *modname,
+			  const unsigned char *buildid)
+{
+	return 0;
+}
+
+#endif /* CONFIG_STACKTRACE_BUILD_ID */
+
 /* Look up a kernel symbol and return it in a text buffer. */
 static int __sprint_symbol(char *buffer, unsigned long address,
 			   int symbol_offset, int add_offset, int add_buildid)
@@ -445,15 +476,8 @@ static int __sprint_symbol(char *buffer, unsigned long address,
 
 	if (modname) {
 		len += sprintf(buffer + len, " [%s", modname);
-#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
-		if (add_buildid && buildid) {
-			/* build ID should match length of sprintf */
-#if IS_ENABLED(CONFIG_MODULES)
-			static_assert(sizeof(typeof_member(struct module, build_id)) == 20);
-#endif
-			len += sprintf(buffer + len, " %20phN", buildid);
-		}
-#endif
+		if (add_buildid)
+			len += append_buildid(buffer + len, modname, buildid);
 		len += sprintf(buffer + len, "]");
 	}
 
-- 
2.51.1

[PATCH 3/6] kallsyms/bpf: Set module buildid in bpf_address_lookup()

[Petr Mladek]

Make bpf_address_lookup() compatible with module_address_lookup()
and clear the pointer to @modbuildid together with @modname.

It is not strictly needed because __sprint_symbol() reads @modbuildid
only when @modname is set. But better be on the safe side and make
the API more safe.

Fixes: 9294523e3768 ("module: add printk formats to add module build ID to stacktraces")
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 include/linux/filter.h | 15 +++++++++++----
 kernel/kallsyms.c      |  4 ++--
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index f5c859b8131a..b7b95840250a 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1362,12 +1362,18 @@ struct bpf_prog *bpf_prog_ksym_find(unsigned long addr);
 
 static inline int
 bpf_address_lookup(unsigned long addr, unsigned long *size,
-		   unsigned long *off, char **modname, char *sym)
+		   unsigned long *off, char **modname,
+		   const unsigned char **modbuildid, char *sym)
 {
 	int ret = __bpf_address_lookup(addr, size, off, sym);
 
-	if (ret && modname)
-		*modname = NULL;
+	if (ret) {
+		if (modname)
+			*modname = NULL;
+		if (modbuildid)
+			*modbuildid = NULL;
+	}
+
 	return ret;
 }
 
@@ -1433,7 +1439,8 @@ static inline struct bpf_prog *bpf_prog_ksym_find(unsigned long addr)
 
 static inline int
 bpf_address_lookup(unsigned long addr, unsigned long *size,
-		   unsigned long *off, char **modname, char *sym)
+		   unsigned long *off, char **modname,
+		   const unsigned char **modbuildid, char *sym)
 {
 	return 0;
 }
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 9455e3bb07fc..efb12b077220 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -374,8 +374,8 @@ static int kallsyms_lookup_buildid(unsigned long addr,
 	ret = module_address_lookup(addr, symbolsize, offset,
 				    modname, modbuildid, namebuf);
 	if (!ret)
-		ret = bpf_address_lookup(addr, symbolsize,
-					 offset, modname, namebuf);
+		ret = bpf_address_lookup(addr, symbolsize, offset,
+					 modname, modbuildid, namebuf);
 
 	if (!ret)
 		ret = ftrace_mod_address_lookup(addr, symbolsize,
-- 
2.51.1

[PATCH 4/6] kallsyms/ftrace: Set module buildid in ftrace_mod_address_lookup()

[Petr Mladek]

__sprint_symbol() might access an invalid pointer when
kallsyms_lookup_buildid() returns a symbol found by
ftrace_mod_address_lookup().

The ftrace lookup function must set both @modname and @modbuildid
the same way as module_address_lookup().

Fixes: 9294523e3768 ("module: add printk formats to add module build ID to stacktraces")
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 include/linux/ftrace.h | 6 ++++--
 kernel/kallsyms.c      | 4 ++--
 kernel/trace/ftrace.c  | 5 ++++-
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 7ded7df6e9b5..a003cf1b32d0 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -87,11 +87,13 @@ struct ftrace_hash;
 	defined(CONFIG_DYNAMIC_FTRACE)
 int
 ftrace_mod_address_lookup(unsigned long addr, unsigned long *size,
-		   unsigned long *off, char **modname, char *sym);
+			  unsigned long *off, char **modname,
+			  const unsigned char **modbuildid, char *sym);
 #else
 static inline int
 ftrace_mod_address_lookup(unsigned long addr, unsigned long *size,
-		   unsigned long *off, char **modname, char *sym)
+			  unsigned long *off, char **modname,
+			  const unsigned char **modbuildid, char *sym)
 {
 	return 0;
 }
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index efb12b077220..71868a76e9a1 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -378,8 +378,8 @@ static int kallsyms_lookup_buildid(unsigned long addr,
 					 modname, modbuildid, namebuf);
 
 	if (!ret)
-		ret = ftrace_mod_address_lookup(addr, symbolsize,
-						offset, modname, namebuf);
+		ret = ftrace_mod_address_lookup(addr, symbolsize, offset,
+						modname, modbuildid, namebuf);
 
 	return ret;
 }
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 42bd2ba68a82..11f5096fb60c 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -7678,7 +7678,8 @@ ftrace_func_address_lookup(struct ftrace_mod_map *mod_map,
 
 int
 ftrace_mod_address_lookup(unsigned long addr, unsigned long *size,
-		   unsigned long *off, char **modname, char *sym)
+			  unsigned long *off, char **modname,
+			  const unsigned char **modbuildid, char *sym)
 {
 	struct ftrace_mod_map *mod_map;
 	int ret = 0;
@@ -7690,6 +7691,8 @@ ftrace_mod_address_lookup(unsigned long addr, unsigned long *size,
 		if (ret) {
 			if (modname)
 				*modname = mod_map->mod->name;
+			if (modbuildid)
+				*modbuildid = module_buildid(mod_map->mod);
 			break;
 		}
 	}
-- 
2.51.1

[PATCH 5/6] kallsyms: Clean up @namebuf initialization in kallsyms_lookup_buildid()

[Petr Mladek]

The function kallsyms_lookup_buildid() initializes the given @namebuf
by clearing the first and the last byte. It is not clear why.

The 1st byte makes sense because some callers ignore the return code
and expect that the buffer contains a valid string, for example:

  - function_stat_show()
    - kallsyms_lookup()
      - kallsyms_lookup_buildid()

The initialization of the last byte does not make much sense because it
can later be overwritten. Fortunately, it seems that all called
functions behave correctly:

  -  kallsyms_expand_symbol() explicitly adds the trailing '\0'
     at the end of the function.

  - All *__address_lookup() functions either use the safe strscpy()
    or they do not touch the buffer at all.

Document the reason for clearing the first byte. And remove the useless
initialization of the last byte.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/kallsyms.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 71868a76e9a1..ff7017337535 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -352,7 +352,12 @@ static int kallsyms_lookup_buildid(unsigned long addr,
 {
 	int ret;
 
-	namebuf[KSYM_NAME_LEN - 1] = 0;
+	/*
+	 * kallsyms_lookus() returns pointer to namebuf on success and
+	 * NULL on error. But some callers ignore the return value.
+	 * Instead they expect @namebuf filled either with valid
+	 * or empty string.
+	 */
 	namebuf[0] = 0;
 
 	if (is_ksym_addr(addr)) {
-- 
2.51.1

[PATCH 6/6] kallsyms: Prevent module removal when printing module name and buildid

[Petr Mladek]

kallsyms_lookup_buildid() copies the symbol name into the given buffer
so that it can be safely read anytime later. But it just copies pointers
to mod->name and mod->build_id which might get reused after the related
struct module gets removed.

The lifetime of struct module is synchronized using RCU. Take the rcu
read lock for the entire __sprint_symbol().

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/kallsyms.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index ff7017337535..1fda06b6638c 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -468,6 +468,9 @@ static int __sprint_symbol(char *buffer, unsigned long address,
 	unsigned long offset, size;
 	int len;
 
+	/* Prevent module removal until modname and modbuildid are printed */
+	guard(rcu)();
+
 	address += symbol_offset;
 	len = kallsyms_lookup_buildid(address, &size, &offset, &modname, &buildid,
 				       buffer);
-- 
2.51.1

Re: [PATCH 2/6] kallsyms: Cleanup code for appending the module buildid

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 2442 bytes --]

> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index 1e7635864..9455e3bb0 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -423,6 +423,37 @@ int lookup_symbol_name(unsigned long addr, char *symname)
>  	return lookup_module_symbol_name(addr, symname);
>  }
>
> +#ifdef CONFIG_STACKTRACE_BUILD_ID
> +
> +static int append_buildid(char *buffer,  const char *modname,
> +			  const unsigned char *buildid)
> +{
> +	if (!modname)
> +		return 0;
> +
> +	if (!buildid) {
> +		pr_warn_once("Undefined buildid for the module %s\n", modname);
> +		return 0;
> +	}

When ftrace_mod_address_lookup() succeeds in kallsyms_lookup_buildid(),
it sets *modname but doesn't initialize *modbuildid. This leaves the
buildid variable uninitialized when __sprint_symbol() calls
append_buildid(). Can the check above read uninitialized memory?

Looking at kallsyms_lookup_buildid():
- module_address_lookup() properly initializes both modname and
  modbuildid
- bpf_address_lookup() sets modname=NULL (so append_buildid isn't
  called)
- ftrace_mod_address_lookup() sets modname=mod_map->mod->name but has
  no modbuildid parameter

The commit message mentions wanting to catch when lookup functions don't
handle buildid, but shouldn't kallsyms_lookup_buildid() initialize
*modbuildid=NULL before calling the lookup functions to avoid undefined
behavior?

> +
> +	/* build ID should match length of sprintf */
> +#ifdef CONFIG_MODULES
> +	static_assert(sizeof(typeof_member(struct module, build_id)) == 20);
> +#endif
> +
> +	return sprintf(buffer, " %20phN", buildid);
> +}

[ ... ]

> @@ -445,15 +476,8 @@ static int __sprint_symbol(char *buffer, unsigned long address,
>
>  	if (modname) {
>  		len += sprintf(buffer + len, " [%s", modname);
> -#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
> -		if (add_buildid && buildid) {
> -			/* build ID should match length of sprintf */
> -#if IS_ENABLED(CONFIG_MODULES)
> -			static_assert(sizeof(typeof_member(struct module, build_id)) == 20);
> -#endif
> -			len += sprintf(buffer + len, " %20phN", buildid);
> -		}
> -#endif
> +		if (add_buildid)
> +			len += append_buildid(buffer + len, modname, buildid);
>  		len += sprintf(buffer + len, "]");
>  	}


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/19105560191

Re: [PATCH 4/6] kallsyms/ftrace: Set module buildid in ftrace_mod_address_lookup()

[Steven Rostedt]

On Wed,  5 Nov 2025 15:23:16 +0100
Petr Mladek <pmladek@suse.com> wrote:

> __sprint_symbol() might access an invalid pointer when
> kallsyms_lookup_buildid() returns a symbol found by
> ftrace_mod_address_lookup().
> 
> The ftrace lookup function must set both @modname and @modbuildid
> the same way as module_address_lookup().
> 
> Fixes: 9294523e3768 ("module: add printk formats to add module build ID to stacktraces")
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  include/linux/ftrace.h | 6 ++++--
>  kernel/kallsyms.c      | 4 ++--
>  kernel/trace/ftrace.c  | 5 ++++-
>  3 files changed, 10 insertions(+), 5 deletions(-)

Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve

Re: [PATCH 3/6] kallsyms/bpf: Set module buildid in bpf_address_lookup()

[Alexei Starovoitov]

On Wed, Nov 5, 2025 at 6:24 AM Petr Mladek <pmladek@suse.com> wrote:
>
> Make bpf_address_lookup() compatible with module_address_lookup()
> and clear the pointer to @modbuildid together with @modname.
>
> It is not strictly needed because __sprint_symbol() reads @modbuildid
> only when @modname is set. But better be on the safe side and make
> the API more safe.
>
> Fixes: 9294523e3768 ("module: add printk formats to add module build ID to stacktraces")
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  include/linux/filter.h | 15 +++++++++++----
>  kernel/kallsyms.c      |  4 ++--
>  2 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index f5c859b8131a..b7b95840250a 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -1362,12 +1362,18 @@ struct bpf_prog *bpf_prog_ksym_find(unsigned long addr);
>
>  static inline int
>  bpf_address_lookup(unsigned long addr, unsigned long *size,
> -                  unsigned long *off, char **modname, char *sym)
> +                  unsigned long *off, char **modname,
> +                  const unsigned char **modbuildid, char *sym)
>  {
>         int ret = __bpf_address_lookup(addr, size, off, sym);
>
> -       if (ret && modname)
> -               *modname = NULL;
> +       if (ret) {
> +               if (modname)
> +                       *modname = NULL;
> +               if (modbuildid)
> +                       *modbuildid = NULL;
> +       }
> +
>         return ret;
>  }
>
> @@ -1433,7 +1439,8 @@ static inline struct bpf_prog *bpf_prog_ksym_find(unsigned long addr)
>
>  static inline int
>  bpf_address_lookup(unsigned long addr, unsigned long *size,
> -                  unsigned long *off, char **modname, char *sym)
> +                  unsigned long *off, char **modname,
> +                  const unsigned char **modbuildid, char *sym)
>  {
>         return 0;
>  }
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index 9455e3bb07fc..efb12b077220 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -374,8 +374,8 @@ static int kallsyms_lookup_buildid(unsigned long addr,
>         ret = module_address_lookup(addr, symbolsize, offset,
>                                     modname, modbuildid, namebuf);
>         if (!ret)
> -               ret = bpf_address_lookup(addr, symbolsize,
> -                                        offset, modname, namebuf);
> +               ret = bpf_address_lookup(addr, symbolsize, offset,
> +                                        modname, modbuildid, namebuf);

The initial bpf_address_lookup() 8 years ago was trying
to copy paste args and style of kallsyms_lookup().
It was odd back then. This change is doubling down on the wrong thing.
It's really odd to pass a pointer into bpf_address_lookup()
so it zero initializes it.
bpf ksyms are in the core kernel. They're never in modules.
Just call __bpf_address_lookup() here and remove the wrapper.

Re: [PATCH 1/6] module: Add helper function for reading module_buildid()

[Petr Pavlu]

On 11/5/25 3:23 PM, Petr Mladek wrote:
> Add a helper function for reading the optional "build_id" member
> of struct module. It is going to be used also in
> ftrace_mod_address_lookup().
> 
> Use "#ifdef" instead of "#if IS_ENABLED()" to match the declaration
> of the optional field in struct module.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>
Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>

-- 
Thanks,
Petr

Re: [PATCH 1/6] module: Add helper function for reading module_buildid()

[Daniel Gomez]

On 05/11/2025 15.23, Petr Mladek wrote:
> Add a helper function for reading the optional "build_id" member
> of struct module. It is going to be used also in
> ftrace_mod_address_lookup().
> 
> Use "#ifdef" instead of "#if IS_ENABLED()" to match the declaration
> of the optional field in struct module.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>

Reviewed-by: Daniel Gomez <da.gomez@samsung.com>

Re: [PATCH 3/6] kallsyms/bpf: Set module buildid in bpf_address_lookup()

[Petr Mladek]

On Wed 2025-11-05 18:53:23, Alexei Starovoitov wrote:
> On Wed, Nov 5, 2025 at 6:24 AM Petr Mladek <pmladek@suse.com> wrote:
> >
> > Make bpf_address_lookup() compatible with module_address_lookup()
> > and clear the pointer to @modbuildid together with @modname.
> >
> > It is not strictly needed because __sprint_symbol() reads @modbuildid
> > only when @modname is set. But better be on the safe side and make
> > the API more safe.
> >
> > Fixes: 9294523e3768 ("module: add printk formats to add module build ID to stacktraces")
> > Signed-off-by: Petr Mladek <pmladek@suse.com>
> > ---
> >  include/linux/filter.h | 15 +++++++++++----
> >  kernel/kallsyms.c      |  4 ++--
> >  2 files changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/linux/filter.h b/include/linux/filter.h
> > index f5c859b8131a..b7b95840250a 100644
> > --- a/include/linux/filter.h
> > +++ b/include/linux/filter.h
> > @@ -1362,12 +1362,18 @@ struct bpf_prog *bpf_prog_ksym_find(unsigned long addr);
> >
> >  static inline int
> >  bpf_address_lookup(unsigned long addr, unsigned long *size,
> > -                  unsigned long *off, char **modname, char *sym)
> > +                  unsigned long *off, char **modname,
> > +                  const unsigned char **modbuildid, char *sym)
> >  {
> >         int ret = __bpf_address_lookup(addr, size, off, sym);
> >
> > -       if (ret && modname)
> > -               *modname = NULL;
> > +       if (ret) {
> > +               if (modname)
> > +                       *modname = NULL;
> > +               if (modbuildid)
> > +                       *modbuildid = NULL;
> > +       }
> > +
> >         return ret;
> >  }
> >
> > @@ -1433,7 +1439,8 @@ static inline struct bpf_prog *bpf_prog_ksym_find(unsigned long addr)
> >
> >  static inline int
> >  bpf_address_lookup(unsigned long addr, unsigned long *size,
> > -                  unsigned long *off, char **modname, char *sym)
> > +                  unsigned long *off, char **modname,
> > +                  const unsigned char **modbuildid, char *sym)
> >  {
> >         return 0;
> >  }
> > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> > index 9455e3bb07fc..efb12b077220 100644
> > --- a/kernel/kallsyms.c
> > +++ b/kernel/kallsyms.c
> > @@ -374,8 +374,8 @@ static int kallsyms_lookup_buildid(unsigned long addr,
> >         ret = module_address_lookup(addr, symbolsize, offset,
> >                                     modname, modbuildid, namebuf);
> >         if (!ret)
> > -               ret = bpf_address_lookup(addr, symbolsize,
> > -                                        offset, modname, namebuf);
> > +               ret = bpf_address_lookup(addr, symbolsize, offset,
> > +                                        modname, modbuildid, namebuf);
> 
> The initial bpf_address_lookup() 8 years ago was trying
> to copy paste args and style of kallsyms_lookup().
> It was odd back then. This change is doubling down on the wrong thing.
> It's really odd to pass a pointer into bpf_address_lookup()
> so it zero initializes it.
> bpf ksyms are in the core kernel. They're never in modules.
> Just call __bpf_address_lookup() here and remove the wrapper.

I agree that it is ugly. It would make sense to initialize the
pointers in kallsyms_lookup_buildid and call there
__bpf_address_lookup() variant. Something like:

static int kallsyms_lookup_buildid(unsigned long addr,
			unsigned long *symbolsize,
			unsigned long *offset, char **modname,
			const unsigned char **modbuildid, char *namebuf)
{
	int ret;

	if (modname)
		*modname = NULL;
	if (modbuildid)
		*modbuildid = NULL;
	namebuf[0] = 0;
[...]
	if (!ret)
		ret = __bpf_address_lookup(addr, symbolsize, offset, namebuf);

}

As a result bpf_address_lookup() won't have any caller.
And __bpf_address_lookup() would have two callers.

It would make sense to remove bpf_address_lookup() and
rename __bpf_address_lookup() to bpf_address_lookup().

How does that sound?
Would you prefer to do this in one patch or in two steps, please?

Best Regards,
Petr

Re: [PATCH 2/6] kallsyms: Cleanup code for appending the module buildid

[Petr Mladek]

On Wed 2025-11-05 14:59:53, bot+bpf-ci@kernel.org wrote:
> > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> > index 1e7635864..9455e3bb0 100644
> > --- a/kernel/kallsyms.c
> > +++ b/kernel/kallsyms.c
> > @@ -423,6 +423,37 @@ int lookup_symbol_name(unsigned long addr, char *symname)
> >  	return lookup_module_symbol_name(addr, symname);
> >  }
> >
> > +#ifdef CONFIG_STACKTRACE_BUILD_ID
> > +
> > +static int append_buildid(char *buffer,  const char *modname,
> > +			  const unsigned char *buildid)
> > +{
> > +	if (!modname)
> > +		return 0;
> > +
> > +	if (!buildid) {
> > +		pr_warn_once("Undefined buildid for the module %s\n", modname);
> > +		return 0;
> > +	}
> 
> When ftrace_mod_address_lookup() succeeds in kallsyms_lookup_buildid(),
> it sets *modname but doesn't initialize *modbuildid. This leaves the
> buildid variable uninitialized when __sprint_symbol() calls
> append_buildid().

Just for record. This is a great analyze. This patchset is fixing
this bug in a later patch. ;-)

> Can the check above read uninitialized memory?> 
> Looking at kallsyms_lookup_buildid():
> - module_address_lookup() properly initializes both modname and
>   modbuildid
> - bpf_address_lookup() sets modname=NULL (so append_buildid isn't
>   called)
> - ftrace_mod_address_lookup() sets modname=mod_map->mod->name but has
>   no modbuildid parameter
> 
> The commit message mentions wanting to catch when lookup functions don't
> handle buildid, but shouldn't kallsyms_lookup_buildid() initialize
> *modbuildid=NULL before calling the lookup functions to avoid undefined
> behavior?

It seems that we are going this way, see
https://lore.kernel.org/all/aQ3vWIqG31BgE4YD@pathway.suse.cz/

Best Regards,
Petr

Re: [PATCH 3/6] kallsyms/bpf: Set module buildid in bpf_address_lookup()

[Alexei Starovoitov]

On Fri, Nov 7, 2025 at 5:08 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Wed 2025-11-05 18:53:23, Alexei Starovoitov wrote:
> > On Wed, Nov 5, 2025 at 6:24 AM Petr Mladek <pmladek@suse.com> wrote:
> > >
> > > Make bpf_address_lookup() compatible with module_address_lookup()
> > > and clear the pointer to @modbuildid together with @modname.
> > >
> > > It is not strictly needed because __sprint_symbol() reads @modbuildid
> > > only when @modname is set. But better be on the safe side and make
> > > the API more safe.
> > >
> > > Fixes: 9294523e3768 ("module: add printk formats to add module build ID to stacktraces")
> > > Signed-off-by: Petr Mladek <pmladek@suse.com>
> > > ---
> > >  include/linux/filter.h | 15 +++++++++++----
> > >  kernel/kallsyms.c      |  4 ++--
> > >  2 files changed, 13 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/include/linux/filter.h b/include/linux/filter.h
> > > index f5c859b8131a..b7b95840250a 100644
> > > --- a/include/linux/filter.h
> > > +++ b/include/linux/filter.h
> > > @@ -1362,12 +1362,18 @@ struct bpf_prog *bpf_prog_ksym_find(unsigned long addr);
> > >
> > >  static inline int
> > >  bpf_address_lookup(unsigned long addr, unsigned long *size,
> > > -                  unsigned long *off, char **modname, char *sym)
> > > +                  unsigned long *off, char **modname,
> > > +                  const unsigned char **modbuildid, char *sym)
> > >  {
> > >         int ret = __bpf_address_lookup(addr, size, off, sym);
> > >
> > > -       if (ret && modname)
> > > -               *modname = NULL;
> > > +       if (ret) {
> > > +               if (modname)
> > > +                       *modname = NULL;
> > > +               if (modbuildid)
> > > +                       *modbuildid = NULL;
> > > +       }
> > > +
> > >         return ret;
> > >  }
> > >
> > > @@ -1433,7 +1439,8 @@ static inline struct bpf_prog *bpf_prog_ksym_find(unsigned long addr)
> > >
> > >  static inline int
> > >  bpf_address_lookup(unsigned long addr, unsigned long *size,
> > > -                  unsigned long *off, char **modname, char *sym)
> > > +                  unsigned long *off, char **modname,
> > > +                  const unsigned char **modbuildid, char *sym)
> > >  {
> > >         return 0;
> > >  }
> > > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> > > index 9455e3bb07fc..efb12b077220 100644
> > > --- a/kernel/kallsyms.c
> > > +++ b/kernel/kallsyms.c
> > > @@ -374,8 +374,8 @@ static int kallsyms_lookup_buildid(unsigned long addr,
> > >         ret = module_address_lookup(addr, symbolsize, offset,
> > >                                     modname, modbuildid, namebuf);
> > >         if (!ret)
> > > -               ret = bpf_address_lookup(addr, symbolsize,
> > > -                                        offset, modname, namebuf);
> > > +               ret = bpf_address_lookup(addr, symbolsize, offset,
> > > +                                        modname, modbuildid, namebuf);
> >
> > The initial bpf_address_lookup() 8 years ago was trying
> > to copy paste args and style of kallsyms_lookup().
> > It was odd back then. This change is doubling down on the wrong thing.
> > It's really odd to pass a pointer into bpf_address_lookup()
> > so it zero initializes it.
> > bpf ksyms are in the core kernel. They're never in modules.
> > Just call __bpf_address_lookup() here and remove the wrapper.
>
> I agree that it is ugly. It would make sense to initialize the
> pointers in kallsyms_lookup_buildid and call there
> __bpf_address_lookup() variant. Something like:
>
> static int kallsyms_lookup_buildid(unsigned long addr,
>                         unsigned long *symbolsize,
>                         unsigned long *offset, char **modname,
>                         const unsigned char **modbuildid, char *namebuf)
> {
>         int ret;
>
>         if (modname)
>                 *modname = NULL;
>         if (modbuildid)
>                 *modbuildid = NULL;
>         namebuf[0] = 0;
> [...]
>         if (!ret)
>                 ret = __bpf_address_lookup(addr, symbolsize, offset, namebuf);
>
> }

Yes. Exactly.

> As a result bpf_address_lookup() won't have any caller.
> And __bpf_address_lookup() would have two callers.

yep

> It would make sense to remove bpf_address_lookup() and
> rename __bpf_address_lookup() to bpf_address_lookup().

yep

> How does that sound?
> Would you prefer to do this in one patch or in two steps, please?

Whichever way is easier. I think it's fine to do it in one go,
though it crosses different subsystems.

Re: [PATCH 2/6] kallsyms: Cleanup code for appending the module buildid

[Alexei Starovoitov]

On Fri, Nov 7, 2025 at 5:15 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Wed 2025-11-05 14:59:53, bot+bpf-ci@kernel.org wrote:
> > > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> > > index 1e7635864..9455e3bb0 100644
> > > --- a/kernel/kallsyms.c
> > > +++ b/kernel/kallsyms.c
> > > @@ -423,6 +423,37 @@ int lookup_symbol_name(unsigned long addr, char *symname)
> > >     return lookup_module_symbol_name(addr, symname);
> > >  }
> > >
> > > +#ifdef CONFIG_STACKTRACE_BUILD_ID
> > > +
> > > +static int append_buildid(char *buffer,  const char *modname,
> > > +                     const unsigned char *buildid)
> > > +{
> > > +   if (!modname)
> > > +           return 0;
> > > +
> > > +   if (!buildid) {
> > > +           pr_warn_once("Undefined buildid for the module %s\n", modname);
> > > +           return 0;
> > > +   }
> >
> > When ftrace_mod_address_lookup() succeeds in kallsyms_lookup_buildid(),
> > it sets *modname but doesn't initialize *modbuildid. This leaves the
> > buildid variable uninitialized when __sprint_symbol() calls
> > append_buildid().
>
> Just for record. This is a great analyze. This patchset is fixing
> this bug in a later patch. ;-)

Currently AI analyses one patch at a time and comments on what it
understands the way humans would do if they read and comment
as they go.
Teaching AI to understand the whole series is on todo list.
Chris may comment on how feasible that is.

Re: [PATCH 4/6] kallsyms/ftrace: Set module buildid in ftrace_mod_address_lookup()

[Aaron Tomlin]

On Wed, Nov 05, 2025 at 03:23:16PM +0100, Petr Mladek wrote:
> __sprint_symbol() might access an invalid pointer when
> kallsyms_lookup_buildid() returns a symbol found by
> ftrace_mod_address_lookup().
> 
> The ftrace lookup function must set both @modname and @modbuildid
> the same way as module_address_lookup().
> 
> Fixes: 9294523e3768 ("module: add printk formats to add module build ID to stacktraces")
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  include/linux/ftrace.h | 6 ++++--
>  kernel/kallsyms.c      | 4 ++--
>  kernel/trace/ftrace.c  | 5 ++++-
>  3 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 7ded7df6e9b5..a003cf1b32d0 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -87,11 +87,13 @@ struct ftrace_hash;
>  	defined(CONFIG_DYNAMIC_FTRACE)
>  int
>  ftrace_mod_address_lookup(unsigned long addr, unsigned long *size,
> -		   unsigned long *off, char **modname, char *sym);
> +			  unsigned long *off, char **modname,
> +			  const unsigned char **modbuildid, char *sym);
>  #else
>  static inline int
>  ftrace_mod_address_lookup(unsigned long addr, unsigned long *size,
> -		   unsigned long *off, char **modname, char *sym)
> +			  unsigned long *off, char **modname,
> +			  const unsigned char **modbuildid, char *sym)
>  {
>  	return 0;
>  }
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index efb12b077220..71868a76e9a1 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -378,8 +378,8 @@ static int kallsyms_lookup_buildid(unsigned long addr,
>  					 modname, modbuildid, namebuf);
>  
>  	if (!ret)
> -		ret = ftrace_mod_address_lookup(addr, symbolsize,
> -						offset, modname, namebuf);
> +		ret = ftrace_mod_address_lookup(addr, symbolsize, offset,
> +						modname, modbuildid, namebuf);
>  
>  	return ret;
>  }
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 42bd2ba68a82..11f5096fb60c 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -7678,7 +7678,8 @@ ftrace_func_address_lookup(struct ftrace_mod_map *mod_map,
>  
>  int
>  ftrace_mod_address_lookup(unsigned long addr, unsigned long *size,
> -		   unsigned long *off, char **modname, char *sym)
> +			  unsigned long *off, char **modname,
> +			  const unsigned char **modbuildid, char *sym)
>  {
>  	struct ftrace_mod_map *mod_map;
>  	int ret = 0;
> @@ -7690,6 +7691,8 @@ ftrace_mod_address_lookup(unsigned long addr, unsigned long *size,
>  		if (ret) {
>  			if (modname)
>  				*modname = mod_map->mod->name;
> +			if (modbuildid)
> +				*modbuildid = module_buildid(mod_map->mod);
>  			break;
>  		}
>  	}
> -- 
> 2.51.1
> 
> 

Reviewed-by: Aaron Tomlin <atomlin@atomlin.com>

-- 
Aaron Tomlin

Re: [PATCH 5/6] kallsyms: Clean up @namebuf initialization in kallsyms_lookup_buildid()

[Aaron Tomlin]

On Wed, Nov 05, 2025 at 03:23:17PM +0100, Petr Mladek wrote:
> The function kallsyms_lookup_buildid() initializes the given @namebuf
> by clearing the first and the last byte. It is not clear why.
> 
> The 1st byte makes sense because some callers ignore the return code
> and expect that the buffer contains a valid string, for example:
> 
>   - function_stat_show()
>     - kallsyms_lookup()
>       - kallsyms_lookup_buildid()
> 
> The initialization of the last byte does not make much sense because it
> can later be overwritten. Fortunately, it seems that all called
> functions behave correctly:
> 
>   -  kallsyms_expand_symbol() explicitly adds the trailing '\0'
>      at the end of the function.
> 
>   - All *__address_lookup() functions either use the safe strscpy()
>     or they do not touch the buffer at all.
> 
> Document the reason for clearing the first byte. And remove the useless
> initialization of the last byte.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  kernel/kallsyms.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index 71868a76e9a1..ff7017337535 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -352,7 +352,12 @@ static int kallsyms_lookup_buildid(unsigned long addr,
>  {
>  	int ret;
>  
> -	namebuf[KSYM_NAME_LEN - 1] = 0;
> +	/*
> +	 * kallsyms_lookus() returns pointer to namebuf on success and
> +	 * NULL on error. But some callers ignore the return value.
> +	 * Instead they expect @namebuf filled either with valid
> +	 * or empty string.
> +	 */
>  	namebuf[0] = 0;
>  
>  	if (is_ksym_addr(addr)) {
> -- 
> 2.51.1
> 
> 

Reviewed-by: Aaron Tomlin <atomlin@atomlin.com>

-- 
Aaron Tomlin

Re: [PATCH 6/6] kallsyms: Prevent module removal when printing module name and buildid

[Aaron Tomlin]

On Wed, Nov 05, 2025 at 03:23:18PM +0100, Petr Mladek wrote:
> kallsyms_lookup_buildid() copies the symbol name into the given buffer
> so that it can be safely read anytime later. But it just copies pointers
> to mod->name and mod->build_id which might get reused after the related
> struct module gets removed.
> 
> The lifetime of struct module is synchronized using RCU. Take the rcu
> read lock for the entire __sprint_symbol().
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  kernel/kallsyms.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index ff7017337535..1fda06b6638c 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -468,6 +468,9 @@ static int __sprint_symbol(char *buffer, unsigned long address,
>  	unsigned long offset, size;
>  	int len;
>  
> +	/* Prevent module removal until modname and modbuildid are printed */
> +	guard(rcu)();
> +
>  	address += symbol_offset;
>  	len = kallsyms_lookup_buildid(address, &size, &offset, &modname, &buildid,
>  				       buffer);
> -- 
> 2.51.1
> 
> 

Hi Petr,

If I am not mistaken, this is handled safely within the context of
module_address_lookup() since f01369239293e ("module: Use RCU in
find_kallsyms_symbol()."), no?


Kind regards,
-- 
Aaron Tomlin

Re: [PATCH 6/6] kallsyms: Prevent module removal when printing module name and buildid

[Petr Mladek]

On Fri 2025-11-07 19:36:35, Aaron Tomlin wrote:
> On Wed, Nov 05, 2025 at 03:23:18PM +0100, Petr Mladek wrote:
> > kallsyms_lookup_buildid() copies the symbol name into the given buffer
> > so that it can be safely read anytime later. But it just copies pointers
> > to mod->name and mod->build_id which might get reused after the related
> > struct module gets removed.
> > 
> > The lifetime of struct module is synchronized using RCU. Take the rcu
> > read lock for the entire __sprint_symbol().
> > 
> > Signed-off-by: Petr Mladek <pmladek@suse.com>
> > ---
> >  kernel/kallsyms.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> > index ff7017337535..1fda06b6638c 100644
> > --- a/kernel/kallsyms.c
> > +++ b/kernel/kallsyms.c
> > @@ -468,6 +468,9 @@ static int __sprint_symbol(char *buffer, unsigned long address,
> >  	unsigned long offset, size;
> >  	int len;
> >  
> > +	/* Prevent module removal until modname and modbuildid are printed */
> > +	guard(rcu)();
> > +
> >  	address += symbol_offset;
> >  	len = kallsyms_lookup_buildid(address, &size, &offset, &modname, &buildid,
> >  				       buffer);
> > -- 
> > 2.51.1
> > 
> > 
> 
> Hi Petr,
> 
> If I am not mistaken, this is handled safely within the context of
> module_address_lookup() since f01369239293e ("module: Use RCU in
> find_kallsyms_symbol()."), no?

The above mention commit fixed an API which is looking only for
the symbol name. It seems to be used, for example, in kprobe
or ftrace code.

This patch is fixing another API which is used in vsprintf() for
printing backtraces. It looks for more information: symbol name,
module name, and buildid. It needs its own RCU read protection.

Best Regards,
Petr

Re: [PATCH 6/6] kallsyms: Prevent module removal when printing module name and buildid

[Aaron Tomlin]

On Mon, Nov 10, 2025 at 02:26:52PM +0100, Petr Mladek wrote:
> > Hi Petr,
> > 
> > If I am not mistaken, this is handled safely within the context of
> > module_address_lookup() since f01369239293e ("module: Use RCU in
> > find_kallsyms_symbol()."), no?
> 
> The above mention commit fixed an API which is looking only for
> the symbol name. It seems to be used, for example, in kprobe
> or ftrace code.
> 
> This patch is fixing another API which is used in vsprintf() for
> printing backtraces. It looks for more information: symbol name,
> module name, and buildid. It needs its own RCU read protection.

Hi Petr,

I see and agree.

-- 
Aaron Tomlin

Re: [PATCH 6/6] kallsyms: Prevent module removal when printing module name and buildid

[Aaron Tomlin]

On Wed, Nov 05, 2025 at 03:23:18PM +0100, Petr Mladek wrote:
> kallsyms_lookup_buildid() copies the symbol name into the given buffer
> so that it can be safely read anytime later. But it just copies pointers
> to mod->name and mod->build_id which might get reused after the related
> struct module gets removed.
> 
> The lifetime of struct module is synchronized using RCU. Take the rcu
> read lock for the entire __sprint_symbol().
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  kernel/kallsyms.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index ff7017337535..1fda06b6638c 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -468,6 +468,9 @@ static int __sprint_symbol(char *buffer, unsigned long address,
>  	unsigned long offset, size;
>  	int len;
>  
> +	/* Prevent module removal until modname and modbuildid are printed */
> +	guard(rcu)();
> +
>  	address += symbol_offset;
>  	len = kallsyms_lookup_buildid(address, &size, &offset, &modname, &buildid,
>  				       buffer);
> -- 
> 2.51.1

Looks good to me.

Reviewed-by: Aaron Tomlin <atomlin@atomlin.com>

-- 
Aaron Tomlin