From mboxrd@z Thu Jan 1 00:00:00 1970 From: nico@fluxnic.net (Nicolas Pitre) Date: Tue, 29 Mar 2011 13:07:34 -0400 (EDT) Subject: [PATCH] Fix ldrd/strd emulation for kprobes/ARM In-Reply-To: <1301403301.2519.125.camel@computer2.home> References: <1301087944.2744.85.camel@computer2.home> <1301327765-6996-1-git-send-email-viktor.rosendahl@nokia.com> <1301403301.2519.125.camel@computer2.home> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, 29 Mar 2011, Tixy wrote: > On Mon, 2011-03-28 at 18:56 +0300, Viktor Rosendahl wrote: > > On 03/25/2011 11:19 PM, ext Tixy wrote: > > > On Fri, 2011-03-25 at 19:01 +0200, Viktor Rosendahl wrote: > > >> The Rn value from the emulation is unconditionally written back; this is fine > > >> as long as Rn != PC because in that case, even if the instruction isn't a write > > >> back instruction, it will only result in the same value being written back. > > >> > > >> In case Rn == PC, then the emulated instruction doesn't have the actual PC > > >> value in Rn but an adjusted value; when this is written back, it will result in > > >> the PC being incorrectly updated. > > > It looks like we have the same problem with emulate_ldrd and > > > emulate_strd as well. > > > > Yes, it seems so. Currently emulate_ldrd and emulate_strd don't even have the > > adjustment of the PC value, so in case of Rn == PC, it will not update the PC > > incorrectly but instead load/store from the wrong address. Below is a patch > > that adds both the adjustment of the PC value and the check for PC == PC. > > > > I haven't tested this code at all and I am not sure of how to test it. > > In a day or two I should have support in my test code for these sorts of > instructions. From looking at them, the changes look functionally > correct though. I do have a suggestion for slight change, see inline > comments below... > > > > > Signed-off-by: Viktor Rosendahl > > --- > > arch/arm/kernel/kprobes-decode.c | 20 +++++++++++++++----- > > 1 files changed, 15 insertions(+), 5 deletions(-) > > > > diff --git a/arch/arm/kernel/kprobes-decode.c b/arch/arm/kernel/kprobes-decode.c > > index 2389131..3b0cf90 100644 > > --- a/arch/arm/kernel/kprobes-decode.c > > +++ b/arch/arm/kernel/kprobes-decode.c > > @@ -540,9 +540,12 @@ static void __kprobes emulate_ldrd(struct kprobe *p, struct pt_regs *regs) > > { > > insn_2arg_fn_t *i_fn = (insn_2arg_fn_t *)&p->ainsn.insn[0]; > > kprobe_opcode_t insn = p->opcode; > > + long ppc = (long)p->addr + 8; > > int rd = (insn >> 12) & 0xf; > > int rn = (insn >> 16) & 0xf; > > int rm = insn & 0xf; /* rm may be invalid, don't care. */ > > + long rmv = (rm == 15) ? ppc : regs->uregs[rm]; > > + long rnv = (rn == 15) ? ppc : regs->uregs[rn]; > > > > /* Not following the C calling convention here, so need asm(). */ > > __asm__ __volatile__ ( > > @@ -554,29 +557,36 @@ static void __kprobes emulate_ldrd(struct kprobe *p, struct pt_regs *regs) > > "str r0, %[rn] \n\t" /* in case of writeback */ > > "str r2, %[rd0] \n\t" > > "str r3, %[rd1] \n\t" > > - : [rn] "+m" (regs->uregs[rn]), > > + : [rn] "+m" (rnv), > > [rd0] "=m" (regs->uregs[rd]), > > [rd1] "=m" (regs->uregs[rd+1]) > > - : [rm] "m" (regs->uregs[rm]), > > + : [rm] "m" (rmv), > > [cpsr] "r" (regs->ARM_cpsr), > > [i_fn] "r" (i_fn) > > : "r0", "r1", "r2", "r3", "lr", "cc" > > ); > > + if (rn != 15) > > + regs->uregs[rn] = rnv; /* Save Rn in case of writeback. */ > > } > > > If the variables rnv and rmv were declared as registers then we could > avoid 3 stores and 3 loads of stack values. This would require changing > the the assembler as well, e.g. something like this... > > register long rnv asm("r0"); > register long rmv asm("r1"); > rnv = (rn == 15) ? ppc : regs->uregs[rn]; > rmv = (rm == 15) ? ppc : regs->uregs[rm]; > > __asm__ __volatile__ ( > "msr cpsr_fs, %[cpsr]\n\t" > "mov lr, pc \n\t" > "mov pc, %[i_fn] \n\t" > "str r2, %[rd0] \n\t" > "str r3, %[rd1] \n\t" > : "=r" (rnv), > [rd0] "=m" (regs->uregs[rd]), > [rd1] "=m" (regs->uregs[rd+1]) > : "0" (rnv), > "r" (rmv), > [cpsr] "r" (regs->ARM_cpsr), > [i_fn] "r" (i_fn) > : "r2", "r3", "lr", "cc" > ); I agree this is a nice improvement. However it is best to keep fixes and improvements into separate patches. Nicolas