1. Patchew
  2. linux
  3. [PATCH 0/6] kallsyms: Prevent invalid access when showing module buildid
  4. View patch

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

Petr Mladek posted 6 patches 1 month, 1 week ago
There is a newer version of this series
[PATCH 2/6] kallsyms: Cleanup code for appending the module buildid
Posted by Petr Mladek 1 month, 1 week ago 
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
Re: [PATCH 2/6] kallsyms: Cleanup code for appending the module buildid
Posted by bot+bpf-ci@kernel.org 1 month, 1 week ago 
> 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 2/6] kallsyms: Cleanup code for appending the module buildid
Posted by Petr Mladek 1 month, 1 week ago 
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 2/6] kallsyms: Cleanup code for appending the module buildid
Posted by Alexei Starovoitov 1 month, 1 week ago 
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.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.