[PATCH 1/5] perf dso: Guard against errno==0 when dso__get_filename() returns NULL

[PATCH 1/5] perf dso: Guard against errno==0 when dso__get_filename() returns NULL

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

__open_dso() computes fd = -errno when dso__get_filename() returns NULL.
Some failure paths in dso__get_filename() (e.g. binary type mismatch)
return NULL without making a syscall, leaving errno at 0 from a prior
successful call.  fd = -0 = 0, which is stdin — subsequent code treats
it as a valid file descriptor.

Fall back to ENOENT when errno is 0, ensuring fd is always negative on
failure.

Fixes: eba5102d2f0b ("perf tools: Add global list of opened dso objects")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/dso.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 2309196d8df3111c..e087a89066bdbc02 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -643,7 +643,7 @@ static int __open_dso(struct dso *dso, struct machine *machine)
 	if (name)
 		fd = do_open(name);
 	else
-		fd = -errno;
+		fd = errno ? -errno : -ENOENT;
 
 	if (decomp)
 		unlink(name);
-- 
2.55.0

Re: [PATCH 1/5] perf dso: Guard against errno==0 when dso__get_filename() returns NULL

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Incomplete fix in `__open_dso()` causes error swallowing in callers by returning negative fd without setting `errno`.

Pre-existing issues:
- [Medium] Data race on adjacent `struct dso` bitfields leads to corruption and a TOCTOU mount namespace leak.
--

--- Patch [1]: [PATCH 1/5] perf dso: Guard against errno==0 when dso__get_filename() returns NULL ---
commit db31953321ccfc3f2f28e5db31230d5fef57578f
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf dso: Guard against errno==0 when dso__get_filename() returns NULL

This commit updates __open_dso() to fall back to ENOENT when
dso__get_filename() returns NULL but leaves errno as 0. This ensures fd
is always negative on failure.

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 2309196d8df31..e087a89066bdb 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -643,7 +643,7 @@ static int __open_dso(struct dso *dso, struct machine *machine)
>  	if (name)
>  		fd = do_open(name);
>  	else
> -		fd = -errno;
> +		fd = errno ? -errno : -ENOENT;

[Severity: Medium]
Does this code need to explicitly set errno to ENOENT as well?

Callers of __open_dso() appear to check if the returned file descriptor is
negative, and then compute their return value using errno. For example, in
file_read():

tools/perf/util/dso.c:file_read() {
    ...
    fd = data_file_open(dso, machine);
    if (fd < 0)
        return -errno;
    ...
}

Since errno is left as 0, callers like file_read() will evaluate -errno
to 0, which incorrectly reports EOF instead of an open failure. Should we
update errno directly?

[Severity: Medium]
This is a pre-existing issue, but can the adjacent bitfields in struct dso
cause a data race that leaks mount namespaces?

Looking at the struct dso definition:

tools/perf/util/dso.h:struct dso {
    ...
    enum dso_binary_type symtab_type:8;
    enum dso_binary_type binary_type:8;
    ...
}

If one thread calls try_to_open_dso() and locklessly modifies binary_type,
while another concurrently modifies symtab_type holding dso__lock(dso),
the concurrent read-modify-write on the shared memory word can corrupt
binary_type.

This corruption appears to break the namespace exit check in open_dso():

tools/perf/util/dso.c:open_dso() {
    ...
    if (dso__binary_type(dso) != DSO_BINARY_TYPE__BUILD_ID_CACHE) {
        mutex_lock(dso__lock(dso));
        nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
        mutex_unlock(dso__lock(dso));
    }
    fd = __open_dso(dso, machine);
    if (dso__binary_type(dso) != DSO_BINARY_TYPE__BUILD_ID_CACHE)
        nsinfo__mountns_exit(&nsc);
    ...
}

If the bitfield gets corrupted during __open_dso(), the exit check might
fail, skipping nsinfo__mountns_exit() and permanently stranding the thread
in the wrong mount namespace. Should access to these bitfields be protected
by dso__lock, or should they be separated to avoid sharing memory bytes?

>  
>  	if (decomp)
>  		unlink(name);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260802142022.154219-1-acme@kernel.org?part=1

Re: [PATCH 1/5] perf dso: Guard against errno==0 when dso__get_filename() returns NULL

[David Laight]

On Sun,  2 Aug 2026 11:20:18 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> __open_dso() computes fd = -errno when dso__get_filename() returns NULL.
> Some failure paths in dso__get_filename() (e.g. binary type mismatch)
> return NULL without making a syscall, leaving errno at 0 from a prior
> successful call.

Except that errno isn't set to zero by successful syscalls.
It is only ever set by ones that fail.
The application may set errno to zero (eg to detect errors from some
library functions).

So paths that don't make syscalls will be returning a 'random' errno.

	David

>  fd = -0 = 0, which is stdin — subsequent code treats
> it as a valid file descriptor.
> 
> Fall back to ENOENT when errno is 0, ensuring fd is always negative on
> failure.
> 
> Fixes: eba5102d2f0b ("perf tools: Add global list of opened dso objects")
> Reported-by: sashiko-bot <sashiko-bot@kernel.org>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Assisted-by: Claude:claude-opus-4.6
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/util/dso.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 2309196d8df3111c..e087a89066bdbc02 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -643,7 +643,7 @@ static int __open_dso(struct dso *dso, struct machine *machine)
>  	if (name)
>  		fd = do_open(name);
>  	else
> -		fd = -errno;
> +		fd = errno ? -errno : -ENOENT;
>  
>  	if (decomp)
>  		unlink(name);

Re: [PATCH 1/5] perf dso: Guard against errno==0 when dso__get_filename() returns NULL

[Arnaldo Carvalho de Melo]

On Sun, Aug 02, 2026 at 02:58:28PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Medium] Incomplete fix in `__open_dso()` causes error swallowing in callers by returning negative fd without setting `errno`.
> 
> Pre-existing issues:
> - [Medium] Data race on adjacent `struct dso` bitfields leads to corruption and a TOCTOU mount namespace leak.
> --
> 
> --- Patch [1]: [PATCH 1/5] perf dso: Guard against errno==0 when dso__get_filename() returns NULL ---
> commit db31953321ccfc3f2f28e5db31230d5fef57578f
> Author: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> perf dso: Guard against errno==0 when dso__get_filename() returns NULL
> 
> This commit updates __open_dso() to fall back to ENOENT when
> dso__get_filename() returns NULL but leaves errno as 0. This ensures fd
> is always negative on failure.
> 
> > diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> > index 2309196d8df31..e087a89066bdb 100644
> > --- a/tools/perf/util/dso.c
> > +++ b/tools/perf/util/dso.c
> > @@ -643,7 +643,7 @@ static int __open_dso(struct dso *dso, struct machine *machine)
> >  	if (name)
> >  		fd = do_open(name);
> >  	else
> > -		fd = -errno;
> > +		fd = errno ? -errno : -ENOENT;
> 
> [Severity: Medium]
> Does this code need to explicitly set errno to ENOENT as well?
> 
> Callers of __open_dso() appear to check if the returned file descriptor is
> negative, and then compute their return value using errno. For example, in
> file_read():
> 
> tools/perf/util/dso.c:file_read() {
>     ...
>     fd = data_file_open(dso, machine);
>     if (fd < 0)
>         return -errno;
>     ...
> }
> 
> Since errno is left as 0, callers like file_read() will evaluate -errno
> to 0, which incorrectly reports EOF instead of an open failure. Should we
> update errno directly?

Agreed, will fix for v2.

- Arnaldo
 
> [Severity: Medium]
> This is a pre-existing issue, but can the adjacent bitfields in struct dso
> cause a data race that leaks mount namespaces?
> 
> Looking at the struct dso definition:
> 
> tools/perf/util/dso.h:struct dso {
>     ...
>     enum dso_binary_type symtab_type:8;
>     enum dso_binary_type binary_type:8;
>     ...
> }
> 
> If one thread calls try_to_open_dso() and locklessly modifies binary_type,
> while another concurrently modifies symtab_type holding dso__lock(dso),
> the concurrent read-modify-write on the shared memory word can corrupt
> binary_type.
> 
> This corruption appears to break the namespace exit check in open_dso():
> 
> tools/perf/util/dso.c:open_dso() {
>     ...
>     if (dso__binary_type(dso) != DSO_BINARY_TYPE__BUILD_ID_CACHE) {
>         mutex_lock(dso__lock(dso));
>         nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
>         mutex_unlock(dso__lock(dso));
>     }
>     fd = __open_dso(dso, machine);
>     if (dso__binary_type(dso) != DSO_BINARY_TYPE__BUILD_ID_CACHE)
>         nsinfo__mountns_exit(&nsc);
>     ...
> }
> 
> If the bitfield gets corrupted during __open_dso(), the exit check might
> fail, skipping nsinfo__mountns_exit() and permanently stranding the thread
> in the wrong mount namespace. Should access to these bitfields be protected
> by dso__lock, or should they be separated to avoid sharing memory bytes?
> 
> >  
> >  	if (decomp)
> >  		unlink(name);
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260802142022.154219-1-acme@kernel.org?part=1

[PATCH 1/5] perf dso: Guard against errno==0 when dso__get_filename() returns NULL

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

__open_dso() computes fd = -errno when dso__get_filename() returns NULL.
Some failure paths in dso__get_filename() (e.g. binary type mismatch)
return NULL without making a syscall, leaving errno at 0 from a prior
successful call.  fd = -0 = 0, which is stdin — subsequent code treats
it as a valid file descriptor.

Fall back to ENOENT when errno is 0, ensuring fd is always negative on
failure.

Fixes: eba5102d2f0b ("perf tools: Add global list of opened dso objects")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/dso.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 2309196d8df3111c..fcd4b462992be6f2 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -640,10 +640,13 @@ static int __open_dso(struct dso *dso, struct machine *machine)
 	mutex_lock(dso__lock(dso));
 
 	name = dso__get_filename(dso, machine ? machine->root_dir : "", &decomp);
-	if (name)
+	if (name) {
 		fd = do_open(name);
-	else
+	} else {
+		if (errno == 0)
+			errno = ENOENT;
 		fd = -errno;
+	}
 
 	if (decomp)
 		unlink(name);
-- 
2.55.0

[PATCH 1/5] perf dso: Guard against errno==0 when dso__get_filename() returns NULL

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

__open_dso() computes fd = -errno when dso__get_filename() returns NULL.
Some failure paths in dso__get_filename() (e.g. binary type mismatch)
return NULL without making a syscall, leaving errno at 0 from a prior
successful call.  fd = -0 = 0, which is stdin — subsequent code treats
it as a valid file descriptor.

Fall back to ENOENT when errno is 0, ensuring fd is always negative on
failure.

Fixes: eba5102d2f0b ("perf tools: Add global list of opened dso objects")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/dso.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 2309196d8df3111c..fcd4b462992be6f2 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -640,10 +640,13 @@ static int __open_dso(struct dso *dso, struct machine *machine)
 	mutex_lock(dso__lock(dso));
 
 	name = dso__get_filename(dso, machine ? machine->root_dir : "", &decomp);
-	if (name)
+	if (name) {
 		fd = do_open(name);
-	else
+	} else {
+		if (errno == 0)
+			errno = ENOENT;
 		fd = -errno;
+	}
 
 	if (decomp)
 		unlink(name);
-- 
2.55.0

[PATCHES v4 0/5] perf DSO hardening series

[Arnaldo Carvalho de Melo]

Hi,

	Please consider merging,

Five hardening fixes for the perf DSO data handling code path, all found
by the sashiko-bot AI reviewer:

  - __open_dso() can return a file descriptor of 0 (stdin) when
    dso__get_filename() fails without setting errno, so fd = -errno = 0
    looked like a successfully opened DSO;

  - dso__decompress_kmodule_path() calls close(fd) with fd = -1 when
    decompression fails or the DSO is not compressed, clobbering errno
    with EBADF on the error path;

  - file_read() and file_size() read the stale global errno after
    try_to_open_dso() fails, when by then errno has been through
    mutex_lock(), nsinfo__mountns_enter() and several open() attempts;

  - dso_cache__memcpy() underflows when cache->size is smaller than the
    RB tree offset (short pread), computing a huge memcpy length;

  - dso__read_symbol() asserts on an input-dependent condition instead
    of doing a runtime check, keeping the BPF metadata safe from crafted
    input.

Best regards,

- Arnaldo

What changed from v3 (41ed39bf2dac1d80):

  PATCH 3/5:
  - Added assert(ret < 0) after ret = dso__data(dso)->fd in both
    file_read() and file_size() to verify the comment that fd is always
    negative, never 0 [Ian Rogers review nit].

  - All 5 patches carry Reviewed-by: Ian Rogers.

What changed from v2 (2f944ea022b6429f):

  PATCH 3/5:
  - file_size() now also uses the stored fd error instead of the stale
    global errno on open failure; the commit was retitled to cover both
    file_read() and file_size() [sashiko-bot review of PATCH 3/5].
  - Comment corrected: on failure dso__data(dso)->fd is always
    negative — -errno from __open_dso() or -1 from do_open() — never 0.

  PATCH 4/5:
  - Clarified that returning 0 for an offset past a short-read chunk
    is EOF semantics — for a regular file a short pread only happens at
    end-of-file, so 0 is what a direct pread() at that offset would
    return — not a cache-miss that triggers a re-read; comment and
    commit message updated [sashiko-bot review of PATCH 4/5].

  - The lockless dso cache RB tree lookup vs. concurrent insert
    question raised in the same review is pre-existing; it is recorded
    in tools/perf/TODO.hardening (item 175) for follow-up work, with
    no change to this series.

What changed from v1 (20260802142022.154219-1-acme@kernel.org):

  PATCH 1/5:
  - __open_dso() now sets errno = ENOENT directly when
    dso__get_filename() fails with errno == 0, instead of only computing
    fd = -ENOENT.  Callers that check errno after a negative fd (e.g.
    file_read() returning -errno) no longer get 0/EOF for an open
    failure [sashiko-bot review of PATCH 1/5].

  - Rebased onto the current perf-tools-next head (bf10e6ee2ac3034c).

This series was developed with assistance from Claude (claude-opus-4.6)
and Opencode (deepseek-v4-flash-free) for code analysis, patch
generation, and commit message composition.  All changes were validated
by the maintainer.

Arnaldo Carvalho de Melo (5):
  perf dso: Guard against errno==0 when dso__get_filename() returns NULL
  perf dso: Guard close() against invalid fd in
    dso__decompress_kmodule_path()
  perf dso: Use stored fd error instead of stale errno in file_read()
    and file_size()
  perf dso: Guard against cache underflow on short reads in
    dso_cache__memcpy()
  perf dso: Replace assert with runtime check in dso__read_symbol()

 tools/perf/util/dso.c | 41 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

base-commit: bf10e6ee2ac3034c9068e03eed418fd16961984e
v3-head: 41ed39bf2dac1d80
v2-head: 2f944ea022b6429f
v1-head: f6cb9e46c7b8949b

[PATCH 1/5] perf dso: Guard against errno==0 when dso__get_filename() returns NULL

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

__open_dso() computes fd = -errno when dso__get_filename() returns NULL.
Some failure paths in dso__get_filename() (e.g. binary type mismatch)
return NULL without making a syscall, leaving errno at 0 from a prior
successful call.  fd = -0 = 0, which is stdin — subsequent code treats
it as a valid file descriptor.

Fall back to ENOENT when errno is 0, ensuring fd is always negative on
failure.

Fixes: eba5102d2f0b ("perf tools: Add global list of opened dso objects")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Reviewed-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/dso.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 2309196d8df3111c..fcd4b462992be6f2 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -640,10 +640,13 @@ static int __open_dso(struct dso *dso, struct machine *machine)
 	mutex_lock(dso__lock(dso));
 
 	name = dso__get_filename(dso, machine ? machine->root_dir : "", &decomp);
-	if (name)
+	if (name) {
 		fd = do_open(name);
-	else
+	} else {
+		if (errno == 0)
+			errno = ENOENT;
 		fd = -errno;
+	}
 
 	if (decomp)
 		unlink(name);
-- 
2.55.0

[PATCH 2/5] perf dso: Guard close() against invalid fd in dso__decompress_kmodule_path()

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

dso__decompress_kmodule_path() unconditionally calls close(fd) on the
return value of decompress_kmodule().  When decompression fails or the
DSO is not compressed, decompress_kmodule() returns -1.  close(-1)
fails with EBADF and clobbers errno, which callers up the chain
(dso__get_filename → __open_dso) depend on for error propagation.

Guard the close() call with fd >= 0 so only valid file descriptors are
closed.

Fixes: 42b3fa670825 ("perf tools: Introduce dso__decompress_kmodule_{fd,path}")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Reviewed-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/dso.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index fcd4b462992be6f2..124193453675ca18 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -395,7 +395,9 @@ int dso__decompress_kmodule_path(struct dso *dso, const char *name,
 {
 	int fd = decompress_kmodule(dso, name, pathname, len);
 
-	close(fd);
+	/* decompress_kmodule() returns -1 on failure, don't close(-1) */
+	if (fd >= 0)
+		close(fd);
 	return fd >= 0 ? 0 : -1;
 }
 
-- 
2.55.0

[PATCH 3/5] perf dso: Use stored fd error instead of stale errno in file_read() and file_size()

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

file_read() and file_size() use ret = -errno when
dso__data(dso)->fd is negative after try_to_open_dso() fails.  By this
point errno has been through mutex_lock(), nsinfo__mountns_enter(), and
multiple open() attempts inside try_to_open_dso() — it no longer
reflects the actual open failure.  If errno happens to be 0, ret = 0
looks like EOF rather than an error, and file_size() callers like
dso__data_size() would then report a zero-sized file instead of
failing.

dso__data(dso)->fd is always negative on failure — -errno from
__open_dso() when no filename could be built (e.g. -EINVAL, -ENOENT),
or -1 when do_open() itself failed — and never 0, so use it directly
instead of reading the stale global errno.

Fixes: 33bdedcea2d7 ("perf tools: Protect dso cache fd with a mutex")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Reviewed-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/dso.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 124193453675ca18..c7fb9e1d07f14f17 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -1029,7 +1029,9 @@ static ssize_t file_read(struct dso *dso, struct machine *machine,
 
 	if (dso__data(dso)->fd < 0) {
 		dso__data(dso)->status = DSO_DATA_STATUS_ERROR;
-		ret = -errno;
+		/* fd is always negative here: -errno or -1, never 0 */
+		ret = dso__data(dso)->fd;
+		assert(ret < 0);
 		goto out;
 	}
 
@@ -1151,8 +1153,10 @@ static int file_size(struct dso *dso, struct machine *machine)
 	try_to_open_dso(dso, machine);
 
 	if (dso__data(dso)->fd < 0) {
-		ret = -errno;
 		dso__data(dso)->status = DSO_DATA_STATUS_ERROR;
+		/* fd is always negative here: -errno or -1, never 0 */
+		ret = dso__data(dso)->fd;
+		assert(ret < 0);
 		goto out;
 	}
 
-- 
2.55.0

[PATCH 4/5] perf dso: Guard against cache underflow on short reads in dso_cache__memcpy()

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

dso_cache__memcpy() computes cache_offset = offset - cache->offset,
then cache_size = min(cache->size - cache_offset, size).  The RB tree
lookup in __dso_cache__find() matches using the full
DSO__DATA_CACHE_SIZE window, but cache->size reflects the actual pread
return value from dso_cache__populate().

A short pread (e.g. near end-of-file) makes cache->size smaller than
DSO__DATA_CACHE_SIZE.  If a subsequent access targets an offset past
cache->offset + cache->size but within the DSO__DATA_CACHE_SIZE
window, the cache entry is found but cache_offset exceeds cache->size.
Since both are u64, the subtraction cache->size - cache_offset wraps
to a large value, min() selects the caller's size, and memcpy reads
out of bounds.

Return 0 for an offset past the valid cached data.  For a regular
file a short pread only happens at end-of-file, so 0 is what a direct
pread() at that offset would return: cached_io() stops its read loop
as on EOF.  Re-reading from the backing file would not help — a
second pread at the same offset returns the same short count.

Fixes: 366df72657e0 ("perf dso: Refactor dso_cache__read()")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Reviewed-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/dso.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index c7fb9e1d07f14f17..03e7f89d5465c91f 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -1005,7 +1005,20 @@ static ssize_t dso_cache__memcpy(struct dso_cache *cache, u64 offset, u8 *data,
 				 u64 size, bool out)
 {
 	u64 cache_offset = offset - cache->offset;
-	u64 cache_size   = min(cache->size - cache_offset, size);
+	u64 cache_size;
+
+	/*
+	 * The RB tree matches using DSO__DATA_CACHE_SIZE, but a short
+	 * pread may leave cache->size smaller.  For a regular file a
+	 * short pread only happens at end-of-file, so an offset past
+	 * the valid data is EOF: return 0, matching what a direct
+	 * pread() at that offset would return, and cached_io() then
+	 * stops its read loop.
+	 */
+	if (cache_offset >= cache->size)
+		return 0;
+
+	cache_size = min(cache->size - cache_offset, size);
 
 	if (out)
 		memcpy(data, cache->data + cache_offset, cache_size);
-- 
2.55.0

[PATCH 5/5] perf dso: Replace assert with runtime check in dso__read_symbol()

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

dso__read_symbol() asserts that len <= jited_prog_len, where len comes
from sym->end - sym->start (parsed from PERF_RECORD_KSYMBOL in
perf.data).  Both values originate from untrusted file input.

With NDEBUG (production builds), the assert is compiled out, allowing
an out-of-bounds heap read when the BPF program buffer is accessed.
Without NDEBUG, a crafted perf.data crashes perf with an assertion
failure.

Replace the assert with a runtime bounds check that returns NULL with
an appropriate error code, matching the existing error handling
pattern in this function.

Fixes: aa04707f507e ("perf dso: Support BPF programs in dso__read_symbol()")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Ian Rogers <irogers@google.com>
Cc: Song Liu <song@kernel.org>
Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Reviewed-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/dso.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 03e7f89d5465c91f..68cd90b5605f8c50 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -2033,7 +2033,12 @@ const u8 *dso__read_symbol(struct dso *dso, const char *symfs_filename,
 			errno = SYMBOL_ANNOTATE_ERRNO__BPF_MISSING_BTF;
 			return NULL;
 		}
-		assert(len <= info_linear->info.jited_prog_len);
+		if (len > info_linear->info.jited_prog_len) {
+			pr_debug("BPF symbol length %zu exceeds jited_prog_len %u\n",
+				 len, info_linear->info.jited_prog_len);
+			errno = SYMBOL_ANNOTATE_ERRNO__BPF_MISSING_BTF;
+			return NULL;
+		}
 		*out_buf_len = len;
 		return (const u8 *)(uintptr_t)(info_linear->info.jited_prog_insns);
 #else
-- 
2.55.0

Re: [PATCH 2/5] perf dso: Guard close() against invalid fd in dso__decompress_kmodule_path()

[sashiko-bot]

> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> dso__decompress_kmodule_path() unconditionally calls close(fd) on the
> return value of decompress_kmodule().  When decompression fails or the
> DSO is not compressed, decompress_kmodule() returns -1.  close(-1)
> fails with EBADF and clobbers errno, which callers up the chain
> (dso__get_filename → __open_dso) depend on for error propagation.
> 
> Guard the close() call with fd >= 0 so only valid file descriptors are
> closed.
> 
> Fixes: 42b3fa670825 ("perf tools: Introduce dso__decompress_kmodule_{fd,path}")
> Reported-by: sashiko-bot <sashiko-bot@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Assisted-by: Claude:claude-opus-4.6
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813004927.16738-1-acme@kernel.org?part=2

Re: [PATCH 4/5] perf dso: Guard against cache underflow on short reads in dso_cache__memcpy()

[sashiko-bot]

> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> dso_cache__memcpy() computes cache_offset = offset - cache->offset,
> then cache_size = min(cache->size - cache_offset, size).  The RB tree
> lookup in __dso_cache__find() matches using the full
> DSO__DATA_CACHE_SIZE window, but cache->size reflects the actual pread
> return value from dso_cache__populate().
> 
> A short pread (e.g. near end-of-file) makes cache->size smaller than
> DSO__DATA_CACHE_SIZE.  If a subsequent access targets an offset past
> cache->offset + cache->size but within the DSO__DATA_CACHE_SIZE
> window, the cache entry is found but cache_offset exceeds cache->size.
> Since both are u64, the subtraction cache->size - cache_offset wraps
> to a large value, min() selects the caller's size, and memcpy reads
> out of bounds.
> 
> Return 0 for an offset past the valid cached data.  For a regular
> file a short pread only happens at end-of-file, so 0 is what a direct
> pread() at that offset would return: cached_io() stops its read loop
> as on EOF.  Re-reading from the backing file would not help — a
> second pread at the same offset returns the same short count.
> 
> Fixes: 366df72657e0 ("perf dso: Refactor dso_cache__read()")
> Reported-by: sashiko-bot <sashiko-bot@kernel.org>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Assisted-by: Claude:claude-opus-4.6
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813004927.16738-1-acme@kernel.org?part=4

Re: [PATCH 5/5] perf dso: Replace assert with runtime check in dso__read_symbol()

[sashiko-bot]

> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> dso__read_symbol() asserts that len <= jited_prog_len, where len comes
> from sym->end - sym->start (parsed from PERF_RECORD_KSYMBOL in
> perf.data).  Both values originate from untrusted file input.
> 
> With NDEBUG (production builds), the assert is compiled out, allowing
> an out-of-bounds heap read when the BPF program buffer is accessed.
> Without NDEBUG, a crafted perf.data crashes perf with an assertion
> failure.
> 
> Replace the assert with a runtime bounds check that returns NULL with
> an appropriate error code, matching the existing error handling
> pattern in this function.
> 
> Fixes: aa04707f507e ("perf dso: Support BPF programs in dso__read_symbol()")
> Reported-by: sashiko-bot <sashiko-bot@kernel.org>
> Cc: Ian Rogers <irogers@google.com>
> Cc: Song Liu <song@kernel.org>
> Assisted-by: Claude:claude-opus-4.6
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813004927.16738-1-acme@kernel.org?part=5

Re: [PATCH 3/5] perf dso: Use stored fd error instead of stale errno in file_read() and file_size()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Missing explicit inclusion of `<assert.h>` for the newly added `assert()` calls, violating musl libc compatibility guidelines.
--

--- Patch [3]: [PATCH 3/5] perf dso: Use stored fd error instead of stale errno in file_read() and file_size() ---
commit 9359e4a3d3517acb5c94e232e0f7e3c94b3a7824
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf dso: Use stored fd error instead of stale errno in file_read() and file_size()

This commit updates file_read() and file_size() to use the negative fd value
as the error code directly instead of relying on errno. This prevents issues 
where a stale errno could incorrectly appear as an EOF rather than an open 
failure.

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 124193453675c..c7fb9e1d07f14 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -1029,7 +1029,9 @@ static ssize_t file_read(struct dso *dso, struct machine *machine,
>  
>  	if (dso__data(dso)->fd < 0) {
>  		dso__data(dso)->status = DSO_DATA_STATUS_ERROR;
> -		ret = -errno;
> +		/* fd is always negative here: -errno or -1, never 0 */
> +		ret = dso__data(dso)->fd;
> +		assert(ret < 0);

[Severity: Low]
Does this missing include break musl libc compatibility?

The perf subsystem guidelines require explicit inclusion of POSIX standard
headers for any libc macros used. Could you add the <assert.h> header at the
top of tools/perf/util/dso.c to ensure this compiles properly under musl?

>  		goto out;
>  	}

[ ... ]

> @@ -1151,8 +1153,10 @@ static int file_size(struct dso *dso, struct machine *machine)
>  	try_to_open_dso(dso, machine);
>  
>  	if (dso__data(dso)->fd < 0) {
> -		ret = -errno;
>  		dso__data(dso)->status = DSO_DATA_STATUS_ERROR;
> +		/* fd is always negative here: -errno or -1, never 0 */
> +		ret = dso__data(dso)->fd;
> +		assert(ret < 0);

[Severity: Low]
Is it possible to ensure <assert.h> is included for this usage as well? 

>  		goto out;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813004927.16738-1-acme@kernel.org?part=3

Re: [PATCH 1/5] perf dso: Guard against errno==0 when dso__get_filename() returns NULL

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Stale `errno` usage in `dso__get_filename()` and `__open_dso()` creates brittle logic, incorrect error propagation, and loop pollution.

Pre-existing issues:
- [High] `file_read()` attempts to extract the failure reason using `-errno` after several cleanup and namespace functions have clobbered the `errno` value.
- [High] Strict AB-BA deadlock between `_dso__data_open_lock` and `dso__lock` when `REFCNT_CHECKING` is enabled and the `EMFILE` limit is hit.
--

--- Patch [1]: [PATCH 1/5] perf dso: Guard against errno==0 when dso__get_filename() returns NULL ---
commit b3606e9e10d4ce3d99991eea12437267b852da75
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf dso: Guard against errno==0 when dso__get_filename() returns NULL

This commit fixes an issue where __open_dso() could incorrectly return 0
(stdin) when dso__get_filename() returns NULL without setting errno.
It forces errno to ENOENT if it is 0 upon failure, ensuring a negative
file descriptor is returned.

> --- tools/perf/util/dso.c
> +++ tools/perf/util/dso.c
> @@ -640,10 +640,13 @@ static int __open_dso(struct dso *dso, struct machine *machine)
>  	mutex_lock(dso__lock(dso));

[Severity: High]
This is a pre-existing issue, but can this lock ordering cause an AB-BA
deadlock when REFCNT_CHECKING is enabled?

If do_open() hits the EMFILE limit, close_first_dso() calls
dso__list_del(). Under REFCNT_CHECKING, dso__list_del() drops and
reacquires dso__data_open_lock() to satisfy constraints:

tools/perf/util/dso.c:dso__list_del() {
#ifdef REFCNT_CHECKING
    mutex_unlock(dso__data_open_lock());
    dso__put(dso__data(dso)->dso);
    mutex_lock(dso__data_open_lock());
#endif
}

Since dso__lock(dso) is held here in __open_dso(), if another thread
acquires dso__data_open_lock() and then attempts __open_dso() on the
same dso, will they deadlock waiting on each other?

>  
>  	name = dso__get_filename(dso, machine ? machine->root_dir : "", &decomp);
> -	if (name)
> +	if (name) {
>  		fd = do_open(name);
> -	else
> +	} else {
> +		if (errno == 0)
> +			errno = ENOENT;

[Severity: High]
Does setting errno to ENOENT here pollute the thread-local state for
subsequent loop iterations?

If a missing DSO file triggers a fallback loop like try_to_open_dso(),
and the next iteration evaluates a directory instead of an ELF file,
is_regular_file() will return false.

Since stat() succeeds on a directory, it won't change errno. The
stale ENOENT from here means dso__get_filename() evaluates
if (errno != ENOENT || ...) as false.

Will this cause the code to incorrectly bypass the error path,
proceed to dso__filename_with_chroot(), and successfully open a
directory instead of an ELF file?

> +		fd = -errno;
> +	}

[Severity: High]
This is a pre-existing issue, but does relying on -errno here and in
file_read() lead to incorrect error propagation?

When try_to_open_dso() fails, file_read() attempts to extract the failure
reason:

tools/perf/util/dso.c:file_read() {
    ...
    if (dso__data(dso)->fd < 0) {
        dso__data(dso)->status = DSO_DATA_STATUS_ERROR;
        ret = -errno;
        goto out;
    }
}

However, between the actual failure and this check, cleanup functions like
unlink(), mutex_unlock(), and nsinfo__mountns_exit() execute.

Will these calls clobber errno, causing file_read() to return an arbitrary
error code or 0, silently failing to report the true failure reason?

>  
>  	if (decomp)
>  		unlink(name);
>  	mutex_unlock(dso__lock(dso));
>  	free(name);
>  	return fd;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813004927.16738-1-acme@kernel.org?part=1