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

[Masami Hiramatsu <mhiramat@kernel.org>]

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>