Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
MIME-Version: 1.0
References: <20230927055954.3755-1-quic_aiquny@quicinc.com>
In-Reply-To: <20230927055954.3755-1-quic_aiquny@quicinc.com>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Wed, 27 Sep 2023 09:26:28 +0000
Message-ID: <CAMj1kXEh=yQ12v1Ht6=-vgXVe8VCQTPdUD8AowkKcpyDXh_0mg@mail.gmail.com>
Subject: Re: [PATCH] ARM: kprobes: Explicitly assign register for local variables
To:     Maria Yu <quic_aiquny@quicinc.com>
Cc:     linux@armlinux.org.uk, mhiramat@kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        kernel@quicinc.com, quic_lijuang@quicinc.com,
        linux-arm-msm@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hello Maria,

On Wed, 27 Sept 2023 at 06:00, Maria Yu <quic_aiquny@quicinc.com> wrote:
>
> Registers r7 is removed in clobber list, so compiler may choose r7 for
> local variables usage, while r7 will be actually updated by the inline asm
> code.

The inline asm does not update R7, it preserves and restores it.

> This caused the runtime behavior wrong.

Could you explain how, exactly? In which cases is the preserve/restore
of R7 failing to achieve the intended result?

> While those kind of reserved registers cannot be set to clobber list
> because of error like "inline asm clobber list contains reserved
> registers".
> To both working for reserved register case and non-reserved register case,
> explicitly assign register for local variables which will be used as asm
> input.
>

If we make this change, could we remove the references to R7 altogether?

> Fixes: dd12e97f3c72 ("ARM: kprobes: treat R7 as the frame pointer register in Thumb2 builds")
> Signed-off-by: Maria Yu <quic_aiquny@quicinc.com>
> ---
>  arch/arm/probes/kprobes/actions-thumb.c | 32 ++++++++++++++++---------
>  1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm/probes/kprobes/actions-thumb.c b/arch/arm/probes/kprobes/actions-thumb.c
> index 51624fc263fc..f667b2f00b3e 100644
> --- a/arch/arm/probes/kprobes/actions-thumb.c
> +++ b/arch/arm/probes/kprobes/actions-thumb.c
> @@ -442,8 +442,10 @@ static unsigned long __kprobes
>  t16_emulate_loregs(probes_opcode_t insn,
>                    struct arch_probes_insn *asi, struct pt_regs *regs)
>  {
> -       unsigned long oldcpsr = regs->ARM_cpsr;
> -       unsigned long newcpsr;
> +       register unsigned long oldcpsr asm("r8") = regs->ARM_cpsr;
> +       register unsigned long newcpsr asm("r9");
> +       register void *rregs asm("r10") = regs;
> +       register void *rfn asm("lr") = asi->insn_fn;
>
>         __asm__ __volatile__ (
>                 "msr    cpsr_fs, %[oldcpsr]     \n\t"
> @@ -454,10 +456,10 @@ t16_emulate_loregs(probes_opcode_t insn,
>                 "mov    r7, r11                 \n\t"
>                 "mrs    %[newcpsr], cpsr        \n\t"
>                 : [newcpsr] "=r" (newcpsr)
> -               : [oldcpsr] "r" (oldcpsr), [regs] "r" (regs),
> -                 [fn] "r" (asi->insn_fn)
> +               : [oldcpsr] "r" (oldcpsr), [regs] "r" (rregs),
> +                 [fn] "r" (rfn)
>                 : "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r11",
> -                 "lr", "memory", "cc"
> +                 "memory", "cc"
>                 );
>
>         return (oldcpsr & ~APSR_MASK) | (newcpsr & APSR_MASK);
> @@ -525,6 +527,9 @@ static void __kprobes
>  t16_emulate_push(probes_opcode_t insn,
>                 struct arch_probes_insn *asi, struct pt_regs *regs)
>  {
> +       register void *rfn asm("lr") = asi->insn_fn;
> +       register void *rregs asm("r10") = regs;
> +
>         __asm__ __volatile__ (
>                 "mov    r11, r7                 \n\t"
>                 "ldr    r9, [%[regs], #13*4]    \n\t"
> @@ -534,9 +539,9 @@ t16_emulate_push(probes_opcode_t insn,
>                 "str    r9, [%[regs], #13*4]    \n\t"
>                 "mov    r7, r11                 \n\t"
>                 :
> -               : [regs] "r" (regs), [fn] "r" (asi->insn_fn)
> +               : [regs] "r" (rregs), [fn] "r" (rfn)
>                 : "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r8", "r9", "r11",
> -                 "lr", "memory", "cc"
> +                 "memory", "cc"
>                 );
>  }
>
> @@ -561,6 +566,9 @@ static void __kprobes
>  t16_emulate_pop_nopc(probes_opcode_t insn,
>                 struct arch_probes_insn *asi, struct pt_regs *regs)
>  {
> +       register void *rfn asm("lr") = asi->insn_fn;
> +       register void *rregs asm("r8") = regs;
> +
>         __asm__ __volatile__ (
>                 "mov    r11, r7                 \n\t"
>                 "ldr    r9, [%[regs], #13*4]    \n\t"
> @@ -570,9 +578,9 @@ t16_emulate_pop_nopc(probes_opcode_t insn,
>                 "str    r9, [%[regs], #13*4]    \n\t"
>                 "mov    r7, r11                 \n\t"
>                 :
> -               : [regs] "r" (regs), [fn] "r" (asi->insn_fn)
> +               : [regs] "r" (rregs), [fn] "r" (rfn)
>                 : "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r9", "r11",
> -                 "lr", "memory", "cc"
> +                 "memory", "cc"
>                 );
>  }
>
> @@ -581,6 +589,8 @@ t16_emulate_pop_pc(probes_opcode_t insn,
>                 struct arch_probes_insn *asi, struct pt_regs *regs)
>  {
>         register unsigned long pc asm("r8");
> +       register void *rfn asm("lr") = asi->insn_fn;
> +       register void *rregs asm("r10") = regs;
>
>         __asm__ __volatile__ (
>                 "mov    r11, r7                 \n\t"
> @@ -591,9 +601,9 @@ t16_emulate_pop_pc(probes_opcode_t insn,
>                 "str    r9, [%[regs], #13*4]    \n\t"
>                 "mov    r7, r11                 \n\t"
>                 : "=r" (pc)
> -               : [regs] "r" (regs), [fn] "r" (asi->insn_fn)
> +               : [regs] "r" (rregs), [fn] "r" (rfn)
>                 : "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r9", "r11",
> -                 "lr", "memory", "cc"
> +                 "memory", "cc"
>                 );
>
>         bx_write_pc(pc, regs);
>
> base-commit: 6465e260f48790807eef06b583b38ca9789b6072
> --
> 2.17.1
>