[PATCH] ARM : unwinder : Prevent data abort due to stack overflow

[Anurag Aggarwal]

>>
>> +     /* we are just starting, initialize last register set as 0 */
>> +     ctrl.last_register_set = 0;
>> +
>>       while (ctrl.entries > 0) {
>> -             int urc = unwind_exec_insn(&ctrl);
>> +             int urc;
>> +             if ((ctrl.sp_high - ctrl.vrs[SP]) < TOTAL_REGISTERS)
>> +                     ctrl.last_register_set = 1;
>
>If this is done once per unwind_exec_insn(), I think it would be better
>to put the check at the start of unwind_exec_insn() instead of outside.

I think it is better to do the above check here only because this check
is strictly not a part of decoder and execution cycle.

I think uniwnd_exec_insn(), should only be used for decoding and
execution of instruction, as you have suggested earlier. So, it makes
sense to keep it in unwind_frame only().


On Fri, Dec 6, 2013 at 5:41 PM, Dave Martin <Dave.Martin at arm.com> wrote:
> On Fri, Dec 06, 2013 at 06:09:53AM +0000, Anurag Aggarwal wrote:
>> While unwinding backtrace, stack overflow is possible. This stack
>> overflow can sometimes lead to data abort in system if the area after
>> stack is not mapped to physical memory.
>>
>> To prevent this problem from happening, execute the instructions that
>> can cause a data abort in separate helper functions, where a check for
>> feasibility is made before reading each word from the stack.
>>
>> Signed-off-by: Anurag Aggarwal <a.anurag at samsung.com>
>> ---
>>  arch/arm/kernel/unwind.c | 138 ++++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 100 insertions(+), 38 deletions(-)
>>
>> diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
>> index 00df012..6d854f8 100644
>> --- a/arch/arm/kernel/unwind.c
>> +++ b/arch/arm/kernel/unwind.c
>> @@ -49,6 +49,8 @@
>>  #include <asm/traps.h>
>>  #include <asm/unwind.h>
>>
>> +#define TOTAL_REGISTERS 16
>> +
>>  /* Dummy functions to avoid linker complaints */
>>  void __aeabi_unwind_cpp_pr0(void)
>>  {
>> @@ -66,9 +68,11 @@ void __aeabi_unwind_cpp_pr2(void)
>>  EXPORT_SYMBOL(__aeabi_unwind_cpp_pr2);
>>
>>  struct unwind_ctrl_block {
>> -     unsigned long vrs[16];          /* virtual register set */
>> +     unsigned long vrs[TOTAL_REGISTERS];     /* virtual register set */
>>       const unsigned long *insn;      /* pointer to the current instructions word */
>> +     unsigned long sp_high;          /* highest value of sp allowed*/
>>       int entries;                    /* number of entries left to interpret */
>> +     int last_register_set;          /* store if we are at the last set */
>
> I find the name and comment a bit confusing here.  Also, strictly
> speaking it can be a bool.
>
> Maybe:
>
>         /*
>          * true: check for stack overflow for each register pop;
>          * false: save overhead if there is plenty of stack remaining.
>          */
>         bool check_each_pop;
>
>
> It shouldn't be too hard to understand from reading the code though, so
> I'm happy with your version if you prefer.
>
> [...]
>
>> @@ -382,11 +438,17 @@ int unwind_frame(struct stackframe *frame)
>>               return -URC_FAILURE;
>>       }
>>
>> +     /* we are just starting, initialize last register set as 0 */
>> +     ctrl.last_register_set = 0;
>> +
>>       while (ctrl.entries > 0) {
>> -             int urc = unwind_exec_insn(&ctrl);
>> +             int urc;
>> +             if ((ctrl.sp_high - ctrl.vrs[SP]) < TOTAL_REGISTERS)
>> +                     ctrl.last_register_set = 1;
>
> If this is done once per unwind_exec_insn(), I think it would be better
> to put the check at the start of unwind_exec_insn() instead of outside.
>
>
> The check still looks wrong too?
>
> ctrl.sp_high - ctrl.vrs[SP] gives the available space in bytes, but
> TOTAL_REGISTERS is measured in words.
>
>
> One way to get the correct value would be simply
>
>         sizeof ctrl.vrs
>
> since that's the array we're trying to fill from the stack.
>
> (in that case I guess that the TOTAL_REGISTERS macro might not be needed
> again)
>
> Cheers
> ---Dave



-- 
Anurag Aggarwal