[PATCH] perf probe: Fix null pointer dereference in convert_variable_location()

[PATCH] perf probe: Fix null pointer dereference in convert_variable_location()

[Li Huafei]

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

Re: [PATCH] perf probe: Fix null pointer dereference in convert_variable_location()

[Arnaldo Carvalho de Melo]

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

Re: [PATCH] perf probe: Fix null pointer dereference in convert_variable_location()

[Masami Hiramatsu]

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>