Re: [PATCH] firmware: qcom: scm: Fix crash in qcom_scm_call_atomic1()

From: Bjorn Andersson
Date: Tue Apr 10 2018 - 14:58:21 EST


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@xxxxxxxxxx>

Regards,
Bjorn

> Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxxxxx>
> ---
> 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
>