When metadata is allocated using arm_spe__alloc_metadata(), if the
metadata version is 1, metadata is returned as NULL. This value
is dereferenced later in arm_spe__free_metadata() and it can cause
null-ptr-deref.

Modify the NULL check for metadata to return -EINVAL even when
metadata_ver == 1.

This issue was reported by Coverity scan [1].
[1] https://scan5.scan.coverity.com/#/project-view/63616/10063?selectedIssue=1636359

Closes: https://scan5.scan.coverity.com/#/project-view/63616/10063?selectedIssue=1636359
Signed-off-by: Nihar Chaithanya <niharchaithanya@gmail.com>
---
 tools/perf/util/arm-spe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c
index dbf13f47879c..55827d8ce133 100644
--- a/tools/perf/util/arm-spe.c
+++ b/tools/perf/util/arm-spe.c
@@ -1497,7 +1497,7 @@ int arm_spe_process_auxtrace_info(union perf_event *event,
 
 	metadata = arm_spe__alloc_metadata(auxtrace_info, &metadata_ver,
 					   &nr_cpu);
-	if (!metadata && metadata_ver != 1) {
+	if (!metadata) {
 		pr_err("Failed to parse Arm SPE metadata.\n");
 		return -EINVAL;
 	}
-- 
2.34.1

On 20/12/2024 2:57 pm, Nihar Chaithanya wrote:
> When metadata is allocated using arm_spe__alloc_metadata(), if the
> metadata version is 1, metadata is returned as NULL. This value
> is dereferenced later in arm_spe__free_metadata() and it can cause
> null-ptr-deref.
> 
> Modify the NULL check for metadata to return -EINVAL even when
> metadata_ver == 1.
> 

Hi Nihar,

I don't think this is the right fix. Doesn't that mean we can't open 
files with V1 anymore? Did you test opening an old SPE file on a new 
version of Perf? I actually thought I tested this before but maybe not 
if there's a NULL deref.

Seems like the correct fix is to not dereference anything if metadata is 
NULL, or make a fake placeholder one.


> This issue was reported by Coverity scan [1].
> [1] https://scan5.scan.coverity.com/#/project-view/63616/10063?selectedIssue=1636359
> 

Is this supposed to be publicly accessible? It goes to a login page for me.

> Closes: https://scan5.scan.coverity.com/#/project-view/63616/10063?selectedIssue=1636359
> Signed-off-by: Nihar Chaithanya <niharchaithanya@gmail.com>

Probably needs a fixes: tag too for the commit that introduced the issue.

Thanks
James

> ---
>   tools/perf/util/arm-spe.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c
> index dbf13f47879c..55827d8ce133 100644
> --- a/tools/perf/util/arm-spe.c
> +++ b/tools/perf/util/arm-spe.c
> @@ -1497,7 +1497,7 @@ int arm_spe_process_auxtrace_info(union perf_event *event,
>   
>   	metadata = arm_spe__alloc_metadata(auxtrace_info, &metadata_ver,
>   					   &nr_cpu);
> -	if (!metadata && metadata_ver != 1) {
> +	if (!metadata) {
>   		pr_err("Failed to parse Arm SPE metadata.\n");
>   		return -EINVAL;
>   	}