linux-kernel - Re: [PATCH] ARM : unwinder : Prevent data abort due to stack overflow

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 7 Dec 2013 13:43:21 +0530
From:	Anurag Aggarwal <anurag19aggarwal@...il.com>
To:	Dave Martin <Dave.Martin@....com>
Cc:	Anurag Aggarwal <a.anurag@...sung.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"cpgs@...sung.com" <cpgs@...sung.com>,
	"narendra.m1@...sung.com" <narendra.m1@...sung.com>,
	"poorva.s@...sung.com" <poorva.s@...sung.com>,
	"naveen.sel@...sung.com" <naveen.sel@...sung.com>,
	"ashish.kalra@...sung.com" <ashish.kalra@...sung.com>,
	"mohammad.a2@...sung.com" <mohammad.a2@...sung.com>,
	"rajat.suri@...sung.com" <rajat.suri@...sung.com>,
	"naveenkrishna.ch@...il.com" <naveenkrishna.ch@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Will Deacon <Will.Deacon@....com>,
	"nico@...aro.org" <nico@...aro.org>,
	Catalin Marinas <Catalin.Marinas@....com>
Subject: Re: [PATCH] ARM : unwinder : Prevent data abort due to stack overflow

>>
>> +     /* 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@....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@...sung.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/