linux-kernel - Re: [PATCH] firmware: qcom: scm: Fix crash in qcom_scm_call

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 10 Apr 2018 11:58:54 -0700
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Niklas Cassel <niklas.cassel@...aro.org>
Cc:     Andy Gross <andy.gross@...aro.org>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Avaneesh Kumar Dwivedi <akdwived@...eaurora.org>,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] firmware: qcom: scm: Fix crash in qcom_scm_call_atomic1()

On Mon 09 Apr 14:40 PDT 2018, Niklas Cassel wrote:

> qcom_scm_call_atomic1() can crash with a NULL pointer dereference at
> qcom_scm_call_atomic1+0x30/0x48.
> 
> disassembly of qcom_scm_call_atomic1():
> ...
> <0xc08d73b0 <+12>: ldr r3, [r12]
> ... (no instruction explicitly modifies r12)
> 0xc08d73cc <+40>: smc 0
> ... (no instruction explicitly modifies r12)
> 0xc08d73d4 <+48>: ldr r3, [r12] <- crashing instruction
> ...
> 
> Since the first ldr is successful, and since r12 isn't explicitly
> modified by any instruction between the first and the second ldr,
> it must have been modified by the smc call, which is ok,
> since r12 is caller save according to the AAPCS.
> 
> Add r12 to the clobber list so that the compiler knows that the
> callee potentially overwrites the value in r12.
> Clobber descriptions may not in any way overlap with an input or
> output operand.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@...aro.org>

Regards,
Bjorn

> Signed-off-by: Niklas Cassel <niklas.cassel@...aro.org>
> ---
>  drivers/firmware/qcom_scm-32.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
> index dfbd894d5bb7..4e24e591ae74 100644
> --- a/drivers/firmware/qcom_scm-32.c
> +++ b/drivers/firmware/qcom_scm-32.c
> @@ -147,7 +147,7 @@ static u32 smc(u32 cmd_addr)
>  			"smc	#0	@ switch to secure world\n"
>  			: "=r" (r0)
>  			: "r" (r0), "r" (r1), "r" (r2)
> -			: "r3");
> +			: "r3", "r12");
>  	} while (r0 == QCOM_SCM_INTERRUPTED);
>  
>  	return r0;
> @@ -263,7 +263,7 @@ static s32 qcom_scm_call_atomic1(u32 svc, u32 cmd, u32 arg1)
>  			"smc    #0      @ switch to secure world\n"
>  			: "=r" (r0)
>  			: "r" (r0), "r" (r1), "r" (r2)
> -			: "r3");
> +			: "r3", "r12");
>  	return r0;
>  }
>  
> @@ -298,7 +298,7 @@ static s32 qcom_scm_call_atomic2(u32 svc, u32 cmd, u32 arg1, u32 arg2)
>  			"smc    #0      @ switch to secure world\n"
>  			: "=r" (r0)
>  			: "r" (r0), "r" (r1), "r" (r2), "r" (r3)
> -			);
> +			: "r12");
>  	return r0;
>  }
>  
> @@ -328,7 +328,7 @@ u32 qcom_scm_get_version(void)
>  			"smc	#0	@ switch to secure world\n"
>  			: "=r" (r0), "=r" (r1)
>  			: "r" (r0), "r" (r1)
> -			: "r2", "r3");
> +			: "r2", "r3", "r12");
>  	} while (r0 == QCOM_SCM_INTERRUPTED);
>  
>  	version = r1;
> -- 
> 2.14.3
>