linux-kernel - Re: [PATCH 4/4] perf symbols: Fix ownership of string in dso__load

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Wed, 8 May 2024 15:14:06 -0700
From: Ian Rogers <irogers@...gle.com>
To: James Clark <james.clark@....com>
Cc: linux-perf-users@...r.kernel.org, atrajeev@...ux.vnet.ibm.com, 
	Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, "Liang, Kan" <kan.liang@...ux.intel.com>, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] perf symbols: Fix ownership of string in dso__load_vmlinux()

On Tue, May 7, 2024 at 7:13 AM James Clark <james.clark@....com> wrote:
>
> The linked commit updated dso__load_vmlinux() to call
> dso__set_long_name() before loading the symbols. Loading the symbols may
> not succeed but dso__set_long_name() takes ownership of the string. The
> two callers of this function free the string themselves on failure
> cases, resulting in the following error:
>
>   $ perf record -- ls
>   $ perf report
>
>   free(): double free detected in tcache 2
>
> Fix it by always taking ownership of the string, even on failure. This
> means the string is either freed at the very first early exit condition,
> or later when the dso is deleted or the long name is replaced. Now no
> special return value is needed to signify that the caller needs to
> free the string.
>
> Fixes: e59fea47f83e ("perf symbols: Fix DSO kernel load and symbol process to correctly map DSO to its long_name, type and adjust_symbols")
> Signed-off-by: James Clark <james.clark@....com>

Reviewed-by: Ian Rogers <irogers@...gle.com>

Thanks,
Ian

> ---
>  tools/perf/util/symbol.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index e98dfe766da3..6a0900dcdfd3 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1977,6 +1977,10 @@ int dso__load(struct dso *dso, struct map *map)
>         return ret;
>  }
>
> +/*
> + * Always takes ownership of vmlinux when vmlinux_allocated == true, even if
> + * it returns an error.
> + */
>  int dso__load_vmlinux(struct dso *dso, struct map *map,
>                       const char *vmlinux, bool vmlinux_allocated)
>  {
> @@ -1995,8 +1999,11 @@ int dso__load_vmlinux(struct dso *dso, struct map *map,
>         else
>                 symtab_type = DSO_BINARY_TYPE__VMLINUX;
>
> -       if (symsrc__init(&ss, dso, symfs_vmlinux, symtab_type))
> +       if (symsrc__init(&ss, dso, symfs_vmlinux, symtab_type)) {
> +               if (vmlinux_allocated)
> +                       free((char *) vmlinux);
>                 return -1;
> +       }
>
>         /*
>          * dso__load_sym() may copy 'dso' which will result in the copies having
> @@ -2039,7 +2046,6 @@ int dso__load_vmlinux_path(struct dso *dso, struct map *map)
>                 err = dso__load_vmlinux(dso, map, filename, true);
>                 if (err > 0)
>                         goto out;
> -               free(filename);
>         }
>  out:
>         return err;
> @@ -2191,7 +2197,6 @@ static int dso__load_kernel_sym(struct dso *dso, struct map *map)
>                 err = dso__load_vmlinux(dso, map, filename, true);
>                 if (err > 0)
>                         return err;
> -               free(filename);
>         }
>
>         if (!symbol_conf.ignore_vmlinux && vmlinux_path != NULL) {
> --
> 2.34.1
>