Re: [PATCH] ARM : unwinder : Prevent data abort due to stack overflow
On Sat, Dec 07, 2013 at 01:43:21PM +0530, Anurag Aggarwal wrote:
> >>
> >> + /* we are just starting, initialize last register set as 0 */
> >> + ctrl.last_register_set = 0;
> >> +
> >> while (ctrl.entries > 0) {
> >> - int urc = unwind_exec_insn();
> >> + 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().
It's debatable, since the stack checking is an intrinsic part of insn
execution. But since there is only one call site for unwind_exec_insn
and it is unlikely than another will appear in the future, I am happy
for it to remain in your current form.
Cheers
---Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM : unwinder : Prevent data abort due to stack overflow
On Sat, Dec 07, 2013 at 01:43:21PM +0530, Anurag Aggarwal wrote:
+ /* 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().
It's debatable, since the stack checking is an intrinsic part of insn
execution. But since there is only one call site for unwind_exec_insn
and it is unlikely than another will appear in the future, I am happy
for it to remain in your current form.
Cheers
---Dave
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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();
>> + 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 wrote:
> On Fri, Dec 06, 2013 at 06:09:53AM +, 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
>> ---
>> 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
>> #include
>>
>> +#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();
>> + 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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.mar...@arm.com wrote:
On Fri, Dec 06, 2013 at 06:09:53AM +, 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.anu...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM : unwinder : Prevent data abort due to stack overflow
On Fri, Dec 06, 2013 at 06:09:53AM +, 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
> ---
> 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
> #include
>
> +#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();
> + 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM : unwinder : Prevent data abort due to stack overflow
On Fri, Dec 06, 2013 at 06:09:53AM +, 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.anu...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] ARM : unwinder : Prevent data abort due to stack overflow
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
---
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
#include
+#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 */
int byte; /* current byte number in the
instructions word */
};
@@ -235,12 +239,85 @@ static unsigned long unwind_get_byte(struct
unwind_ctrl_block *ctrl)
return ret;
}
+/* Before poping a register check whether it is feasible or not */
+static int unwind_pop_register(struct unwind_ctrl_block *ctrl,
+ unsigned long **vsp, unsigned int reg)
+{
+ if (unlikely(ctrl->last_register_set))
+ if (*vsp >= (unsigned long *)ctrl->sp_high)
+ return -URC_FAILURE;
+
+ ctrl->vrs[reg] = *(*vsp)++;
+ return URC_OK;
+}
+
+/* Helper functions to execute the instructions */
+static int unwind_exec_pop_subset_r4_to_r13(struct unwind_ctrl_block *ctrl,
+ unsigned long mask)
+{
+ unsigned long *vsp = (unsigned long *)ctrl->vrs[SP];
+ int load_sp, reg = 4;
+
+ load_sp = mask & (1 << (13 - 4));
+ while (mask) {
+ if (mask & 1)
+ if (unwind_pop_register(ctrl, , reg))
+ return -URC_FAILURE;
+ mask >>= 1;
+ reg++;
+ }
+ if (!load_sp)
+ ctrl->vrs[SP] = (unsigned long)vsp;
+
+ return URC_OK;
+}
+
+static int unwind_exec_pop_r4_to_rN(struct unwind_ctrl_block *ctrl,
+ unsigned long insn)
+{
+ unsigned long *vsp = (unsigned long *)ctrl->vrs[SP];
+ int reg;
+
+ /* pop R4-R[4+bbb] */
+ for (reg = 4; reg <= 4 + (insn & 7); reg++)
+ if (unwind_pop_register(ctrl, , reg))
+ return -URC_FAILURE;
+
+ if (insn & 0x80)
+ if (unwind_pop_register(ctrl, , 14))
+ return -URC_FAILURE;
+
+ ctrl->vrs[SP] = (unsigned long)vsp;
+
+ return URC_OK;
+}
+
+static int unwind_exec_pop_subset_r0_to_r3(struct unwind_ctrl_block *ctrl,
+ unsigned long mask)
+{
+ unsigned long *vsp = (unsigned long *)ctrl->vrs[SP];
+ int reg = 0;
+
+ /* pop R0-R3 according to mask */
+ while (mask) {
+ if (mask & 1)
+ if (unwind_pop_register(ctrl, , reg))
+ return -URC_FAILURE;
+ mask >>= 1;
+ reg++;
+ }
+ ctrl->vrs[SP] = (unsigned long)vsp;
+
+ return URC_OK;
+}
+
/*
* Execute the current unwind instruction.
*/
static int unwind_exec_insn(struct unwind_ctrl_block *ctrl)
{
unsigned long insn = unwind_get_byte(ctrl);
+ int ret = URC_OK;
pr_debug("%s: insn = %08lx\n", __func__, insn);
@@ -250,8 +327,6 @@ static int unwind_exec_insn(struct unwind_ctrl_block *ctrl)
ctrl->vrs[SP] -= ((insn & 0x3f) << 2) + 4;
else if ((insn & 0xf0) == 0x80) {
unsigned long mask;
- unsigned long *vsp = (unsigned long *)ctrl->vrs[SP];
- int load_sp, reg = 4;
insn = (insn << 8) | unwind_get_byte(ctrl);
mask = insn & 0x0fff;
@@ -261,29 +336,16 @@ static int unwind_exec_insn(struct unwind_ctrl_block
*ctrl)
return -URC_FAILURE;
}
- /* pop R4-R15 according to mask */
- load_sp = mask & (1 << (13 - 4));
- while (mask) {
- if (mask & 1)
- ctrl->vrs[reg] =
[PATCH] ARM : unwinder : Prevent data abort due to stack overflow
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.anu...@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 */
int byte; /* current byte number in the
instructions word */
};
@@ -235,12 +239,85 @@ static unsigned long unwind_get_byte(struct
unwind_ctrl_block *ctrl)
return ret;
}
+/* Before poping a register check whether it is feasible or not */
+static int unwind_pop_register(struct unwind_ctrl_block *ctrl,
+ unsigned long **vsp, unsigned int reg)
+{
+ if (unlikely(ctrl-last_register_set))
+ if (*vsp = (unsigned long *)ctrl-sp_high)
+ return -URC_FAILURE;
+
+ ctrl-vrs[reg] = *(*vsp)++;
+ return URC_OK;
+}
+
+/* Helper functions to execute the instructions */
+static int unwind_exec_pop_subset_r4_to_r13(struct unwind_ctrl_block *ctrl,
+ unsigned long mask)
+{
+ unsigned long *vsp = (unsigned long *)ctrl-vrs[SP];
+ int load_sp, reg = 4;
+
+ load_sp = mask (1 (13 - 4));
+ while (mask) {
+ if (mask 1)
+ if (unwind_pop_register(ctrl, vsp, reg))
+ return -URC_FAILURE;
+ mask = 1;
+ reg++;
+ }
+ if (!load_sp)
+ ctrl-vrs[SP] = (unsigned long)vsp;
+
+ return URC_OK;
+}
+
+static int unwind_exec_pop_r4_to_rN(struct unwind_ctrl_block *ctrl,
+ unsigned long insn)
+{
+ unsigned long *vsp = (unsigned long *)ctrl-vrs[SP];
+ int reg;
+
+ /* pop R4-R[4+bbb] */
+ for (reg = 4; reg = 4 + (insn 7); reg++)
+ if (unwind_pop_register(ctrl, vsp, reg))
+ return -URC_FAILURE;
+
+ if (insn 0x80)
+ if (unwind_pop_register(ctrl, vsp, 14))
+ return -URC_FAILURE;
+
+ ctrl-vrs[SP] = (unsigned long)vsp;
+
+ return URC_OK;
+}
+
+static int unwind_exec_pop_subset_r0_to_r3(struct unwind_ctrl_block *ctrl,
+ unsigned long mask)
+{
+ unsigned long *vsp = (unsigned long *)ctrl-vrs[SP];
+ int reg = 0;
+
+ /* pop R0-R3 according to mask */
+ while (mask) {
+ if (mask 1)
+ if (unwind_pop_register(ctrl, vsp, reg))
+ return -URC_FAILURE;
+ mask = 1;
+ reg++;
+ }
+ ctrl-vrs[SP] = (unsigned long)vsp;
+
+ return URC_OK;
+}
+
/*
* Execute the current unwind instruction.
*/
static int unwind_exec_insn(struct unwind_ctrl_block *ctrl)
{
unsigned long insn = unwind_get_byte(ctrl);
+ int ret = URC_OK;
pr_debug(%s: insn = %08lx\n, __func__, insn);
@@ -250,8 +327,6 @@ static int unwind_exec_insn(struct unwind_ctrl_block *ctrl)
ctrl-vrs[SP] -= ((insn 0x3f) 2) + 4;
else if ((insn 0xf0) == 0x80) {
unsigned long mask;
- unsigned long *vsp = (unsigned long *)ctrl-vrs[SP];
- int load_sp, reg = 4;
insn = (insn 8) | unwind_get_byte(ctrl);
mask = insn 0x0fff;
@@ -261,29 +336,16 @@ static int unwind_exec_insn(struct unwind_ctrl_block
*ctrl)
return -URC_FAILURE;
}
- /* pop R4-R15 according to mask */
- load_sp = mask (1 (13 - 4));
- while (mask) {
- if (mask 1)
-
Re: [PATCH] ARM : unwinder : Prevent data abort due to stack overflow in unwind_exec_insn Signed-off-by: Anurag Aggarwal
Hi Dave,
I aplogize for wrong formatting of multiline comments.
> I really think this shouldn't be separated out in this way, because it
> means the decoder has to be implemented twice, and moving the checks far
> away from the code that the checks need to match.
I believe that you are right in this case, it requires decoder to be
implemented twice. I will seperate the function and correct comments
and send a new patch.
>Although this appears safe, I wonder whether it just creates additional
>ways for the code to be wrong, without providing much optimisation.
>(For example, the maximum number of registers that can be read is
>not actually TOTAL_REGISTERS.)
In this case I believe that for the instructions that can cause data abort
are reading at max 13 registers (which is less than TOTAL_REGISTERS) currently
this is what I was able to understand from the documentation I could find and t
he current code written.
So I believe that this condition will provide good optimization and will not
create
additional ways for the code to go wrong
Regards
Anurag Aggarwal
--- Original Message ---
Sender : Dave Martin
Date : Nov 29, 2013 01:54 (GMT+05:30)
Title : Re: [PATCH] ARM : unwinder : Prevent data abort due to stack overflow
in unwind_exec_insn Signed-off-by: Anurag Aggarwal
On Thu, Nov 28, 2013 at 03:57:19PM +0530, Anurag Aggarwal wrote:
> While executing some unwind instructions stack overflow can cause a data abort
> when area beyond stack is not mapped to physical memory.
>
> To prevent the data abort check whether it is possible to execute
> these instructions before unwinding the stack
> ---
> arch/arm/kernel/unwind.c | 59
> +-
> 1 files changed, 58 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
> index 00df012..3777cd7 100644
> --- a/arch/arm/kernel/unwind.c
> +++ b/arch/arm/kernel/unwind.c
> @@ -49,6 +49,8 @@
> #include
> #include
>
> +#define TOTAL_REGISTERS 16
> +
> /* Dummy functions to avoid linker complaints */
> void __aeabi_unwind_cpp_pr0(void)
> {
> @@ -66,7 +68,7 @@ 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 */
> int entries; /* number of entries left to interpret */
> int byte; /* current byte number in the instructions word */
> @@ -235,6 +237,58 @@ static unsigned long unwind_get_byte(struct
> unwind_ctrl_block *ctrl)
> return ret;
> }
>
> +/* check whether there is enough space on stack to execute instructions
> + that can cause a data abort*/
Nit: strange comment formatting in all your multi-line comments.
/*
* Please format multi-line comments
* like this.
*/
> +static int unwind_check_insn(struct unwind_ctrl_block *ctrl, unsigned long
> insn)
> +{
I really think this shouldn't be separated out in this way, because it
means the decoder has to be implemented twice, and moving the checks far
away from the code that the checks need to match.
Maybe you could refactor the code so that each insn has its own function,
including the check and the execution.
Then
> + unsigned long high, low;
> + int required_stack = 0;
> +
> + low = ctrl->vrs[SP];
> + high = ALIGN(low, THREAD_SIZE);
> +
> + /* check whether we have enough space to extract
> + atleast one set of registers*/
> + if ((high - low) > TOTAL_REGISTERS)
> + return URC_OK;
Although this appears safe, I wonder whether it just creates additional
ways for the code to be wrong, without providing much optimisation.
(For example, the maximum number of registers that can be read is
not actually TOTAL_REGISTERS.)
Cheers
---Dave
> +
> + if ((insn & 0xf0) == 0x80) {
> + unsigned long mask;
> + insn = (insn << 8) | unwind_get_byte(ctrl);
> + mask = insn & 0x0fff;
> + if (mask == 0) {
> + pr_warning("unwind: 'Refuse to unwind' instruction %04lx\n",
> + insn);
> + return -URC_FAILURE;
> + }
> + while (mask) {
> + if (mask & 1)
> + required_stack++;
> + mask >>= 1;
> + }
> + } else if ((insn & 0xf0) == 0xa0) {
> + required_stack += insn & 7;
> + required_stack += (insn & 0x80) ? 1 : 0;
> + } else if (insn == 0xb1) {
> + unsigned long mask = unwind_get_byte(ctrl);
> + if (mask == 0 || mask & 0xf0) {
> + pr_warning("unwind: Spare encoding %04lx\n",
> + (insn << 8) | mask);
> + return -URC_FAILURE;
> + }
> + while (mask) {
> + if (mask & 1)
> + required_stack++;
> + mask >>= 1;
> + }
> + }
>
Re: [PATCH] ARM : unwinder : Prevent data abort due to stack overflow in unwind_exec_insn Signed-off-by: Anurag Aggarwal
On Thu, Nov 28, 2013 at 03:57:19PM +0530, Anurag Aggarwal wrote:
> While executing some unwind instructions stack overflow can cause a data abort
> when area beyond stack is not mapped to physical memory.
>
> To prevent the data abort check whether it is possible to execute
> these instructions before unwinding the stack
> ---
> arch/arm/kernel/unwind.c | 59
> +-
> 1 files changed, 58 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
> index 00df012..3777cd7 100644
> --- a/arch/arm/kernel/unwind.c
> +++ b/arch/arm/kernel/unwind.c
> @@ -49,6 +49,8 @@
> #include
> #include
>
> +#define TOTAL_REGISTERS 16
> +
> /* Dummy functions to avoid linker complaints */
> void __aeabi_unwind_cpp_pr0(void)
> {
> @@ -66,7 +68,7 @@ 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 */
> int entries;/* number of entries left to interpret
> */
> int byte; /* current byte number in the
> instructions word */
> @@ -235,6 +237,58 @@ static unsigned long unwind_get_byte(struct
> unwind_ctrl_block *ctrl)
> return ret;
> }
>
> +/* check whether there is enough space on stack to execute instructions
> + that can cause a data abort*/
Nit: strange comment formatting in all your multi-line comments.
/*
* Please format multi-line comments
* like this.
*/
> +static int unwind_check_insn(struct unwind_ctrl_block *ctrl, unsigned long
> insn)
> +{
I really think this shouldn't be separated out in this way, because it
means the decoder has to be implemented twice, and moving the checks far
away from the code that the checks need to match.
Maybe you could refactor the code so that each insn has its own function,
including the check and the execution.
Then
> + unsigned long high, low;
> + int required_stack = 0;
> +
> + low = ctrl->vrs[SP];
> + high = ALIGN(low, THREAD_SIZE);
> +
> + /* check whether we have enough space to extract
> + atleast one set of registers*/
> + if ((high - low) > TOTAL_REGISTERS)
> + return URC_OK;
Although this appears safe, I wonder whether it just creates additional
ways for the code to be wrong, without providing much optimisation.
(For example, the maximum number of registers that can be read is
not actually TOTAL_REGISTERS.)
Cheers
---Dave
> +
> + if ((insn & 0xf0) == 0x80) {
> + unsigned long mask;
> + insn = (insn << 8) | unwind_get_byte(ctrl);
> + mask = insn & 0x0fff;
> + if (mask == 0) {
> + pr_warning("unwind: 'Refuse to unwind' instruction
> %04lx\n",
> + insn);
> + return -URC_FAILURE;
> + }
> + while (mask) {
> + if (mask & 1)
> + required_stack++;
> + mask >>= 1;
> + }
> + } else if ((insn & 0xf0) == 0xa0) {
> + required_stack += insn & 7;
> + required_stack += (insn & 0x80) ? 1 : 0;
> + } else if (insn == 0xb1) {
> + unsigned long mask = unwind_get_byte(ctrl);
> + if (mask == 0 || mask & 0xf0) {
> + pr_warning("unwind: Spare encoding %04lx\n",
> + (insn << 8) | mask);
> + return -URC_FAILURE;
> + }
> + while (mask) {
> + if (mask & 1)
> + required_stack++;
> + mask >>= 1;
> + }
> + }
> +
> + if ((high - low) < required_stack)
> + return -URC_FAILURE;
> +
> + return URC_OK;
> +}
> +
> /*
> * Execute the current unwind instruction.
> */
> @@ -244,6 +298,9 @@ static int unwind_exec_insn(struct unwind_ctrl_block
> *ctrl)
>
> pr_debug("%s: insn = %08lx\n", __func__, insn);
>
> + if (unwind_check_insn(ctrl, insn) < 0)
> + return -URC_FAILURE;
> +
> if ((insn & 0xc0) == 0x00)
> ctrl->vrs[SP] += ((insn & 0x3f) << 2) + 4;
> else if ((insn & 0xc0) == 0x40)
> --
> 1.7.0.4
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] ARM : unwinder : Prevent data abort due to stack overflow in unwind_exec_insn Signed-off-by: Anurag Aggarwal
While executing some unwind instructions stack overflow can cause a data abort
when area beyond stack is not mapped to physical memory.
To prevent the data abort check whether it is possible to execute
these instructions before unwinding the stack
---
arch/arm/kernel/unwind.c | 59 +-
1 files changed, 58 insertions(+), 1 deletions(-)
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 00df012..3777cd7 100644
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -49,6 +49,8 @@
#include
#include
+#define TOTAL_REGISTERS 16
+
/* Dummy functions to avoid linker complaints */
void __aeabi_unwind_cpp_pr0(void)
{
@@ -66,7 +68,7 @@ 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 */
int entries;/* number of entries left to interpret
*/
int byte; /* current byte number in the
instructions word */
@@ -235,6 +237,58 @@ static unsigned long unwind_get_byte(struct
unwind_ctrl_block *ctrl)
return ret;
}
+/* check whether there is enough space on stack to execute instructions
+ that can cause a data abort*/
+static int unwind_check_insn(struct unwind_ctrl_block *ctrl, unsigned long
insn)
+{
+ unsigned long high, low;
+ int required_stack = 0;
+
+ low = ctrl->vrs[SP];
+ high = ALIGN(low, THREAD_SIZE);
+
+ /* check whether we have enough space to extract
+ atleast one set of registers*/
+ if ((high - low) > TOTAL_REGISTERS)
+ return URC_OK;
+
+ if ((insn & 0xf0) == 0x80) {
+ unsigned long mask;
+ insn = (insn << 8) | unwind_get_byte(ctrl);
+ mask = insn & 0x0fff;
+ if (mask == 0) {
+ pr_warning("unwind: 'Refuse to unwind' instruction
%04lx\n",
+ insn);
+ return -URC_FAILURE;
+ }
+ while (mask) {
+ if (mask & 1)
+ required_stack++;
+ mask >>= 1;
+ }
+ } else if ((insn & 0xf0) == 0xa0) {
+ required_stack += insn & 7;
+ required_stack += (insn & 0x80) ? 1 : 0;
+ } else if (insn == 0xb1) {
+ unsigned long mask = unwind_get_byte(ctrl);
+ if (mask == 0 || mask & 0xf0) {
+ pr_warning("unwind: Spare encoding %04lx\n",
+ (insn << 8) | mask);
+ return -URC_FAILURE;
+ }
+ while (mask) {
+ if (mask & 1)
+ required_stack++;
+ mask >>= 1;
+ }
+ }
+
+ if ((high - low) < required_stack)
+ return -URC_FAILURE;
+
+ return URC_OK;
+}
+
/*
* Execute the current unwind instruction.
*/
@@ -244,6 +298,9 @@ static int unwind_exec_insn(struct unwind_ctrl_block *ctrl)
pr_debug("%s: insn = %08lx\n", __func__, insn);
+ if (unwind_check_insn(ctrl, insn) < 0)
+ return -URC_FAILURE;
+
if ((insn & 0xc0) == 0x00)
ctrl->vrs[SP] += ((insn & 0x3f) << 2) + 4;
else if ((insn & 0xc0) == 0x40)
--
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] ARM : unwinder : Prevent data abort due to stack overflow in unwind_exec_insn Signed-off-by: Anurag Aggarwal a.anu...@samsung.com
While executing some unwind instructions stack overflow can cause a data abort
when area beyond stack is not mapped to physical memory.
To prevent the data abort check whether it is possible to execute
these instructions before unwinding the stack
---
arch/arm/kernel/unwind.c | 59 +-
1 files changed, 58 insertions(+), 1 deletions(-)
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 00df012..3777cd7 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,7 +68,7 @@ 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 */
int entries;/* number of entries left to interpret
*/
int byte; /* current byte number in the
instructions word */
@@ -235,6 +237,58 @@ static unsigned long unwind_get_byte(struct
unwind_ctrl_block *ctrl)
return ret;
}
+/* check whether there is enough space on stack to execute instructions
+ that can cause a data abort*/
+static int unwind_check_insn(struct unwind_ctrl_block *ctrl, unsigned long
insn)
+{
+ unsigned long high, low;
+ int required_stack = 0;
+
+ low = ctrl-vrs[SP];
+ high = ALIGN(low, THREAD_SIZE);
+
+ /* check whether we have enough space to extract
+ atleast one set of registers*/
+ if ((high - low) TOTAL_REGISTERS)
+ return URC_OK;
+
+ if ((insn 0xf0) == 0x80) {
+ unsigned long mask;
+ insn = (insn 8) | unwind_get_byte(ctrl);
+ mask = insn 0x0fff;
+ if (mask == 0) {
+ pr_warning(unwind: 'Refuse to unwind' instruction
%04lx\n,
+ insn);
+ return -URC_FAILURE;
+ }
+ while (mask) {
+ if (mask 1)
+ required_stack++;
+ mask = 1;
+ }
+ } else if ((insn 0xf0) == 0xa0) {
+ required_stack += insn 7;
+ required_stack += (insn 0x80) ? 1 : 0;
+ } else if (insn == 0xb1) {
+ unsigned long mask = unwind_get_byte(ctrl);
+ if (mask == 0 || mask 0xf0) {
+ pr_warning(unwind: Spare encoding %04lx\n,
+ (insn 8) | mask);
+ return -URC_FAILURE;
+ }
+ while (mask) {
+ if (mask 1)
+ required_stack++;
+ mask = 1;
+ }
+ }
+
+ if ((high - low) required_stack)
+ return -URC_FAILURE;
+
+ return URC_OK;
+}
+
/*
* Execute the current unwind instruction.
*/
@@ -244,6 +298,9 @@ static int unwind_exec_insn(struct unwind_ctrl_block *ctrl)
pr_debug(%s: insn = %08lx\n, __func__, insn);
+ if (unwind_check_insn(ctrl, insn) 0)
+ return -URC_FAILURE;
+
if ((insn 0xc0) == 0x00)
ctrl-vrs[SP] += ((insn 0x3f) 2) + 4;
else if ((insn 0xc0) == 0x40)
--
1.7.0.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM : unwinder : Prevent data abort due to stack overflow in unwind_exec_insn Signed-off-by: Anurag Aggarwal a.anu...@samsung.com
On Thu, Nov 28, 2013 at 03:57:19PM +0530, Anurag Aggarwal wrote:
While executing some unwind instructions stack overflow can cause a data abort
when area beyond stack is not mapped to physical memory.
To prevent the data abort check whether it is possible to execute
these instructions before unwinding the stack
---
arch/arm/kernel/unwind.c | 59
+-
1 files changed, 58 insertions(+), 1 deletions(-)
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 00df012..3777cd7 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,7 +68,7 @@ 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 */
int entries;/* number of entries left to interpret
*/
int byte; /* current byte number in the
instructions word */
@@ -235,6 +237,58 @@ static unsigned long unwind_get_byte(struct
unwind_ctrl_block *ctrl)
return ret;
}
+/* check whether there is enough space on stack to execute instructions
+ that can cause a data abort*/
Nit: strange comment formatting in all your multi-line comments.
/*
* Please format multi-line comments
* like this.
*/
+static int unwind_check_insn(struct unwind_ctrl_block *ctrl, unsigned long
insn)
+{
I really think this shouldn't be separated out in this way, because it
means the decoder has to be implemented twice, and moving the checks far
away from the code that the checks need to match.
Maybe you could refactor the code so that each insn has its own function,
including the check and the execution.
Then
+ unsigned long high, low;
+ int required_stack = 0;
+
+ low = ctrl-vrs[SP];
+ high = ALIGN(low, THREAD_SIZE);
+
+ /* check whether we have enough space to extract
+ atleast one set of registers*/
+ if ((high - low) TOTAL_REGISTERS)
+ return URC_OK;
Although this appears safe, I wonder whether it just creates additional
ways for the code to be wrong, without providing much optimisation.
(For example, the maximum number of registers that can be read is
not actually TOTAL_REGISTERS.)
Cheers
---Dave
+
+ if ((insn 0xf0) == 0x80) {
+ unsigned long mask;
+ insn = (insn 8) | unwind_get_byte(ctrl);
+ mask = insn 0x0fff;
+ if (mask == 0) {
+ pr_warning(unwind: 'Refuse to unwind' instruction
%04lx\n,
+ insn);
+ return -URC_FAILURE;
+ }
+ while (mask) {
+ if (mask 1)
+ required_stack++;
+ mask = 1;
+ }
+ } else if ((insn 0xf0) == 0xa0) {
+ required_stack += insn 7;
+ required_stack += (insn 0x80) ? 1 : 0;
+ } else if (insn == 0xb1) {
+ unsigned long mask = unwind_get_byte(ctrl);
+ if (mask == 0 || mask 0xf0) {
+ pr_warning(unwind: Spare encoding %04lx\n,
+ (insn 8) | mask);
+ return -URC_FAILURE;
+ }
+ while (mask) {
+ if (mask 1)
+ required_stack++;
+ mask = 1;
+ }
+ }
+
+ if ((high - low) required_stack)
+ return -URC_FAILURE;
+
+ return URC_OK;
+}
+
/*
* Execute the current unwind instruction.
*/
@@ -244,6 +298,9 @@ static int unwind_exec_insn(struct unwind_ctrl_block
*ctrl)
pr_debug(%s: insn = %08lx\n, __func__, insn);
+ if (unwind_check_insn(ctrl, insn) 0)
+ return -URC_FAILURE;
+
if ((insn 0xc0) == 0x00)
ctrl-vrs[SP] += ((insn 0x3f) 2) + 4;
else if ((insn 0xc0) == 0x40)
--
1.7.0.4
___
linux-arm-kernel mailing list
linux-arm-ker...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM : unwinder : Prevent data abort due to stack overflow in unwind_exec_insn Signed-off-by: Anurag Aggarwal a.anu...@samsung.com
Hi Dave,
I aplogize for wrong formatting of multiline comments.
I really think this shouldn't be separated out in this way, because it
means the decoder has to be implemented twice, and moving the checks far
away from the code that the checks need to match.
I believe that you are right in this case, it requires decoder to be
implemented twice. I will seperate the function and correct comments
and send a new patch.
Although this appears safe, I wonder whether it just creates additional
ways for the code to be wrong, without providing much optimisation.
(For example, the maximum number of registers that can be read is
not actually TOTAL_REGISTERS.)
In this case I believe that for the instructions that can cause data abort
are reading at max 13 registers (which is less than TOTAL_REGISTERS) currently
this is what I was able to understand from the documentation I could find and t
he current code written.
So I believe that this condition will provide good optimization and will not
create
additional ways for the code to go wrong
Regards
Anurag Aggarwal
--- Original Message ---
Sender : Dave Martindave.mar...@arm.com
Date : Nov 29, 2013 01:54 (GMT+05:30)
Title : Re: [PATCH] ARM : unwinder : Prevent data abort due to stack overflow
in unwind_exec_insn Signed-off-by: Anurag Aggarwal
On Thu, Nov 28, 2013 at 03:57:19PM +0530, Anurag Aggarwal wrote:
While executing some unwind instructions stack overflow can cause a data abort
when area beyond stack is not mapped to physical memory.
To prevent the data abort check whether it is possible to execute
these instructions before unwinding the stack
---
arch/arm/kernel/unwind.c | 59
+-
1 files changed, 58 insertions(+), 1 deletions(-)
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 00df012..3777cd7 100644
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -49,6 +49,8 @@
#include
#include
+#define TOTAL_REGISTERS 16
+
/* Dummy functions to avoid linker complaints */
void __aeabi_unwind_cpp_pr0(void)
{
@@ -66,7 +68,7 @@ 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 */
int entries; /* number of entries left to interpret */
int byte; /* current byte number in the instructions word */
@@ -235,6 +237,58 @@ static unsigned long unwind_get_byte(struct
unwind_ctrl_block *ctrl)
return ret;
}
+/* check whether there is enough space on stack to execute instructions
+ that can cause a data abort*/
Nit: strange comment formatting in all your multi-line comments.
/*
* Please format multi-line comments
* like this.
*/
+static int unwind_check_insn(struct unwind_ctrl_block *ctrl, unsigned long
insn)
+{
I really think this shouldn't be separated out in this way, because it
means the decoder has to be implemented twice, and moving the checks far
away from the code that the checks need to match.
Maybe you could refactor the code so that each insn has its own function,
including the check and the execution.
Then
+ unsigned long high, low;
+ int required_stack = 0;
+
+ low = ctrl-vrs[SP];
+ high = ALIGN(low, THREAD_SIZE);
+
+ /* check whether we have enough space to extract
+ atleast one set of registers*/
+ if ((high - low) TOTAL_REGISTERS)
+ return URC_OK;
Although this appears safe, I wonder whether it just creates additional
ways for the code to be wrong, without providing much optimisation.
(For example, the maximum number of registers that can be read is
not actually TOTAL_REGISTERS.)
Cheers
---Dave
+
+ if ((insn 0xf0) == 0x80) {
+ unsigned long mask;
+ insn = (insn 8) | unwind_get_byte(ctrl);
+ mask = insn 0x0fff;
+ if (mask == 0) {
+ pr_warning(unwind: 'Refuse to unwind' instruction %04lx\n,
+ insn);
+ return -URC_FAILURE;
+ }
+ while (mask) {
+ if (mask 1)
+ required_stack++;
+ mask = 1;
+ }
+ } else if ((insn 0xf0) == 0xa0) {
+ required_stack += insn 7;
+ required_stack += (insn 0x80) ? 1 : 0;
+ } else if (insn == 0xb1) {
+ unsigned long mask = unwind_get_byte(ctrl);
+ if (mask == 0 || mask 0xf0) {
+ pr_warning(unwind: Spare encoding %04lx\n,
+ (insn 8) | mask);
+ return -URC_FAILURE;
+ }
+ while (mask) {
+ if (mask 1)
+ required_stack++;
+ mask = 1;
+ }
+ }
+
+ if ((high - low) required_stack)
+ return -URC_FAILURE;
+
+ return URC_OK;
+}
+
/*
* Execute the current unwind instruction.
*/
@@ -244,6 +298,9 @@ static int unwind_exec_insn(struct unwind_ctrl_block
*ctrl)
pr_debug(%s: insn = %08lx\n, __func__, insn);
+ if (unwind_check_insn(ctrl, insn) 0)
+ return -URC_FAILURE;
+
if ((insn 0xc0) == 0x00)
ctrl-vrs[SP] += ((insn 0x3f) 2) + 4;
else if ((insn 0xc0
