ARC: Change ld.as instruction to regular ld.
diff mbox

Message ID 1471334135-21801-1-git-send-email-liavr@mellanox.com
State New
Headers show

Commit Message

Liav Rehana Aug. 16, 2016, 7:55 a.m. UTC
From: Liav Rehana <liavr@mellanox.com>

The instruction ld.as takes as operands a base address and an offset,
and doesn't access the sum of these two, but the sum of the base
address and a shifted version of the offset.
This isn't what we want in that case, since it causes a bug during
the push and pop of r25, since his actual offset is given during
resume_user_mode_begin.
Thus, the use of ld solves this problem.

Signed-off-by: Liav Rehana <liavr@mellanox.com>
---
 arch/arc/include/asm/entry.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Comments

Alexey Brodkin Aug. 16, 2016, 1:15 p.m. UTC | #1
Hi Liav,

On Tue, 2016-08-16 at 10:55 +0300, Liav Rehana wrote:
> From: Liav Rehana <liavr@mellanox.com>
> 
> The instruction ld.as takes as operands a base address and an offset,
> and doesn't access the sum of these two, but the sum of the base
> address and a shifted version of the offset.
> This isn't what we want in that case, since it causes a bug during
> the push and pop of r25, since his actual offset is given during
> resume_user_mode_begin.
> Thus, the use of ld solves this problem.
> 
> Signed-off-by: Liav Rehana <liavr@mellanox.com>
> ---

Very nice catch!

But IMHO description could be improved a little bit.
Probably something like that:
--------------------->8---------------------
"PT_user_r25" is offset in bytes within pt_regs structure.

In its turn what "ld.as r1, [r2, x]" really does is
r1 <- load_from(r2 + (x << data_size)) = load_from(r2 + x*4).

But the code in question is supposed to load_from(r2 + x).

This leads to obvious stack corruption.
--------------------->8---------------------

Reviewed-by: Alexey Brodkin <abrodkin@synopsys.com>
Vineet Gupta Aug. 16, 2016, 3:47 p.m. UTC | #2
On 08/16/2016 06:15 AM, Alexey Brodkin wrote:
> Hi Liav,
> 
> On Tue, 2016-08-16 at 10:55 +0300, Liav Rehana wrote:
>> From: Liav Rehana <liavr@mellanox.com>
>>
>> The instruction ld.as takes as operands a base address and an offset,
>> and doesn't access the sum of these two, but the sum of the base
>> address and a shifted version of the offset.
>> This isn't what we want in that case, since it causes a bug during
>> the push and pop of r25, since his actual offset is given during
>> resume_user_mode_begin.
>> Thus, the use of ld solves this problem.
>>
>> Signed-off-by: Liav Rehana <liavr@mellanox.com>
>> ---
> 
> Very nice catch!
> 
> But IMHO description could be improved a little bit.
> Probably something like that:
> --------------------->8---------------------
> "PT_user_r25" is offset in bytes within pt_regs structure.
> 
> In its turn what "ld.as r1, [r2, x]" really does is
> r1 <- load_from(r2 + (x << data_size)) = load_from(r2 + x*4).
> 
> But the code in question is supposed to load_from(r2 + x).
> 
> This leads to obvious stack corruption.
> --------------------->8---------------------

Right - this is much better. A good changelog also needs to explain the context of
problem. How does below sound ...

-------->
User mode callee regs are explicitly collected before signal delivery or
breakpoint trap. r25 is special for kernel as it serves as task pointer,
so user mode value is clobbered very early. It is saved in pt_regs where
generally only scratch (caller saved) res are saved.

The code to access the corresponding pt_regs location had a subtle bug as
it was using load/store with scaling of offset, whereas the offset was already
byte wise correct. So fix this by replacing LD.AS with a standard LD

Patch
diff mbox

diff --git a/arch/arc/include/asm/entry.h b/arch/arc/include/asm/entry.h
index 337ab6d..9d8f85d 100644
--- a/arch/arc/include/asm/entry.h
+++ b/arch/arc/include/asm/entry.h
@@ -138,7 +138,7 @@ 
 
 #ifdef CONFIG_ARC_CURR_IN_REG
 	; Retrieve orig r25 and save it with rest of callee_regs
-	ld.as   r12, [r12, PT_user_r25]
+	ld	r12, [r12, PT_user_r25]
 	PUSH	r12
 #else
 	PUSH	r25
@@ -194,7 +194,7 @@ 
 
 	; SP is back to start of pt_regs
 #ifdef CONFIG_ARC_CURR_IN_REG
-	st.as   r12, [sp, PT_user_r25]
+	st	r12, [sp, PT_user_r25]
 #endif
 .endm