Re: [PATCH v2 09/10] perf annotate: Simplify symbol__disassemble() with dso binary_type

[Namhyung Kim <namhyung@kernel.org>]

On Wed, Oct 07, 2026 at 12:01:39AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] perf: string truncation in symbol__disassemble() due to sizeof(pointer)
> - [High] perf: NULL pointer dereference in LLVM disassembly backend for BPF
> - [High] perf annotate: UAF in dso__get_filename due to missing dso__lock
> --
> 
> commit b52fe9a1d630e0d03a547c6ca49efe1679043beb
> Author: Namhyung Kim <namhyung@kernel.org>
> Subject: perf annotate: Simplify symbol__disassemble() with dso binary_type
> 
> Now it should set the binary type of a DSO properly.  No need to figure
> out a filename suitable for disassembly.
> 
> Just call dso__find_binary_type() and dso__get_filename() for that type.
> 
> > diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> > index 5478c134e7e3b..07a345beb6250 100644
> > --- a/tools/perf/util/disasm.c
> > +++ b/tools/perf/util/disasm.c
> [ ... ]
> > @@ -1566,24 +1488,30 @@ int symbol__disassemble(struct symbol *sym, struct annotate_args *args)
> >  	struct annotation_options *options = args->options;
> >  	struct map *map = args->ms->map;
> >  	struct dso *dso = map__dso(map);
> > -	char symfs_filename[PATH_MAX];
> > +	char *symfs_filename;
> >  	bool delete_extract = false;
> >  	struct kcore_extract kce;
> > +	enum dso_binary_type dbt;
> >  	bool decomp = false;
> > -	int err = dso__disassemble_filename(dso, symfs_filename, sizeof(symfs_filename));
> > +	int err;
> >  
> > -	if (err)
> > -		return err;
> > +	dbt = dso__find_binary_type(dso);
> > +
> > +	if (dbt == DSO_BINARY_TYPE__KALLSYMS)
> > +		return SYMBOL_ANNOTATE_ERRNO__NO_VMLINUX;
> > +
> > +	symfs_filename = dso__get_filename(dso, "", &decomp, dbt);
> 
> [Severity: High]
> Does this call to dso__get_filename() need to be protected by dso__lock()?
> 
> The previous code had an explicit mutex_lock(dso__lock(dso)) around the
> access to dso__nsinfo(dso). dso__get_filename() reads and uses
> dso__nsinfo(dso) via dso__filename_with_chroot(), but lacks internal
> locking.
> 
> If another thread concurrently processes an MMAP event and calls
> dso__set_nsinfo(), could the nsinfo pointer be freed between the check and
> its use, leading to a use-after-free?

Ok, will add the lock.

> 
> > +	if (symfs_filename == NULL && dbt != DSO_BINARY_TYPE__BPF_PROG_INFO &&
> > +	    dbt != DSO_BINARY_TYPE__BPF_IMAGE)
> > +		return SYMBOL_ANNOTATE_ERRNO__COULDNT_DETERMINE_FILE_TYPE;
> 
> [Severity: High]
> When annotating BPF programs, this explicitly allows symfs_filename to be
> NULL. However, could this NULL pointer be passed down to the LLVM
> disassembly backend?
> 
> In the LLVM backend:
> symbol__disassemble_llvm()
>   llvm_name_for_code(..., filename, ...)
> 
> Inside llvm-c-helpers.cpp, symbolizer->symbolizeCode(dso_name, ...)
> implicitly constructs an llvm::StringRef from the dso_name pointer.
> 
> If the pointer is NULL, won't this perform a strlen(NULL) and cause a
> segmentation fault? Similarly, on PowerPC, could symbol__disassemble_raw()
> invoke pr_debug("%s", filename) or open(filename, ...) with the NULL pointer,
> triggering undefined behavior?

Will handle it properly.

> 
> >  
> >  	pr_debug("%s: filename=%s, sym=%s, start=%#" PRIx64 ", end=%#" PRIx64 "\n", __func__,
> > -		 symfs_filename, sym->name, map__unmap_ip(map, sym->start),
> > -		 map__unmap_ip(map, sym->end));
> > +		 symfs_filename ?: dso__long_name(dso), sym->name,
> > +		 map__unmap_ip(map, sym->start), map__unmap_ip(map, sym->end));
> >  
> >  	pr_debug("annotating [%p] %30s : [%p] %30s\n", dso, dso__long_name(dso), sym, sym->name);
> >  
> > -	if (dso__binary_type(dso) == DSO_BINARY_TYPE__NOT_FOUND) {
> > -		return SYMBOL_ANNOTATE_ERRNO__COULDNT_DETERMINE_FILE_TYPE;
> > -	} else if (dso__is_kcore(dso)) {
> > +	if (dso__is_kcore(dso)) {
> >  		kce.addr = map__rip_2objdump(map, sym->start);
> >  		kce.kcore_filename = symfs_filename;
> >  		kce.len = sym->end - sym->start;
> > @@ -1593,17 +1521,6 @@ int symbol__disassemble(struct symbol *sym, struct annotate_args *args)
> >  			delete_extract = true;
> >  			strlcpy(symfs_filename, kce.extract_filename, sizeof(symfs_filename));
> 
> [Severity: High]
> Since symfs_filename is now a char * pointer rather than an array of size
> PATH_MAX, doesn't sizeof(symfs_filename) evaluate to 8 (on 64-bit systems)?
> 
> Will this cause strlcpy() to truncate the kcore extraction path to 7
> characters, leading to ENOENT failures in the downstream disassemblers?

Right, will fix.

Thanks,
Namhyung