* [PATCH] perf probe: Fix null pointer dereference in convert_variable_location()
@ 2021-06-01 9:27 Li Huafei
2021-06-01 13:23 ` Arnaldo Carvalho de Melo
2021-06-01 23:45 ` Masami Hiramatsu
0 siblings, 2 replies; 3+ messages in thread
From: Li Huafei @ 2021-06-01 9:27 UTC (permalink / raw)
To: acme, mhiramat
Cc: namhyung, peterz, mingo, mark.rutland, alexander.shishkin, jolsa,
srikar, fche, Jianlin.Lv, linux-perf-users, linux-kernel,
yangjihong1, zhangjinhao2, lihuafei1
If we just check whether the variable can be converted, 'tvar' should be
a null pointer. However, the null pointer check is missing in the
'Constant value' execution path.
The following cases can trigger this problem:
$ cat test.c
#include <stdio.h>
void main(void)
{
int a;
const int b = 1;
asm volatile("mov %1, %0" : "=r"(a): "i"(b));
printf("a: %d\n", a);
}
$ gcc test.c -o test -O -g
$ sudo ./perf probe -x ./test -L "main"
<main@/home/lhf/test.c:0>
0 void main(void)
{
2 int a;
const int b = 1;
asm volatile("mov %1, %0" : "=r"(a): "i"(b));
6 printf("a: %d\n", a);
}
$ sudo ./perf probe -x ./test -V "main:6"
Segmentation fault
The check on 'tvar' is added. If 'tavr' is a null pointer, we return 0
to indicate that the variable can be converted. Now, we can successfully
show the variables that can be accessed.
$ sudo ./perf probe -x ./test -V "main:6"
Available variables at main:6
@<main+13>
char* __fmt
int a
int b
However, the variable 'b' cannot be tracked.
$ sudo ./perf probe -x ./test -D "main:6 b"
Failed to find the location of the 'b' variable at this address.
Perhaps it has been optimized out.
Use -V with the --range option to show 'b' location range.
Error: Failed to add events.
This is because __die_find_variable_cb() did not successfully match
variable 'b', which has the DW_AT_const_value attribute instead of
DW_AT_location. We added support for DW_AT_const_value in
__die_find_variable_cb(). With this modification, we can successfully
track the variable 'b'.
$ sudo ./perf probe -x ./test -D "main:6 b"
p:probe_test/main_L6 /home/lhf/test:0x1156 b=\1:s32
Fixes: 66f69b219716 ("perf probe: Support DW_AT_const_value constant value")
Signed-off-by: Li Huafei <lihuafei1@huawei.com>
---
tools/perf/util/dwarf-aux.c | 8 ++++++--
tools/perf/util/probe-finder.c | 3 +++
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index b2f4920e19a6..7d2ba8419b0c 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -975,9 +975,13 @@ static int __die_find_variable_cb(Dwarf_Die *die_mem, void *data)
if ((tag == DW_TAG_formal_parameter ||
tag == DW_TAG_variable) &&
die_compare_name(die_mem, fvp->name) &&
- /* Does the DIE have location information or external instance? */
+ /*
+ * Does the DIE have location information or const value
+ * or external instance?
+ */
(dwarf_attr(die_mem, DW_AT_external, &attr) ||
- dwarf_attr(die_mem, DW_AT_location, &attr)))
+ dwarf_attr(die_mem, DW_AT_location, &attr) ||
+ dwarf_attr(die_mem, DW_AT_const_value, &attr)))
return DIE_FIND_CB_END;
if (dwarf_haspc(die_mem, fvp->addr))
return DIE_FIND_CB_CONTINUE;
diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index 866f2d514d72..b029c29ce227 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -190,6 +190,9 @@ static int convert_variable_location(Dwarf_Die *vr_die, Dwarf_Addr addr,
immediate_value_is_supported()) {
Dwarf_Sword snum;
+ if (!tvar)
+ return 0;
+
dwarf_formsdata(&attr, &snum);
ret = asprintf(&tvar->value, "\\%ld", (long)snum);
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] perf probe: Fix null pointer dereference in convert_variable_location()
2021-06-01 9:27 [PATCH] perf probe: Fix null pointer dereference in convert_variable_location() Li Huafei
@ 2021-06-01 13:23 ` Arnaldo Carvalho de Melo
2021-06-01 23:45 ` Masami Hiramatsu
1 sibling, 0 replies; 3+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-06-01 13:23 UTC (permalink / raw)
To: Li Huafei
Cc: mhiramat, namhyung, peterz, mingo, mark.rutland,
alexander.shishkin, jolsa, srikar, fche, Jianlin.Lv,
linux-perf-users, linux-kernel, yangjihong1, zhangjinhao2
Em Tue, Jun 01, 2021 at 05:27:50PM +0800, Li Huafei escreveu:
> If we just check whether the variable can be converted, 'tvar' should be
> a null pointer. However, the null pointer check is missing in the
> 'Constant value' execution path.
>
> The following cases can trigger this problem:
>
> $ cat test.c
> #include <stdio.h>
Thanks for providing the detailed analysis and test steps, I've
reproduced the problem before the patch and it now works with your
patch:
[acme@five tmp]$ sudo perf probe -x ./test main:6
Added new event:
probe_test:main_L6 (on main:6 in /tmp/test)
You can now use it in all perf tools, such as:
perf record -e probe_test:main_L6 -aR sleep 1
[acme@five tmp]$
Thanks, applied.
- Arnaldo
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] perf probe: Fix null pointer dereference in convert_variable_location()
2021-06-01 9:27 [PATCH] perf probe: Fix null pointer dereference in convert_variable_location() Li Huafei
2021-06-01 13:23 ` Arnaldo Carvalho de Melo
@ 2021-06-01 23:45 ` Masami Hiramatsu
1 sibling, 0 replies; 3+ messages in thread
From: Masami Hiramatsu @ 2021-06-01 23:45 UTC (permalink / raw)
To: Li Huafei
Cc: acme, namhyung, peterz, mingo, mark.rutland, alexander.shishkin,
jolsa, srikar, fche, Jianlin.Lv, linux-perf-users, linux-kernel,
yangjihong1, zhangjinhao2
On Tue, 1 Jun 2021 17:27:50 +0800
Li Huafei <lihuafei1@huawei.com> wrote:
> If we just check whether the variable can be converted, 'tvar' should be
> a null pointer. However, the null pointer check is missing in the
> 'Constant value' execution path.
>
> The following cases can trigger this problem:
>
> $ cat test.c
> #include <stdio.h>
>
> void main(void)
> {
> int a;
> const int b = 1;
>
> asm volatile("mov %1, %0" : "=r"(a): "i"(b));
> printf("a: %d\n", a);
> }
>
> $ gcc test.c -o test -O -g
> $ sudo ./perf probe -x ./test -L "main"
> <main@/home/lhf/test.c:0>
> 0 void main(void)
> {
> 2 int a;
> const int b = 1;
>
> asm volatile("mov %1, %0" : "=r"(a): "i"(b));
> 6 printf("a: %d\n", a);
> }
>
> $ sudo ./perf probe -x ./test -V "main:6"
> Segmentation fault
Oops, thanks for fixing!
>
> The check on 'tvar' is added. If 'tavr' is a null pointer, we return 0
> to indicate that the variable can be converted. Now, we can successfully
> show the variables that can be accessed.
>
> $ sudo ./perf probe -x ./test -V "main:6"
> Available variables at main:6
> @<main+13>
> char* __fmt
> int a
> int b
>
> However, the variable 'b' cannot be tracked.
>
> $ sudo ./perf probe -x ./test -D "main:6 b"
> Failed to find the location of the 'b' variable at this address.
> Perhaps it has been optimized out.
> Use -V with the --range option to show 'b' location range.
> Error: Failed to add events.
>
> This is because __die_find_variable_cb() did not successfully match
> variable 'b', which has the DW_AT_const_value attribute instead of
> DW_AT_location. We added support for DW_AT_const_value in
> __die_find_variable_cb(). With this modification, we can successfully
> track the variable 'b'.
>
> $ sudo ./perf probe -x ./test -D "main:6 b"
> p:probe_test/main_L6 /home/lhf/test:0x1156 b=\1:s32
Great! This looks good to me.
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Thank you!
>
> Fixes: 66f69b219716 ("perf probe: Support DW_AT_const_value constant value")
> Signed-off-by: Li Huafei <lihuafei1@huawei.com>
> ---
> tools/perf/util/dwarf-aux.c | 8 ++++++--
> tools/perf/util/probe-finder.c | 3 +++
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index b2f4920e19a6..7d2ba8419b0c 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
> @@ -975,9 +975,13 @@ static int __die_find_variable_cb(Dwarf_Die *die_mem, void *data)
> if ((tag == DW_TAG_formal_parameter ||
> tag == DW_TAG_variable) &&
> die_compare_name(die_mem, fvp->name) &&
> - /* Does the DIE have location information or external instance? */
> + /*
> + * Does the DIE have location information or const value
> + * or external instance?
> + */
> (dwarf_attr(die_mem, DW_AT_external, &attr) ||
> - dwarf_attr(die_mem, DW_AT_location, &attr)))
> + dwarf_attr(die_mem, DW_AT_location, &attr) ||
> + dwarf_attr(die_mem, DW_AT_const_value, &attr)))
> return DIE_FIND_CB_END;
> if (dwarf_haspc(die_mem, fvp->addr))
> return DIE_FIND_CB_CONTINUE;
> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> index 866f2d514d72..b029c29ce227 100644
> --- a/tools/perf/util/probe-finder.c
> +++ b/tools/perf/util/probe-finder.c
> @@ -190,6 +190,9 @@ static int convert_variable_location(Dwarf_Die *vr_die, Dwarf_Addr addr,
> immediate_value_is_supported()) {
> Dwarf_Sword snum;
>
> + if (!tvar)
> + return 0;
> +
> dwarf_formsdata(&attr, &snum);
> ret = asprintf(&tvar->value, "\\%ld", (long)snum);
>
> --
> 2.17.1
>
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-06-01 23:45 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-01 9:27 [PATCH] perf probe: Fix null pointer dereference in convert_variable_location() Li Huafei
2021-06-01 13:23 ` Arnaldo Carvalho de Melo
2021-06-01 23:45 ` Masami Hiramatsu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.