2017-03-01 07:31:26

by Matt Redfearn

Subject: [PATCH 1/4] MIPS: Handle non word sized instructions when examining frame

Commit b6c7a324df37 ("MIPS: Fix get_frame_info() handling of microMIPS
function size") goes some way to fixing get_frame_info() to iterate over
microMIPS instuctions, but increments the instruction pointer using a
postincrement of the instruction pointer, which is of union
mips_instruction type. Since the union is sized to the largest member (a
word), but microMIPS instructions are a mix of halfword and word sizes,
the function does not always iterate correctly, ending up misaligned
with the instruction stream and interpreting it incorrectly.

Since the instruction modifying the stack pointer is usually the first
in the function, that one is ususally handled correctly. But the
instruction which saves the return address to the sp is some variable
number of instructions into the frame and is frequently missed due to
not being on a word boundary, leading to incomplete walking of the
stack.

Fix this by incrementing the instruction pointer based on the size of
the previously decoded instruction.

Fixes: b6c7a324df37 ("MIPS: Fix get_frame_info() handling of microMIPS function size")
Signed-off-by: Matt Redfearn <[email protected]>
---

arch/mips/kernel/process.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index 803e255b6fc3..5b1e932ae973 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -347,6 +347,7 @@ static int get_frame_info(struct mips_frame_info *info)
union mips_instruction insn, *ip, *ip_end;
const unsigned int max_insns = 128;
unsigned int i;
+ unsigned int last_insn_size = 0;

info->pc_offset = -1;
info->frame_size = 0;
@@ -357,15 +358,19 @@ static int get_frame_info(struct mips_frame_info *info)

ip_end = (void *)ip + info->func_size;

- for (i = 0; i < max_insns && ip < ip_end; i++, ip++) {
+ for (i = 0; i < max_insns && ip < ip_end; i++) {
+ ip = (void *)ip + last_insn_size;
if (is_mmips && mm_insn_16bit(ip->halfword[0])) {
insn.halfword[0] = 0;
insn.halfword[1] = ip->halfword[0];
+ last_insn_size = sizeof(unsigned short);
} else if (is_mmips) {
insn.halfword[0] = ip->halfword[1];
insn.halfword[1] = ip->halfword[0];
+ last_insn_size = sizeof(unsigned int);
} else {
insn.word = ip->word;
+ last_insn_size = sizeof(unsigned int);
}

if (is_jump_ins(&insn))
--
2.7.4

2017-03-01 14:07:02

by Matt Redfearn

Subject: Re: [PATCH 1/4] MIPS: Handle non word sized instructions when examining frame

Hi Maciej


On 28/02/17 20:54, Maciej W. Rozycki wrote:
> On Tue, 28 Feb 2017, Matt Redfearn wrote:
>
>> Since the instruction modifying the stack pointer is usually the first
>> in the function, that one is ususally handled correctly. But the
> s/ususally/usually/

oops

>
>> diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
>> index 803e255b6fc3..5b1e932ae973 100644
>> --- a/arch/mips/kernel/process.c
>> +++ b/arch/mips/kernel/process.c
>> @@ -347,6 +347,7 @@ static int get_frame_info(struct mips_frame_info *info)
>> union mips_instruction insn, *ip, *ip_end;
>> const unsigned int max_insns = 128;
>> unsigned int i;
>> + unsigned int last_insn_size = 0;
> Please keep declarations in the reverse Christmas tree order, i.e. swap
> the last two.
>
> Maciej

OK, no problem.
Thanks,
Matt