[no subject]
unsubscribe linux-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/
Fwd:
unsubscribe linux-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/
[no subject]
unsubscribe linux-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/
Fwd:
unsubscribe linux-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 V7] ARM : unwinder : Prevent data abort due to stack overflow
Any more update on the patch.? -- 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 V7] ARM : unwinder : Prevent data abort due to stack overflow
Any more update on the patch.? -- 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 V7] 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
Reviewed-by: Dave Martin
---
arch/arm/kernel/unwind.c | 137 +-
1 file changed, 100 insertions(+), 37 deletions(-)
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 00df012..3c21769 100644
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -68,6 +68,12 @@ EXPORT_SYMBOL(__aeabi_unwind_cpp_pr2);
struct unwind_ctrl_block {
unsigned long vrs[16]; /* virtual register set */
const unsigned long *insn; /* pointer to the current instructions
word */
+ unsigned long sp_high; /* highest value of sp allowed */
+ /*
+* 1 : check for stack overflow for each register pop.
+* 0 : save overhead if there is plenty of stack remaining.
+*/
+ int check_each_pop;
int entries;/* number of entries left to interpret
*/
int byte; /* current byte number in the
instructions word */
};
@@ -235,12 +241,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->check_each_pop))
+ 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 +329,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 +338,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] = *vsp++;
-
[PATCH V7] 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
Reviewed-by: Dave Martin dave.mar...@arm.com
---
arch/arm/kernel/unwind.c | 137 +-
1 file changed, 100 insertions(+), 37 deletions(-)
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 00df012..3c21769 100644
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -68,6 +68,12 @@ EXPORT_SYMBOL(__aeabi_unwind_cpp_pr2);
struct unwind_ctrl_block {
unsigned long vrs[16]; /* virtual register set */
const unsigned long *insn; /* pointer to the current instructions
word */
+ unsigned long sp_high; /* highest value of sp allowed */
+ /*
+* 1 : check for stack overflow for each register pop.
+* 0 : save overhead if there is plenty of stack remaining.
+*/
+ int check_each_pop;
int entries;/* number of entries left to interpret
*/
int byte; /* current byte number in the
instructions word */
};
@@ -235,12 +241,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-check_each_pop))
+ 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 +329,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 +338,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] = *vsp++;
- mask = 1;
- reg++;
- }
- if (!load_sp
Re: [PATCH V6] ARM : unwinder : Prevent data abort due to stack overflow
>You could try adding some debug printks to see how the backtrace fails.
>You could also try adding a few hand-crafted assembler functions
>with appropriate code and unwind directives to trigger different kinds
>of backtrace failure. You might have to add a way to artificially limit
>sp_high to check the cases where you run out of stack in the middle of
>popping multiple registers.
I added a a printk statement
+ if (*vsp >= (unsigned long *)ctrl->sp_high) {
+ printk(KERN_ERR "Stack Overflow Detected, vsp = %lx",
+ (unsigned long)*vsp);
+ return -URC_FAILURE;
+ }
I ran a many test cases to try and get the above print in the dmesg log.
I tried the following things :
1) Calling unwind_backtrace from diffrenet locations in the kernel, I
added the unwind call
in some irq, fork, exit and some sysfs entries call.
2) I limited the value of sp_high in unwind_frame() itself, I tried
many values of sp_high,
varrying from (low + sizeof(ctrl.vrs)/4) to (low + 4*sizeof(ctrl.vrs)).
When running the above cases I was able to see the above printk quiet
a few times in dmesg log.
So, the error condition is being handled.
If you have some test cases for verifying the unwinder, please share
the same. They might help
in thorough testing of unwinder.
Regards
On Wed, Dec 11, 2013 at 3:10 PM, Anurag Aggarwal wrote:
>>You could try adding some debug printks to see how the backtrace fails.
>>You could also try adding a few hand-crafted assembler functions
>>with appropriate code and unwind directives to trigger different kinds
>>of backtrace failure. You might have to add a way to artificially limit
>>sp_high to check the cases where you run out of stack in the middle of
>>popping multiple registers.
>
> I added a a printk statement
> + if (*vsp >= (unsigned long *)ctrl->sp_high) {
> + printk(KERN_ERR "Stack Overflow Detected, vsp = %lx",
> + (unsigned long)*vsp);
> + return -URC_FAILURE;
> + }
>
> I ran a many test cases to try and get the above print in the dmesg log.
>
> I tried the following things :
>
> 1) Calling unwind_backtrace from diffrenet locations in the kernel, I added
> the unwind call
> in some irq, fork, exit and some sysfs entries call.
> 2) I limited the value of sp_high in unwind_frame() itself, I tried many
> values of sp_high,
> varrying from (low + sizeof(ctrl.vrs)/4) to (low + 4*sizeof(ctrl.vrs)).
>
> When running the above cases I was able to see the above printk quiet a few
> times in dmesg log.
>
> So, the error condition is being handled.
>
> If you have some test cases for verifying the unwinder, please share the
> same. They might help
> in thorough testing of unwinder.
>
>
>
> Regards
> Anurag
--
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 V6] ARM : unwinder : Prevent data abort due to stack overflow
You could try adding some debug printks to see how the backtrace fails.
You could also try adding a few hand-crafted assembler functions
with appropriate code and unwind directives to trigger different kinds
of backtrace failure. You might have to add a way to artificially limit
sp_high to check the cases where you run out of stack in the middle of
popping multiple registers.
I added a a printk statement
+ if (*vsp = (unsigned long *)ctrl-sp_high) {
+ printk(KERN_ERR Stack Overflow Detected, vsp = %lx,
+ (unsigned long)*vsp);
+ return -URC_FAILURE;
+ }
I ran a many test cases to try and get the above print in the dmesg log.
I tried the following things :
1) Calling unwind_backtrace from diffrenet locations in the kernel, I
added the unwind call
in some irq, fork, exit and some sysfs entries call.
2) I limited the value of sp_high in unwind_frame() itself, I tried
many values of sp_high,
varrying from (low + sizeof(ctrl.vrs)/4) to (low + 4*sizeof(ctrl.vrs)).
When running the above cases I was able to see the above printk quiet
a few times in dmesg log.
So, the error condition is being handled.
If you have some test cases for verifying the unwinder, please share
the same. They might help
in thorough testing of unwinder.
Regards
On Wed, Dec 11, 2013 at 3:10 PM, Anurag Aggarwal a.anu...@samsung.com wrote:
You could try adding some debug printks to see how the backtrace fails.
You could also try adding a few hand-crafted assembler functions
with appropriate code and unwind directives to trigger different kinds
of backtrace failure. You might have to add a way to artificially limit
sp_high to check the cases where you run out of stack in the middle of
popping multiple registers.
I added a a printk statement
+ if (*vsp = (unsigned long *)ctrl-sp_high) {
+ printk(KERN_ERR Stack Overflow Detected, vsp = %lx,
+ (unsigned long)*vsp);
+ return -URC_FAILURE;
+ }
I ran a many test cases to try and get the above print in the dmesg log.
I tried the following things :
1) Calling unwind_backtrace from diffrenet locations in the kernel, I added
the unwind call
in some irq, fork, exit and some sysfs entries call.
2) I limited the value of sp_high in unwind_frame() itself, I tried many
values of sp_high,
varrying from (low + sizeof(ctrl.vrs)/4) to (low + 4*sizeof(ctrl.vrs)).
When running the above cases I was able to see the above printk quiet a few
times in dmesg log.
So, the error condition is being handled.
If you have some test cases for verifying the unwinder, please share the
same. They might help
in thorough testing of unwinder.
Regards
Anurag
--
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 V6] ARM : unwinder : Prevent data abort due to stack overflow
>You could try adding some debug printks to see how the backtrace fails.
>You could also try adding a few hand-crafted assembler functions
>with appropriate code and unwind directives to trigger different kinds
>of backtrace failure. You might have to add a way to artificially limit
>sp_high to check the cases where you run out of stack in the middle of
>popping multiple registers.
I added a a printk statement
+ if (*vsp >= (unsigned long *)ctrl->sp_high) {
+ printk(KERN_ERR "Stack Overflow Detected, vsp = %lx",
+ (unsigned long)*vsp);
+ return -URC_FAILURE;
+ }
I ran a many test cases to try and get the above print in the dmesg log.
I tried the following things :
1) Calling unwind_backtrace from diffrenet locations in the kernel, I added the
unwind call
in some irq, fork, exit and some sysfs entries call.
2) I limited the value of sp_high in unwind_frame() itself, I tried many values
of sp_high,
varrying from (low + sizeof(ctrl.vrs)/4) to (low + 4*sizeof(ctrl.vrs)).
When running the above cases I was able to see the above printk quiet a few
times in dmesg log.
So, the error condition is being handled.
If you have some test cases for verifying the unwinder, please share the same.
They might help
in thorough testing of unwinder.
Regards
Anurag
Re: [PATCH V6] ARM : unwinder : Prevent data abort due to stack overflow
You could try adding some debug printks to see how the backtrace fails.
You could also try adding a few hand-crafted assembler functions
with appropriate code and unwind directives to trigger different kinds
of backtrace failure. You might have to add a way to artificially limit
sp_high to check the cases where you run out of stack in the middle of
popping multiple registers.
I added a a printk statement
+ if (*vsp = (unsigned long *)ctrl-sp_high) {
+ printk(KERN_ERR Stack Overflow Detected, vsp = %lx,
+ (unsigned long)*vsp);
+ return -URC_FAILURE;
+ }
I ran a many test cases to try and get the above print in the dmesg log.
I tried the following things :
1) Calling unwind_backtrace from diffrenet locations in the kernel, I added the
unwind call
in some irq, fork, exit and some sysfs entries call.
2) I limited the value of sp_high in unwind_frame() itself, I tried many values
of sp_high,
varrying from (low + sizeof(ctrl.vrs)/4) to (low + 4*sizeof(ctrl.vrs)).
When running the above cases I was able to see the above printk quiet a few
times in dmesg log.
So, the error condition is being handled.
If you have some test cases for verifying the unwinder, please share the same.
They might help
in thorough testing of unwinder.
Regards
Anurag
Re: [PATCH V6] ARM : unwinder : Prevent data abort due to stack overflow
>>+ if (*vsp >= (unsigned long *)ctrl->sp_high) >>+ return -URC_FAILURE; I tested the same patch, by adding a printk statement in the above if condition. The print statement I added came a few times as a part of dmesg log. I think this proves that such corner cases are being handled by the above code Regards Anurag On Tue, Dec 10, 2013 at 9:24 AM, Anurag Aggarwal wrote: >>Reviewed-by: Dave Martin >> >>I can confirm that the kernel "doesn't crash" with this applied, and >>that backtracing at least partially works. But this is not really >>sufficient to demontrate that the now code works better than the old >>code in corner cases (which is the point of the patch). >> >>Can you give details of what additional testing you have, or plan to >>do? > > We saw a data abort in unwinder for one of Samsung Project, during a > Samsung Automation test case. > After that I created the initial the patch, and the data abort has not been > seen till now. > > Is it possible for you to give an idea on what other kind of additional > testing > do you have in mind. > > Regads > Anurag > -- 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 V6] ARM : unwinder : Prevent data abort due to stack overflow
+ if (*vsp = (unsigned long *)ctrl-sp_high) + return -URC_FAILURE; I tested the same patch, by adding a printk statement in the above if condition. The print statement I added came a few times as a part of dmesg log. I think this proves that such corner cases are being handled by the above code Regards Anurag On Tue, Dec 10, 2013 at 9:24 AM, Anurag Aggarwal a.anu...@samsung.com wrote: Reviewed-by: Dave Martin dave.mar...@arm.com I can confirm that the kernel doesn't crash with this applied, and that backtracing at least partially works. But this is not really sufficient to demontrate that the now code works better than the old code in corner cases (which is the point of the patch). Can you give details of what additional testing you have, or plan to do? We saw a data abort in unwinder for one of Samsung Project, during a Samsung Automation test case. After that I created the initial the patch, and the data abort has not been seen till now. Is it possible for you to give an idea on what other kind of additional testing do you have in mind. Regads Anurag -- 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 V6] ARM : unwinder : Prevent data abort due to stack overflow
>Reviewed-by: Dave Martin > >I can confirm that the kernel "doesn't crash" with this applied, and >that backtracing at least partially works. But this is not really >sufficient to demontrate that the now code works better than the old >code in corner cases (which is the point of the patch). > >Can you give details of what additional testing you have, or plan to >do? We saw a data abort in unwinder for one of Samsung Project, during a Samsung Automation test case. After that I created the initial the patch, and the data abort has not been seen till now. Is it possible for you to give an idea on what other kind of additional testing do you have in mind. Regads Anurag
[PATCH V6] 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 | 137 +
1 files changed, 100 insertions(+), 37 deletions(-)
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 00df012..3c21769 100644
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -68,6 +68,12 @@ EXPORT_SYMBOL(__aeabi_unwind_cpp_pr2);
struct unwind_ctrl_block {
unsigned long vrs[16]; /* virtual register set */
const unsigned long *insn; /* pointer to the current instructions
word */
+ unsigned long sp_high; /* highest value of sp allowed */
+ /*
+* 1 : check for stack overflow for each register pop.
+* 0 : save overhead if there is plenty of stack remaining.
+*/
+ int check_each_pop;
int entries;/* number of entries left to interpret
*/
int byte; /* current byte number in the
instructions word */
};
@@ -235,12 +241,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->check_each_pop))
+ 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 +329,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 +338,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] = *vsp++;
- mask >>= 1;
[PATCH V6] 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 | 137 +
1 files changed, 100 insertions(+), 37 deletions(-)
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 00df012..3c21769 100644
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -68,6 +68,12 @@ EXPORT_SYMBOL(__aeabi_unwind_cpp_pr2);
struct unwind_ctrl_block {
unsigned long vrs[16]; /* virtual register set */
const unsigned long *insn; /* pointer to the current instructions
word */
+ unsigned long sp_high; /* highest value of sp allowed */
+ /*
+* 1 : check for stack overflow for each register pop.
+* 0 : save overhead if there is plenty of stack remaining.
+*/
+ int check_each_pop;
int entries;/* number of entries left to interpret
*/
int byte; /* current byte number in the
instructions word */
};
@@ -235,12 +241,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-check_each_pop))
+ 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 +329,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 +338,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] = *vsp++;
- mask = 1;
- reg++;
- }
- if (!load_sp)
- ctrl-vrs[SP] = (unsigned long)vsp
Re: [PATCH V6] ARM : unwinder : Prevent data abort due to stack overflow
Reviewed-by: Dave Martin dave.mar...@arm.com I can confirm that the kernel doesn't crash with this applied, and that backtracing at least partially works. But this is not really sufficient to demontrate that the now code works better than the old code in corner cases (which is the point of the patch). Can you give details of what additional testing you have, or plan to do? We saw a data abort in unwinder for one of Samsung Project, during a Samsung Automation test case. After that I created the initial the patch, and the data abort has not been seen till now. Is it possible for you to give an idea on what other kind of additional testing do you have in mind. Regads Anurag
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/
[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));
-
Re: [PATCH V4] ARM : unwinder : Prevent data abort due to stack overflow
>>if (unlikely(check) &&
>>*vsp >= (unsigned long *)ctrl->sp_high))
Good idea, It can help in optimizing the code and will leave code more readable
and it will be easy to maintain also.
>>Does the code check anywhere that SP is word-aligned?
>>
>>That should probably be checked if it's not done already.
I think this should be handled in separate patch
I would also like to hear more your ideas for the file
Regards
Anurag
On Thu, Dec 5, 2013 at 7:29 PM, Dave Martin wrote:
> On Thu, Dec 05, 2013 at 11:26:25AM +, 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 | 127
>> -
>> 1 files changed, 90 insertions(+), 37 deletions(-)
>>
>> diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
>> index 00df012..94f6ef4 100644
>> --- a/arch/arm/kernel/unwind.c
>> +++ b/arch/arm/kernel/unwind.c
>> @@ -68,6 +68,7 @@ EXPORT_SYMBOL(__aeabi_unwind_cpp_pr2);
>> struct unwind_ctrl_block {
>> unsigned long vrs[16]; /* 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 byte; /* current byte number in the
>> instructions word */
>> };
>> @@ -235,6 +236,86 @@ 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 (*vsp >= (unsigned long *)ctrl->sp_high)
>> + return -URC_FAILURE;
>> +
>> + ctrl->vrs[reg] = *(*vsp)++;
>> + return URC_OK;
>
> It occurred to me that your optimisation can still be implemented on
> top on this.
>
> If you add an extra flag parameter to unwind_pop_register telling it
> whether to do checking or not, I think that the compiler and/or
> CPU branch predictor should be able to do a pretty good job of
> optimising the common case. Until we get near sp_high, if(check) will
> branch the same way every time.
>
> if (unlikely(check) &&
> *vsp >= (unsigned long *)ctrl->sp_high))
>
> would make sense, in fact.
>
>
> I think this brings most of the benefit, without needing to code the
> insn exec rules twice.
>
> I'm still not sure the optimisation benefits us much, but I think
> that would be a tidier way of doing it if you want to try.
>
>> +}
>> +
>> +/* 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;
>> +
>> + pr_debug("%s: fp = %08lx sp = %08lx lr = %08lx pc = %08lx\n", __func__,
>> + ctrl->vrs[FP], ctrl->vrs[SP], ctrl->vrs[LR], ctrl->vrs[PC]);
>
> Minor-ish nit: you now duplicate this same pr_debug() in many places.
> Apologies, I didn't spot that in the previous review.
>
> What about something like this:
>
> static int unwind_exec_insn(...)
> {
> int ret = URC_OK;
>
> } else if (...)
> ret = unwind_exec_pop_subset_r4_to_r13(...);
> if (ret)
> goto error;
> else ...
>
> pr_debug(...);
>
> error:
> return
[PATCH V4] 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 | 127 -
1 files changed, 90 insertions(+), 37 deletions(-)
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 00df012..94f6ef4 100644
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -68,6 +68,7 @@ EXPORT_SYMBOL(__aeabi_unwind_cpp_pr2);
struct unwind_ctrl_block {
unsigned long vrs[16]; /* 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 byte; /* current byte number in the
instructions word */
};
@@ -235,6 +236,86 @@ 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 (*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;
+
+ pr_debug("%s: fp = %08lx sp = %08lx lr = %08lx pc = %08lx\n", __func__,
+ ctrl->vrs[FP], ctrl->vrs[SP], ctrl->vrs[LR], ctrl->vrs[PC]);
+
+ 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;
+
+ pr_debug("%s: fp = %08lx sp = %08lx lr = %08lx pc = %08lx\n", __func__,
+ ctrl->vrs[FP], ctrl->vrs[SP], ctrl->vrs[LR], ctrl->vrs[PC]);
+
+ 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;
+
+ pr_debug("%s: fp = %08lx sp = %08lx lr = %08lx pc = %08lx\n", __func__,
+ ctrl->vrs[FP], ctrl->vrs[SP], ctrl->vrs[LR], ctrl->vrs[PC]);
+
+ return URC_OK;
+}
+
/*
* Execute the current unwind instruction.
*/
@@ -250,8 +331,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,38 +340,19 @@ 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 &
[PATCH V4] 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 | 127 -
1 files changed, 90 insertions(+), 37 deletions(-)
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 00df012..94f6ef4 100644
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -68,6 +68,7 @@ EXPORT_SYMBOL(__aeabi_unwind_cpp_pr2);
struct unwind_ctrl_block {
unsigned long vrs[16]; /* 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 byte; /* current byte number in the
instructions word */
};
@@ -235,6 +236,86 @@ 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 (*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;
+
+ pr_debug(%s: fp = %08lx sp = %08lx lr = %08lx pc = %08lx\n, __func__,
+ ctrl-vrs[FP], ctrl-vrs[SP], ctrl-vrs[LR], ctrl-vrs[PC]);
+
+ 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;
+
+ pr_debug(%s: fp = %08lx sp = %08lx lr = %08lx pc = %08lx\n, __func__,
+ ctrl-vrs[FP], ctrl-vrs[SP], ctrl-vrs[LR], ctrl-vrs[PC]);
+
+ 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;
+
+ pr_debug(%s: fp = %08lx sp = %08lx lr = %08lx pc = %08lx\n, __func__,
+ ctrl-vrs[FP], ctrl-vrs[SP], ctrl-vrs[LR], ctrl-vrs[PC]);
+
+ return URC_OK;
+}
+
/*
* Execute the current unwind instruction.
*/
@@ -250,8 +331,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,38 +340,19 @@ 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] = *vsp++;
- mask = 1;
- reg++;
- }
- if (!load_sp)
- ctrl-vrs[SP] = (unsigned
Re: [PATCH V4] ARM : unwinder : Prevent data abort due to stack overflow
if (unlikely(check)
*vsp = (unsigned long *)ctrl-sp_high))
Good idea, It can help in optimizing the code and will leave code more readable
and it will be easy to maintain also.
Does the code check anywhere that SP is word-aligned?
That should probably be checked if it's not done already.
I think this should be handled in separate patch
I would also like to hear more your ideas for the file
Regards
Anurag
On Thu, Dec 5, 2013 at 7:29 PM, Dave Martin dave.mar...@arm.com wrote:
On Thu, Dec 05, 2013 at 11:26:25AM +, 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 | 127
-
1 files changed, 90 insertions(+), 37 deletions(-)
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 00df012..94f6ef4 100644
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -68,6 +68,7 @@ EXPORT_SYMBOL(__aeabi_unwind_cpp_pr2);
struct unwind_ctrl_block {
unsigned long vrs[16]; /* 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 byte; /* current byte number in the
instructions word */
};
@@ -235,6 +236,86 @@ 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 (*vsp = (unsigned long *)ctrl-sp_high)
+ return -URC_FAILURE;
+
+ ctrl-vrs[reg] = *(*vsp)++;
+ return URC_OK;
It occurred to me that your optimisation can still be implemented on
top on this.
If you add an extra flag parameter to unwind_pop_register telling it
whether to do checking or not, I think that the compiler and/or
CPU branch predictor should be able to do a pretty good job of
optimising the common case. Until we get near sp_high, if(check) will
branch the same way every time.
if (unlikely(check)
*vsp = (unsigned long *)ctrl-sp_high))
would make sense, in fact.
I think this brings most of the benefit, without needing to code the
insn exec rules twice.
I'm still not sure the optimisation benefits us much, but I think
that would be a tidier way of doing it if you want to try.
+}
+
+/* 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;
+
+ pr_debug(%s: fp = %08lx sp = %08lx lr = %08lx pc = %08lx\n, __func__,
+ ctrl-vrs[FP], ctrl-vrs[SP], ctrl-vrs[LR], ctrl-vrs[PC]);
Minor-ish nit: you now duplicate this same pr_debug() in many places.
Apologies, I didn't spot that in the previous review.
What about something like this:
static int unwind_exec_insn(...)
{
int ret = URC_OK;
} else if (...)
ret = unwind_exec_pop_subset_r4_to_r13(...);
if (ret)
goto error;
else ...
pr_debug(...);
error:
return ret;
}
Then everything returns through the same pr_debug() after the insn has
been executed.
[...]
@@ -329,13 +382,13 @@ static int unwind_exec_insn(struct unwind_ctrl_block
*ctrl)
*/
int unwind_frame(struct stackframe *frame)
{
- unsigned long high, low;
+ unsigned long low;
const struct unwind_idx *idx;
struct unwind_ctrl_block ctrl;
- /* only go to a higher address on the stack */
+ /* store the highest address on the stack to avoid crossing it*/
low = frame-sp;
- high = ALIGN(low, THREAD_SIZE);
+ ctrl.sp_high = ALIGN(low, THREAD_SIZE);
Does the code check anywhere that SP is word-aligned?
That should probably be checked if it's not done already.
I have some unrelated changes I want to make in this file (which
[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)
- ctrl
[PATCH V3] ARM : unwinder : Prevent data abort due to stack overflow
Signed-off-by: Anurag Aggarwal
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 data from the stack.
---
arch/arm/kernel/unwind.c | 207 ++
1 files changed, 153 insertions(+), 54 deletions(-)
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 00df012..a140725 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,8 +68,9 @@ 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 byte; /* current byte number in the
instructions word */
};
@@ -235,6 +238,148 @@ static unsigned long unwind_get_byte(struct
unwind_ctrl_block *ctrl)
return ret;
}
+static int unwind_exec_pop_subset_r4_to_r13(struct unwind_ctrl_block *ctrl,
+ unsigned long insn)
+{
+ int available_stack;
+ unsigned long mask;
+ unsigned long *vsp = (unsigned long *)ctrl->vrs[SP];
+ int load_sp, reg = 4;
+
+ /* caculate the space available on stack */
+ available_stack = ctrl->sp_high - ctrl->vrs[SP];
+
+ 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;
+ }
+
+ /*
+* Check whether there is enough space
+* on stack to execute the instruction
+* if not then return failure
+*/
+ if (available_stack < TOTAL_REGISTERS) {
+ unsigned long mask_copy = mask;
+ int required_stack = 0;
+
+ while (mask_copy) {
+ if (mask_copy & 1)
+ required_stack++;
+ mask_copy >>= 1;
+ }
+
+ if (available_stack < required_stack)
+ return -URC_FAILURE;
+ }
+
+ load_sp = mask & (1 << (13 - 4));
+ while (mask) {
+ if (mask & 1)
+ ctrl->vrs[reg] = *vsp++;
+ mask >>= 1;
+ reg++;
+ }
+ if (!load_sp)
+ ctrl->vrs[SP] = (unsigned long)vsp;
+
+ pr_debug("%s: fp = %08lx sp = %08lx lr = %08lx pc = %08lx\n", __func__,
+ ctrl->vrs[FP], ctrl->vrs[SP], ctrl->vrs[LR], ctrl->vrs[PC]);
+
+ return URC_OK;
+}
+
+static int unwind_exec_pop_r4_to_rN(struct unwind_ctrl_block *ctrl,
+ unsigned long insn)
+{
+ int available_stack;
+ unsigned long *vsp = (unsigned long *)ctrl->vrs[SP];
+ int reg;
+
+ /* caculate the space available on stack */
+ available_stack = ctrl->sp_high - ctrl->vrs[SP];
+
+ /*
+* Check whether there is enough space
+* on stack to execute the instruction
+* if not then return failure
+*/
+ if (available_stack < TOTAL_REGISTERS) {
+ int required_stack;
+
+ required_stack = insn & 7;
+ required_stack += (insn & 0x80) ? 1 : 0;
+
+ if (available_stack < required_stack)
+ return -URC_FAILURE;
+ }
+
+ /* pop R4-R[4+bbb] */
+ for (reg = 4; reg <= 4 + (insn & 7); reg++)
+ ctrl->vrs[reg] = *vsp++;
+ if (insn & 0x80)
+ ctrl->vrs[14] = *vsp++;
+ ctrl->vrs[SP] = (unsigned long)vsp;
+
+ pr_debug("%s: fp = %08lx sp = %08lx lr = %08lx pc = %08lx\n", __func__,
+ ctrl->vrs[FP], ctrl->vrs[SP], ctrl->vrs[LR], ctrl->vrs[PC]);
+
+ return URC_OK;
+}
+
+static int unwind_exec_pop_subset_r0_to_r3(struct unwind_ctrl_block *ctrl,
+ unsigned long insn)
+{
+ int available_stack;
+ unsigned long mask = unwind_get_byte(ctrl);
+ unsigned long *vsp = (unsigned long *)ctrl->vrs[SP
[PATCH V3] ARM : unwinder : Prevent data abort due to stack overflow
Signed-off-by: Anurag Aggarwal a.anu...@samsung.com
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 data from the stack.
---
arch/arm/kernel/unwind.c | 207 ++
1 files changed, 153 insertions(+), 54 deletions(-)
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 00df012..a140725 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,8 +68,9 @@ 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 byte; /* current byte number in the
instructions word */
};
@@ -235,6 +238,148 @@ static unsigned long unwind_get_byte(struct
unwind_ctrl_block *ctrl)
return ret;
}
+static int unwind_exec_pop_subset_r4_to_r13(struct unwind_ctrl_block *ctrl,
+ unsigned long insn)
+{
+ int available_stack;
+ unsigned long mask;
+ unsigned long *vsp = (unsigned long *)ctrl-vrs[SP];
+ int load_sp, reg = 4;
+
+ /* caculate the space available on stack */
+ available_stack = ctrl-sp_high - ctrl-vrs[SP];
+
+ 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;
+ }
+
+ /*
+* Check whether there is enough space
+* on stack to execute the instruction
+* if not then return failure
+*/
+ if (available_stack TOTAL_REGISTERS) {
+ unsigned long mask_copy = mask;
+ int required_stack = 0;
+
+ while (mask_copy) {
+ if (mask_copy 1)
+ required_stack++;
+ mask_copy = 1;
+ }
+
+ if (available_stack required_stack)
+ return -URC_FAILURE;
+ }
+
+ load_sp = mask (1 (13 - 4));
+ while (mask) {
+ if (mask 1)
+ ctrl-vrs[reg] = *vsp++;
+ mask = 1;
+ reg++;
+ }
+ if (!load_sp)
+ ctrl-vrs[SP] = (unsigned long)vsp;
+
+ pr_debug(%s: fp = %08lx sp = %08lx lr = %08lx pc = %08lx\n, __func__,
+ ctrl-vrs[FP], ctrl-vrs[SP], ctrl-vrs[LR], ctrl-vrs[PC]);
+
+ return URC_OK;
+}
+
+static int unwind_exec_pop_r4_to_rN(struct unwind_ctrl_block *ctrl,
+ unsigned long insn)
+{
+ int available_stack;
+ unsigned long *vsp = (unsigned long *)ctrl-vrs[SP];
+ int reg;
+
+ /* caculate the space available on stack */
+ available_stack = ctrl-sp_high - ctrl-vrs[SP];
+
+ /*
+* Check whether there is enough space
+* on stack to execute the instruction
+* if not then return failure
+*/
+ if (available_stack TOTAL_REGISTERS) {
+ int required_stack;
+
+ required_stack = insn 7;
+ required_stack += (insn 0x80) ? 1 : 0;
+
+ if (available_stack required_stack)
+ return -URC_FAILURE;
+ }
+
+ /* pop R4-R[4+bbb] */
+ for (reg = 4; reg = 4 + (insn 7); reg++)
+ ctrl-vrs[reg] = *vsp++;
+ if (insn 0x80)
+ ctrl-vrs[14] = *vsp++;
+ ctrl-vrs[SP] = (unsigned long)vsp;
+
+ pr_debug(%s: fp = %08lx sp = %08lx lr = %08lx pc = %08lx\n, __func__,
+ ctrl-vrs[FP], ctrl-vrs[SP], ctrl-vrs[LR], ctrl-vrs[PC]);
+
+ return URC_OK;
+}
+
+static int unwind_exec_pop_subset_r0_to_r3(struct unwind_ctrl_block *ctrl,
+ unsigned long insn)
+{
+ int available_stack;
+ unsigned long mask = unwind_get_byte(ctrl);
+ unsigned long *vsp = (unsigned long *)ctrl-vrs[SP];
+ int reg = 0;
+
+ if (mask == 0 || mask 0xf0) {
+ pr_warning(unwind: Spare encoding %04lx\n,
+ (insn 8) | mask
Re: [PATCH V2] ARM : unwinder : Prevent data abort due to stack overflow while executing unwind instructions Signed-off-by: Anurag Aggarwal
>> Looks like you still need to move your S-o-B line. It needs to be at
>> the end of the commit message.
>>
>> On Fri, Nov 29, 2013 at 09:34:31AM +, 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 data
>>> abort in there seperate functions instead of unwind_exec_insn, where a
>>> check for there
>>> feasibility is made first.
>>
>> Minor nit, but please wrap the commit message lines to 72 chars or less.
>> This helps the patch message to be listed nicely in git log.
>>
>>
>> If you agree with the changes I suggest below, the second paragraph
>> could be reworded something like:
>>
>> --snip--
>>
>> 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.
>>
>> --snip--
>>
>>
>>> ---
>>> arch/arm/kernel/unwind.c | 197
>>> ++
>>> 1 files changed, 148 insertions(+), 49 deletions(-)
>>>
>>> diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
>>> index 00df012..150e0fc 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,148 @@ static unsigned long unwind_get_byte(struct
>>> unwind_ctrl_block *ctrl)
>>> return ret;
>>> }
>>>
>>> +static int unwind_exec_insn_0x80(struct unwind_ctrl_block *ctrl,
>>> + unsigned long insn)
>>
>> Since these are now split out as named functions, it's useful to have
>> human readable names.
>>
>> Maybe something like:
>>
>> unwind_exec_pop_subset_r4_to_r13
>>
>> What do you think?
>>
>>> +{
>>> + unsigned long high, low;
>>> + unsigned long mask;
>>> + unsigned long *vsp = (unsigned long *)ctrl->vrs[SP];
>>> + int load_sp, reg = 4;
>>> +
>>> + low = ctrl->vrs[SP];
>>> + high = ALIGN(low, THREAD_SIZE);
>>
>> You can calculate low, high and high - low once and store them in
>> unwind_ctrl_block. No need to recalculate them every time.
>
> I don't think it is feasible to store high and low in unwind_ctrl_block,
> we will have to recalculate them every time in this case also as the
> value of sp is which change every time and depending on the value
> of sp the value of high and low will also change.
>
>
>>
>> Field names like "low" may be confusing though. "sp_low" etc. may be
>> clearer.
>>
>>> +
>>> + 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;
>>> + }
>>> +
>>> + /*
>>> + * Check whether there is enough space
>>> + * on stack to execute the instruction
>>> + * if not then return failure
>>> + */
>>> + if ((high - low) < TOTAL_REGISTERS) {
>>
>> I'm still not sure we are optimising something valuable by factoring out
>> this check.
>
> Even I am confused as to you why you ar
Re: [PATCH V2] ARM : unwinder : Prevent data abort due to stack overflow while executing unwind instructions Signed-off-by: Anurag Aggarwal
> Looks like you still need to move your S-o-B line. It needs to be at
> the end of the commit message.
>
> On Fri, Nov 29, 2013 at 09:34:31AM +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 data
>> abort in there seperate functions instead of unwind_exec_insn, where a check
>> for there
>> feasibility is made first.
>
> Minor nit, but please wrap the commit message lines to 72 chars or less.
> This helps the patch message to be listed nicely in git log.
>
>
> If you agree with the changes I suggest below, the second paragraph
> could be reworded something like:
>
> --snip--
>
> 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.
>
> --snip--
>
>
>> ---
>> arch/arm/kernel/unwind.c | 197
>> ++
>> 1 files changed, 148 insertions(+), 49 deletions(-)
>>
>> diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
>> index 00df012..150e0fc 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,148 @@ static unsigned long unwind_get_byte(struct
>> unwind_ctrl_block *ctrl)
>> return ret;
>> }
>>
>> +static int unwind_exec_insn_0x80(struct unwind_ctrl_block *ctrl,
>> + unsigned long insn)
>
> Since these are now split out as named functions, it's useful to have
> human readable names.
>
> Maybe something like:
>
> unwind_exec_pop_subset_r4_to_r13
>
> What do you think?
>
>> +{
>> + unsigned long high, low;
>> + unsigned long mask;
>> + unsigned long *vsp = (unsigned long *)ctrl->vrs[SP];
>> + int load_sp, reg = 4;
>> +
>> + low = ctrl->vrs[SP];
>> + high = ALIGN(low, THREAD_SIZE);
>
> You can calculate low, high and high - low once and store them in
> unwind_ctrl_block. No need to recalculate them every time.
I don't think it is feasible to store high and low in unwind_ctrl_block,
we will have to recalculate them every time in this case also as the
value of sp is which change every time and depending on the value
of sp the value of high and low will also change.
>
> Field names like "low" may be confusing though. "sp_low" etc. may be
> clearer.
>
>> +
>> + 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;
>> + }
>> +
>> + /*
>> + * Check whether there is enough space
>> + * on stack to execute the instruction
>> + * if not then return failure
>> + */
>> + if ((high - low) < TOTAL_REGISTERS) {
>
> I'm still not sure we are optimising something valuable by factoring out
> this check.
Even I am confused as to you why you are not sure. From the documentation
and the original source code, it seems clear that only the last set of registers
can create a stack overflow for these three instructions, so why waste cpu
cycles in calculations that are unnecessary
>
> It's also unfortunate that the code describes how many registers to pop
> twice -- once in the check, and once in the while loop that does the
> popping. That kind of duplication brings risks of accidentally breaking
Re: [PATCH V2] ARM : unwinder : Prevent data abort due to stack overflow while executing unwind instructions Signed-off-by: Anurag Aggarwal a.anu...@samsung.com
Looks like you still need to move your S-o-B line. It needs to be at
the end of the commit message.
On Fri, Nov 29, 2013 at 09:34:31AM +, 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 data
abort in there seperate functions instead of unwind_exec_insn, where a check
for there
feasibility is made first.
Minor nit, but please wrap the commit message lines to 72 chars or less.
This helps the patch message to be listed nicely in git log.
If you agree with the changes I suggest below, the second paragraph
could be reworded something like:
--snip--
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.
--snip--
---
arch/arm/kernel/unwind.c | 197
++
1 files changed, 148 insertions(+), 49 deletions(-)
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 00df012..150e0fc 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,148 @@ static unsigned long unwind_get_byte(struct
unwind_ctrl_block *ctrl)
return ret;
}
+static int unwind_exec_insn_0x80(struct unwind_ctrl_block *ctrl,
+ unsigned long insn)
Since these are now split out as named functions, it's useful to have
human readable names.
Maybe something like:
unwind_exec_pop_subset_r4_to_r13
What do you think?
+{
+ unsigned long high, low;
+ unsigned long mask;
+ unsigned long *vsp = (unsigned long *)ctrl-vrs[SP];
+ int load_sp, reg = 4;
+
+ low = ctrl-vrs[SP];
+ high = ALIGN(low, THREAD_SIZE);
You can calculate low, high and high - low once and store them in
unwind_ctrl_block. No need to recalculate them every time.
I don't think it is feasible to store high and low in unwind_ctrl_block,
we will have to recalculate them every time in this case also as the
value of sp is which change every time and depending on the value
of sp the value of high and low will also change.
Field names like low may be confusing though. sp_low etc. may be
clearer.
+
+ 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;
+ }
+
+ /*
+ * Check whether there is enough space
+ * on stack to execute the instruction
+ * if not then return failure
+ */
+ if ((high - low) TOTAL_REGISTERS) {
I'm still not sure we are optimising something valuable by factoring out
this check.
Even I am confused as to you why you are not sure. From the documentation
and the original source code, it seems clear that only the last set of registers
can create a stack overflow for these three instructions, so why waste cpu
cycles in calculations that are unnecessary
It's also unfortunate that the code describes how many registers to pop
twice -- once in the check, and once in the while loop that does the
popping. That kind of duplication brings risks of accidentally breaking
the code during future maintenance. Someone might not modify the two
pieces of code in a consistent way. Because stack overflow should never
normally occur anyway, a mistake like that could easily get missed
during testing.
Removing this whole if block and just doing a simple check each time a
register is loaded...
+ unsigned long mask_copy = mask;
+ int required_stack = 0;
+
+ while (mask_copy) {
+ if (mask_copy 1)
+ required_stack++;
+ mask_copy = 1;
+ }
+
+ if ((high - low) required_stack)
+ return -URC_FAILURE;
+ }
+
+ load_sp = mask (1 (13 - 4));
+ while (mask) {
+ if (mask 1
Re: [PATCH V2] ARM : unwinder : Prevent data abort due to stack overflow while executing unwind instructions Signed-off-by: Anurag Aggarwal a.anu...@samsung.com
Looks like you still need to move your S-o-B line. It needs to be at
the end of the commit message.
On Fri, Nov 29, 2013 at 09:34:31AM +, 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 data
abort in there seperate functions instead of unwind_exec_insn, where a
check for there
feasibility is made first.
Minor nit, but please wrap the commit message lines to 72 chars or less.
This helps the patch message to be listed nicely in git log.
If you agree with the changes I suggest below, the second paragraph
could be reworded something like:
--snip--
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.
--snip--
---
arch/arm/kernel/unwind.c | 197
++
1 files changed, 148 insertions(+), 49 deletions(-)
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 00df012..150e0fc 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,148 @@ static unsigned long unwind_get_byte(struct
unwind_ctrl_block *ctrl)
return ret;
}
+static int unwind_exec_insn_0x80(struct unwind_ctrl_block *ctrl,
+ unsigned long insn)
Since these are now split out as named functions, it's useful to have
human readable names.
Maybe something like:
unwind_exec_pop_subset_r4_to_r13
What do you think?
+{
+ unsigned long high, low;
+ unsigned long mask;
+ unsigned long *vsp = (unsigned long *)ctrl-vrs[SP];
+ int load_sp, reg = 4;
+
+ low = ctrl-vrs[SP];
+ high = ALIGN(low, THREAD_SIZE);
You can calculate low, high and high - low once and store them in
unwind_ctrl_block. No need to recalculate them every time.
I don't think it is feasible to store high and low in unwind_ctrl_block,
we will have to recalculate them every time in this case also as the
value of sp is which change every time and depending on the value
of sp the value of high and low will also change.
Field names like low may be confusing though. sp_low etc. may be
clearer.
+
+ 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;
+ }
+
+ /*
+ * Check whether there is enough space
+ * on stack to execute the instruction
+ * if not then return failure
+ */
+ if ((high - low) TOTAL_REGISTERS) {
I'm still not sure we are optimising something valuable by factoring out
this check.
Even I am confused as to you why you are not sure. From the documentation
and the original source code, it seems clear that only the last set of
registers
can create a stack overflow for these three instructions, so why waste cpu
cycles in calculations that are unnecessary
It's also unfortunate that the code describes how many registers to pop
twice -- once in the check, and once in the while loop that does the
popping. That kind of duplication brings risks of accidentally breaking
the code during future maintenance. Someone might not modify the two
pieces of code in a consistent way. Because stack overflow should never
normally occur anyway, a mistake like that could easily get missed
during testing.
Removing this whole if block and just doing a simple check each time a
register is loaded...
+ unsigned long mask_copy = mask;
+ int required_stack = 0;
+
+ while (mask_copy) {
+ if (mask_copy 1)
+ required_stack++;
+ mask_copy = 1;
+ }
+
+ if ((high - low) required_stack)
+ return -URC_FAILURE;
+ }
+
+ load_sp = mask (1 (13 - 4));
+ while (mask) {
+ if (mask 1
[PATCH V2] ARM : unwinder : Prevent data abort due to stack overflow while executing unwind instructions Signed-off-by: Anurag Aggarwal
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
data
abort in there seperate functions instead of unwind_exec_insn, where a check
for there
feasibility is made first.
---
arch/arm/kernel/unwind.c | 197 ++
1 files changed, 148 insertions(+), 49 deletions(-)
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 00df012..150e0fc 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,148 @@ static unsigned long unwind_get_byte(struct
unwind_ctrl_block *ctrl)
return ret;
}
+static int unwind_exec_insn_0x80(struct unwind_ctrl_block *ctrl,
+ unsigned long insn)
+{
+ unsigned long high, low;
+ unsigned long mask;
+ unsigned long *vsp = (unsigned long *)ctrl->vrs[SP];
+ int load_sp, reg = 4;
+
+ low = ctrl->vrs[SP];
+ high = ALIGN(low, THREAD_SIZE);
+
+ 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;
+ }
+
+ /*
+* Check whether there is enough space
+* on stack to execute the instruction
+* if not then return failure
+*/
+ if ((high - low) < TOTAL_REGISTERS) {
+ unsigned long mask_copy = mask;
+ int required_stack = 0;
+
+ while (mask_copy) {
+ if (mask_copy & 1)
+ required_stack++;
+ mask_copy >>= 1;
+ }
+
+ if ((high - low) < required_stack)
+ return -URC_FAILURE;
+ }
+
+ load_sp = mask & (1 << (13 - 4));
+ while (mask) {
+ if (mask & 1)
+ ctrl->vrs[reg] = *vsp++;
+ mask >>= 1;
+ reg++;
+ }
+ if (!load_sp)
+ ctrl->vrs[SP] = (unsigned long)vsp;
+
+ pr_debug("%s: fp = %08lx sp = %08lx lr = %08lx pc = %08lx\n", __func__,
+ ctrl->vrs[FP], ctrl->vrs[SP], ctrl->vrs[LR], ctrl->vrs[PC]);
+
+ return URC_OK;
+}
+
+static int unwind_exec_insn_0xa0(struct unwind_ctrl_block *ctrl,
+ unsigned long insn)
+{
+ unsigned long high, low;
+ unsigned long *vsp = (unsigned long *)ctrl->vrs[SP];
+ int reg;
+
+ low = ctrl->vrs[SP];
+ high = ALIGN(low, THREAD_SIZE);
+
+ /*
+* Check whether there is enough space
+* on stack to execute the instruction
+* if not then return failure
+*/
+ if ((high-low) < TOTAL_REGISTERS) {
+ int required_stack;
+
+ required_stack = insn & 7;
+ required_stack += (insn & 0x80) ? 1 : 0;
+
+ if ((high-low) < required_stack)
+ return -URC_FAILURE;
+ }
+
+ /* pop R4-R[4+bbb] */
+ for (reg = 4; reg <= 4 + (insn & 7); reg++)
+ ctrl->vrs[reg] = *vsp++;
+ if (insn & 0x80)
+ ctrl->vrs[14] = *vsp++;
+ ctrl->vrs[SP] = (unsigned long)vsp;
+
+ pr_debug("%s: fp = %08lx sp = %08lx lr = %08lx pc = %08lx\n", __func__,
+ ctrl->vrs[FP], ctrl->vrs[SP], ctrl->vrs[LR], ctrl->vrs[PC]);
+
+ return URC_OK;
+}
+
+static int unwind_exec_insn_0xb1(struct unwind_ctrl_block *ctrl,
+ unsigned long insn)
+{
+ unsigned long high, low;
+ unsigned long mask = unwind_get_byte(ctrl);
+ unsigned long *vsp = (unsigned long *)ctrl->vrs[SP];
+ int reg = 0;
+
+ if (mask == 0 || mask & 0xf0) {
+ pr_warning("unwind: Spare encoding %04lx\n",
+ (insn << 8) | mask);
+ return -URC_FAILURE;
+ }
+
+ low = ctrl->vrs[SP];
+ high = ALIGN(low, THREAD_SIZE);
+
+ /*
+* Check whether there is enough space
+* on stack to execute the instruction
+*
[PATCH V2] ARM : unwinder : Prevent data abort due to stack overflow while executing unwind instructions Signed-off-by: Anurag Aggarwal a.anu...@samsung.com
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
data
abort in there seperate functions instead of unwind_exec_insn, where a check
for there
feasibility is made first.
---
arch/arm/kernel/unwind.c | 197 ++
1 files changed, 148 insertions(+), 49 deletions(-)
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 00df012..150e0fc 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,148 @@ static unsigned long unwind_get_byte(struct
unwind_ctrl_block *ctrl)
return ret;
}
+static int unwind_exec_insn_0x80(struct unwind_ctrl_block *ctrl,
+ unsigned long insn)
+{
+ unsigned long high, low;
+ unsigned long mask;
+ unsigned long *vsp = (unsigned long *)ctrl-vrs[SP];
+ int load_sp, reg = 4;
+
+ low = ctrl-vrs[SP];
+ high = ALIGN(low, THREAD_SIZE);
+
+ 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;
+ }
+
+ /*
+* Check whether there is enough space
+* on stack to execute the instruction
+* if not then return failure
+*/
+ if ((high - low) TOTAL_REGISTERS) {
+ unsigned long mask_copy = mask;
+ int required_stack = 0;
+
+ while (mask_copy) {
+ if (mask_copy 1)
+ required_stack++;
+ mask_copy = 1;
+ }
+
+ if ((high - low) required_stack)
+ return -URC_FAILURE;
+ }
+
+ load_sp = mask (1 (13 - 4));
+ while (mask) {
+ if (mask 1)
+ ctrl-vrs[reg] = *vsp++;
+ mask = 1;
+ reg++;
+ }
+ if (!load_sp)
+ ctrl-vrs[SP] = (unsigned long)vsp;
+
+ pr_debug(%s: fp = %08lx sp = %08lx lr = %08lx pc = %08lx\n, __func__,
+ ctrl-vrs[FP], ctrl-vrs[SP], ctrl-vrs[LR], ctrl-vrs[PC]);
+
+ return URC_OK;
+}
+
+static int unwind_exec_insn_0xa0(struct unwind_ctrl_block *ctrl,
+ unsigned long insn)
+{
+ unsigned long high, low;
+ unsigned long *vsp = (unsigned long *)ctrl-vrs[SP];
+ int reg;
+
+ low = ctrl-vrs[SP];
+ high = ALIGN(low, THREAD_SIZE);
+
+ /*
+* Check whether there is enough space
+* on stack to execute the instruction
+* if not then return failure
+*/
+ if ((high-low) TOTAL_REGISTERS) {
+ int required_stack;
+
+ required_stack = insn 7;
+ required_stack += (insn 0x80) ? 1 : 0;
+
+ if ((high-low) required_stack)
+ return -URC_FAILURE;
+ }
+
+ /* pop R4-R[4+bbb] */
+ for (reg = 4; reg = 4 + (insn 7); reg++)
+ ctrl-vrs[reg] = *vsp++;
+ if (insn 0x80)
+ ctrl-vrs[14] = *vsp++;
+ ctrl-vrs[SP] = (unsigned long)vsp;
+
+ pr_debug(%s: fp = %08lx sp = %08lx lr = %08lx pc = %08lx\n, __func__,
+ ctrl-vrs[FP], ctrl-vrs[SP], ctrl-vrs[LR], ctrl-vrs[PC]);
+
+ return URC_OK;
+}
+
+static int unwind_exec_insn_0xb1(struct unwind_ctrl_block *ctrl,
+ unsigned long insn)
+{
+ unsigned long high, low;
+ unsigned long mask = unwind_get_byte(ctrl);
+ unsigned long *vsp = (unsigned long *)ctrl-vrs[SP];
+ int reg = 0;
+
+ if (mask == 0 || mask 0xf0) {
+ pr_warning(unwind: Spare encoding %04lx\n,
+ (insn 8) | mask);
+ return -URC_FAILURE;
+ }
+
+ low = ctrl-vrs[SP];
+ high = ALIGN(low, THREAD_SIZE);
+
+ /*
+* Check whether there is enough space
+* on stack to execute the instruction
+* if not then return failure
+
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;
> + }
> + }
>
[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
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
Re: Re: [PATCH] ARM: unwinder: Handle Stackoverflow in unwind_exec_insn
Hi Dave,
I think that we don't need to avoid stack overflow completely but we need to
avoid data being derefrenced that is not part of stack.
I agree with you that there is no rule in ABI to guarantee that stack overflow
will only occur when backtracking last set of register.
From my understanding of code the only chances of getting a data abort is while
executing these four instructions:
1) 1000 : Pop up to 12 integer registers under masks {r15-r12},
{r11-r4}
2) 10100nnn : Pop r4-r[4+nnn]
3) 10101nnn : Pop r4-r[4+nnn], r14
4) 10110001 : Pop integer registers under mask {r3, r2, r1, r0}
I think it would be better to execute these instruction in their own seperate
functions, which would be called from unwind_exec_insn, and in those function
depending on the depth of stack remaining we can decide whether it is possible
to execute the instruction or not.
The above method will add some extra code but will avoid additional checks that
are not required every where.
and solve our purpose also as we need to avoid data abort due to stack overflow
not stack overflow completely.
Regards
Anurag Aggarwal
--- Original Message ---
Sender : Dave Martin
Date : Nov 23, 2013 01:07 (GMT+05:30)
Title : Re: [PATCH] ARM: unwinder: Handle Stackoverflow in unwind_exec_insn
On Sat, Nov 09, 2013 at 12:28:57PM +0530, Anurag Aggarwal wrote:
> Thanks for your input Dave,
>
> I think there is another way to avoid the stack overflow and reduce
> the number of checks also,
>
> Stack overflow will cause a problem only when we are backtracking the
> last set of registers.
> i.e when the difference between current SP and top of stack is less
> than or equal to number of registers
Apologies, it looks like I failed to respond to this earlier...
Although that will usually be correct, there is no rule in the ABI to
guarantee it.
> we can create two unwind_exec_insn, one without checks and one with checks.
>
> then we call the correct function from unwind_frame depending on the
> difference of SP and top of stack.
>
> This will reduce the amount of checks every time we read a set of
> registers from stack
That sounds like it might duplicate a lot of code, to optimise based on
assumptions that may not always be true, for what really should not be a
hot path in the kernel.
If you can find a tidy way of doing it, it would be certainly worth
reviewing, but I still think it would be simpler just to do a simple
bounds check for every word read from the stack -- it should be
impossible for that to go wrong, even if some of the bounds checks
are not stictly required.
Cheers
---DaveN‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±‘êçzX§¶›¡Ü¨}©ž²Æ
zÚ:+v‰¨¾«‘êçzZ+€Ê+zf£¢·hšˆ§~††Ûiÿûàz¹ ®w¥¢¸?™¨èÚ&¢)ߢf”ù^jÇ«y§m…á@A«a¶Úÿ
0¶ìh®å’i
Re: Re: [PATCH] ARM: unwinder: Handle Stackoverflow in unwind_exec_insn
Hi Dave,
I think that we don't need to avoid stack overflow completely but we need to
avoid data being derefrenced that is not part of stack.
I agree with you that there is no rule in ABI to guarantee that stack overflow
will only occur when backtracking last set of register.
From my understanding of code the only chances of getting a data abort is while
executing these four instructions:
1) 1000 : Pop up to 12 integer registers under masks {r15-r12},
{r11-r4}
2) 10100nnn : Pop r4-r[4+nnn]
3) 10101nnn : Pop r4-r[4+nnn], r14
4) 10110001 : Pop integer registers under mask {r3, r2, r1, r0}
I think it would be better to execute these instruction in their own seperate
functions, which would be called from unwind_exec_insn, and in those function
depending on the depth of stack remaining we can decide whether it is possible
to execute the instruction or not.
The above method will add some extra code but will avoid additional checks that
are not required every where.
and solve our purpose also as we need to avoid data abort due to stack overflow
not stack overflow completely.
Regards
Anurag Aggarwal
--- Original Message ---
Sender : Dave Martindave.mar...@arm.com
Date : Nov 23, 2013 01:07 (GMT+05:30)
Title : Re: [PATCH] ARM: unwinder: Handle Stackoverflow in unwind_exec_insn
On Sat, Nov 09, 2013 at 12:28:57PM +0530, Anurag Aggarwal wrote:
Thanks for your input Dave,
I think there is another way to avoid the stack overflow and reduce
the number of checks also,
Stack overflow will cause a problem only when we are backtracking the
last set of registers.
i.e when the difference between current SP and top of stack is less
than or equal to number of registers
Apologies, it looks like I failed to respond to this earlier...
Although that will usually be correct, there is no rule in the ABI to
guarantee it.
we can create two unwind_exec_insn, one without checks and one with checks.
then we call the correct function from unwind_frame depending on the
difference of SP and top of stack.
This will reduce the amount of checks every time we read a set of
registers from stack
That sounds like it might duplicate a lot of code, to optimise based on
assumptions that may not always be true, for what really should not be a
hot path in the kernel.
If you can find a tidy way of doing it, it would be certainly worth
reviewing, but I still think it would be simpler just to do a simple
bounds check for every word read from the stack -- it should be
impossible for that to go wrong, even if some of the bounds checks
are not stictly required.
Cheers
---DaveN‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±‘êçzX§¶›¡Ü¨}©ž²Æ
zÚj:+v‰¨¾«‘êçzZ+€Ê+zf£¢·hšˆ§~††Ûiÿûàz¹ ®w¥¢¸?™¨èÚ¢)ߢf”ù^jÇ«y§m…á@A«a¶Úÿ
0¶ìh®å’i
Re: [PATCH] ARM: unwinder: Handle Stackoverflow in unwind_exec_insn
Thanks for your input Dave,
I think there is another way to avoid the stack overflow and reduce
the number of checks also,
Stack overflow will cause a problem only when we are backtracking the
last set of registers.
i.e when the difference between current SP and top of stack is less
than or equal to number of registers
we can create two unwind_exec_insn, one without checks and one with checks.
then we call the correct function from unwind_frame depending on the
difference of SP and top of stack.
This will reduce the amount of checks every time we read a set of
registers from stack
Regards
Anurag
On Fri, Nov 8, 2013 at 6:51 PM, Dave Martin wrote:
> On Wed, Nov 06, 2013 at 03:20:48PM +0530, Anurag Aggarwal wrote:
>> Altough stack overflow is expected in unwind_exec_insn, but in cases when
>> area beyond stack is not mapped to physical memory this can cause data abort.
>>
>> To avoid above condition handle stack overflow in unwind_exec_insn by
>> checking vsp pointer from top of stack
>
> This looks worthwhile, but this patch duplicates the same check in
> many places, making the code harder to read and maintain than
> necessary. It would be a good opportunity for a bit of refactoring,
> since we've already tried to fix these backtrace overrun issues
> at least once in the past (not 100% successfully ...)
>
>
> Really, there is a single common check needed: every time we want to
> read a word from the stack, we need to check that word is within
> the proper stack bounds before doing the read.
>
>
> At the start of the backtrace, we should determine the thread stack
> bounds for the thread. Those bounds should be fixed for the whole
> backtrace; it makes sense to store them in the unwind_ctrl_block
> structure, so that called functions can see them.
>
> Then a helper can be written that does the correct bounds check and
> reads a word from the stack, using the current frame's base virtual
> SP and the thread stack bounds.
>
> Then we just need to use that helper whenever we want to read a
> word from the stack.
>
> Cheers
> ---Dave
>
>
>> Signed-off-by: Anurag Aggarwal
>> ---
>> arch/arm/kernel/unwind.c | 23 +++
>> 1 files changed, 15 insertions(+), 8 deletions(-)
>> diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
>> index 00df012..d8b8721 100644
>> --- a/arch/arm/kernel/unwind.c
>> +++ b/arch/arm/kernel/unwind.c
>> @@ -241,6 +241,10 @@ static unsigned long unwind_get_byte(struct
>> unwind_ctrl_block *ctrl)
>> static int unwind_exec_insn(struct unwind_ctrl_block *ctrl)
>> {
>> unsigned long insn = unwind_get_byte(ctrl);
>> + unsigned long high, low;
>> + unsigned long *vsp = (unsigned long *)ctrl->vrs[SP];
>> + low = ctrl->vrs[SP];
>> + high = ALIGN(low, THREAD_SIZE);
>>
>> pr_debug("%s: insn = %08lx\n", __func__, insn);
>>
>> @@ -263,27 +267,27 @@ static int unwind_exec_insn(struct unwind_ctrl_block
>> *ctrl)
>>
>> /* pop R4-R15 according to mask */
>> load_sp = mask & (1 << (13 - 4));
>> - while (mask) {
>> + while (mask && vsp < high) {
>> if (mask & 1)
>> ctrl->vrs[reg] = *vsp++;
>> mask >>= 1;
>> reg++;
>> }
>> - if (!load_sp)
>> + if (!load_sp && vsp < high)
>> ctrl->vrs[SP] = (unsigned long)vsp;
>> } else if ((insn & 0xf0) == 0x90 &&
>> (insn & 0x0d) != 0x0d)
>> ctrl->vrs[SP] = ctrl->vrs[insn & 0x0f];
>> else if ((insn & 0xf0) == 0xa0) {
>> - unsigned long *vsp = (unsigned long *)ctrl->vrs[SP];
>> int reg;
>>
>> /* pop R4-R[4+bbb] */
>> - for (reg = 4; reg <= 4 + (insn & 7); reg++)
>> + for (reg = 4; (reg <= 4 + (insn & 7)) && (vsp < high; reg++)
>> ctrl->vrs[reg] = *vsp++;
>> - if (insn & 0x80)
>> + if (insn & 0x80 && vsp < high)
>> ctrl->vrs[14] = *vsp++;
>> - ctrl->vrs[SP] = (unsigned long)vsp;
>> + if (vsp < high)
>> + ctrl->vrs[SP] = (unsigned long)vsp;
>> } else if (insn == 0xb0) {
>> if (ctrl->vrs[PC] == 0)
>> ctrl->vrs[PC] = ctrl-&g
Re: [PATCH] ARM: unwinder: Handle Stackoverflow in unwind_exec_insn
Thanks for your input Dave,
I think there is another way to avoid the stack overflow and reduce
the number of checks also,
Stack overflow will cause a problem only when we are backtracking the
last set of registers.
i.e when the difference between current SP and top of stack is less
than or equal to number of registers
we can create two unwind_exec_insn, one without checks and one with checks.
then we call the correct function from unwind_frame depending on the
difference of SP and top of stack.
This will reduce the amount of checks every time we read a set of
registers from stack
Regards
Anurag
On Fri, Nov 8, 2013 at 6:51 PM, Dave Martin dave.mar...@arm.com wrote:
On Wed, Nov 06, 2013 at 03:20:48PM +0530, Anurag Aggarwal wrote:
Altough stack overflow is expected in unwind_exec_insn, but in cases when
area beyond stack is not mapped to physical memory this can cause data abort.
To avoid above condition handle stack overflow in unwind_exec_insn by
checking vsp pointer from top of stack
This looks worthwhile, but this patch duplicates the same check in
many places, making the code harder to read and maintain than
necessary. It would be a good opportunity for a bit of refactoring,
since we've already tried to fix these backtrace overrun issues
at least once in the past (not 100% successfully ...)
Really, there is a single common check needed: every time we want to
read a word from the stack, we need to check that word is within
the proper stack bounds before doing the read.
At the start of the backtrace, we should determine the thread stack
bounds for the thread. Those bounds should be fixed for the whole
backtrace; it makes sense to store them in the unwind_ctrl_block
structure, so that called functions can see them.
Then a helper can be written that does the correct bounds check and
reads a word from the stack, using the current frame's base virtual
SP and the thread stack bounds.
Then we just need to use that helper whenever we want to read a
word from the stack.
Cheers
---Dave
Signed-off-by: Anurag Aggarwal a.anu...@samsung.com
---
arch/arm/kernel/unwind.c | 23 +++
1 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 00df012..d8b8721 100644
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -241,6 +241,10 @@ static unsigned long unwind_get_byte(struct
unwind_ctrl_block *ctrl)
static int unwind_exec_insn(struct unwind_ctrl_block *ctrl)
{
unsigned long insn = unwind_get_byte(ctrl);
+ unsigned long high, low;
+ unsigned long *vsp = (unsigned long *)ctrl-vrs[SP];
+ low = ctrl-vrs[SP];
+ high = ALIGN(low, THREAD_SIZE);
pr_debug(%s: insn = %08lx\n, __func__, insn);
@@ -263,27 +267,27 @@ static int unwind_exec_insn(struct unwind_ctrl_block
*ctrl)
/* pop R4-R15 according to mask */
load_sp = mask (1 (13 - 4));
- while (mask) {
+ while (mask vsp high) {
if (mask 1)
ctrl-vrs[reg] = *vsp++;
mask = 1;
reg++;
}
- if (!load_sp)
+ if (!load_sp vsp high)
ctrl-vrs[SP] = (unsigned long)vsp;
} else if ((insn 0xf0) == 0x90
(insn 0x0d) != 0x0d)
ctrl-vrs[SP] = ctrl-vrs[insn 0x0f];
else if ((insn 0xf0) == 0xa0) {
- unsigned long *vsp = (unsigned long *)ctrl-vrs[SP];
int reg;
/* pop R4-R[4+bbb] */
- for (reg = 4; reg = 4 + (insn 7); reg++)
+ for (reg = 4; (reg = 4 + (insn 7)) (vsp high; reg++)
ctrl-vrs[reg] = *vsp++;
- if (insn 0x80)
+ if (insn 0x80 vsp high)
ctrl-vrs[14] = *vsp++;
- ctrl-vrs[SP] = (unsigned long)vsp;
+ if (vsp high)
+ ctrl-vrs[SP] = (unsigned long)vsp;
} else if (insn == 0xb0) {
if (ctrl-vrs[PC] == 0)
ctrl-vrs[PC] = ctrl-vrs[LR];
@@ -301,13 +305,14 @@ static int unwind_exec_insn(struct unwind_ctrl_block
*ctrl)
}
/* pop R0-R3 according to mask */
- while (mask) {
+ while (mask vsp high) {
if (mask 1)
ctrl-vrs[reg] = *vsp++;
mask = 1;
reg++;
}
- ctrl-vrs[SP] = (unsigned long)vsp;
+ if (vsp high)
+ ctrl-vrs[SP] = (unsigned long)vsp;
} else if (insn == 0xb2) {
unsigned long uleb128 = unwind_get_byte(ctrl);
@@ -317,6 +322,8 @@ static int unwind_exec_insn(struct unwind_ctrl_block
*ctrl)
return -URC_FAILURE;
}
+ if (vsp = high
[PATCH] ARM: unwinder: Handle Stackoverflow in unwind_exec_insn
Altough stack overflow is expected in unwind_exec_insn, but in cases when area
beyond stack is not mapped to physical memory this can cause data abort.
To avoid above condition handle stack overflow in unwind_exec_insn by checking
vsp pointer from top of stack
Signed-off-by: Anurag Aggarwal
---
arch/arm/kernel/unwind.c | 23 +++
1 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 00df012..d8b8721 100644
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -241,6 +241,10 @@ static unsigned long unwind_get_byte(struct
unwind_ctrl_block *ctrl)
static int unwind_exec_insn(struct unwind_ctrl_block *ctrl)
{
unsigned long insn = unwind_get_byte(ctrl);
+ unsigned long high, low;
+ unsigned long *vsp = (unsigned long *)ctrl->vrs[SP];
+ low = ctrl->vrs[SP];
+ high = ALIGN(low, THREAD_SIZE);
pr_debug("%s: insn = %08lx\n", __func__, insn);
@@ -263,27 +267,27 @@ static int unwind_exec_insn(struct unwind_ctrl_block
*ctrl)
/* pop R4-R15 according to mask */
load_sp = mask & (1 << (13 - 4));
- while (mask) {
+ while (mask && vsp < high) {
if (mask & 1)
ctrl->vrs[reg] = *vsp++;
mask >>= 1;
reg++;
}
- if (!load_sp)
+ if (!load_sp && vsp < high)
ctrl->vrs[SP] = (unsigned long)vsp;
} else if ((insn & 0xf0) == 0x90 &&
(insn & 0x0d) != 0x0d)
ctrl->vrs[SP] = ctrl->vrs[insn & 0x0f];
else if ((insn & 0xf0) == 0xa0) {
- unsigned long *vsp = (unsigned long *)ctrl->vrs[SP];
int reg;
/* pop R4-R[4+bbb] */
- for (reg = 4; reg <= 4 + (insn & 7); reg++)
+ for (reg = 4; (reg <= 4 + (insn & 7)) && (vsp < high; reg++)
ctrl->vrs[reg] = *vsp++;
- if (insn & 0x80)
+ if (insn & 0x80 && vsp < high)
ctrl->vrs[14] = *vsp++;
- ctrl->vrs[SP] = (unsigned long)vsp;
+ if (vsp < high)
+ ctrl->vrs[SP] = (unsigned long)vsp;
} else if (insn == 0xb0) {
if (ctrl->vrs[PC] == 0)
ctrl->vrs[PC] = ctrl->vrs[LR];
@@ -301,13 +305,14 @@ static int unwind_exec_insn(struct unwind_ctrl_block
*ctrl)
}
/* pop R0-R3 according to mask */
- while (mask) {
+ while (mask && vsp < high) {
if (mask & 1)
ctrl->vrs[reg] = *vsp++;
mask >>= 1;
reg++;
}
- ctrl->vrs[SP] = (unsigned long)vsp;
+ if (vsp < high)
+ ctrl->vrs[SP] = (unsigned long)vsp;
} else if (insn == 0xb2) {
unsigned long uleb128 = unwind_get_byte(ctrl);
@@ -317,6 +322,8 @@ static int unwind_exec_insn(struct unwind_ctrl_block *ctrl)
return -URC_FAILURE;
}
+ if (vsp >= high)
+ return -URC_FAILURE;
pr_debug("%s: fp = %08lx sp = %08lx lr = %08lx pc = %08lx\n", __func__,
ctrl->vrs[FP], ctrl->vrs[SP], ctrl->vrs[LR], ctrl->vrs[PC]);
--
--
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: Handle Stackoverflow in unwind_exec_insn
Altough stack overflow is expected in unwind_exec_insn, but in cases when area
beyond stack is not mapped to physical memory this can cause data abort.
To avoid above condition handle stack overflow in unwind_exec_insn by checking
vsp pointer from top of stack
Signed-off-by: Anurag Aggarwal a.anu...@samsung.com
---
arch/arm/kernel/unwind.c | 23 +++
1 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 00df012..d8b8721 100644
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -241,6 +241,10 @@ static unsigned long unwind_get_byte(struct
unwind_ctrl_block *ctrl)
static int unwind_exec_insn(struct unwind_ctrl_block *ctrl)
{
unsigned long insn = unwind_get_byte(ctrl);
+ unsigned long high, low;
+ unsigned long *vsp = (unsigned long *)ctrl-vrs[SP];
+ low = ctrl-vrs[SP];
+ high = ALIGN(low, THREAD_SIZE);
pr_debug(%s: insn = %08lx\n, __func__, insn);
@@ -263,27 +267,27 @@ static int unwind_exec_insn(struct unwind_ctrl_block
*ctrl)
/* pop R4-R15 according to mask */
load_sp = mask (1 (13 - 4));
- while (mask) {
+ while (mask vsp high) {
if (mask 1)
ctrl-vrs[reg] = *vsp++;
mask = 1;
reg++;
}
- if (!load_sp)
+ if (!load_sp vsp high)
ctrl-vrs[SP] = (unsigned long)vsp;
} else if ((insn 0xf0) == 0x90
(insn 0x0d) != 0x0d)
ctrl-vrs[SP] = ctrl-vrs[insn 0x0f];
else if ((insn 0xf0) == 0xa0) {
- unsigned long *vsp = (unsigned long *)ctrl-vrs[SP];
int reg;
/* pop R4-R[4+bbb] */
- for (reg = 4; reg = 4 + (insn 7); reg++)
+ for (reg = 4; (reg = 4 + (insn 7)) (vsp high; reg++)
ctrl-vrs[reg] = *vsp++;
- if (insn 0x80)
+ if (insn 0x80 vsp high)
ctrl-vrs[14] = *vsp++;
- ctrl-vrs[SP] = (unsigned long)vsp;
+ if (vsp high)
+ ctrl-vrs[SP] = (unsigned long)vsp;
} else if (insn == 0xb0) {
if (ctrl-vrs[PC] == 0)
ctrl-vrs[PC] = ctrl-vrs[LR];
@@ -301,13 +305,14 @@ static int unwind_exec_insn(struct unwind_ctrl_block
*ctrl)
}
/* pop R0-R3 according to mask */
- while (mask) {
+ while (mask vsp high) {
if (mask 1)
ctrl-vrs[reg] = *vsp++;
mask = 1;
reg++;
}
- ctrl-vrs[SP] = (unsigned long)vsp;
+ if (vsp high)
+ ctrl-vrs[SP] = (unsigned long)vsp;
} else if (insn == 0xb2) {
unsigned long uleb128 = unwind_get_byte(ctrl);
@@ -317,6 +322,8 @@ static int unwind_exec_insn(struct unwind_ctrl_block *ctrl)
return -URC_FAILURE;
}
+ if (vsp = high)
+ return -URC_FAILURE;
pr_debug(%s: fp = %08lx sp = %08lx lr = %08lx pc = %08lx\n, __func__,
ctrl-vrs[FP], ctrl-vrs[SP], ctrl-vrs[LR], ctrl-vrs[PC]);
--
--
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: [Query] Stack Overflow in "arch/arm/kernel/unwind.c" while unwinding frame
>From what I saw, it happened when the next page is not mapped to physical memory. I don't think that stack corruption can cause this. >From what I could understand about of the code there is not check if the memory beyond stack is physically mapped or not. To handle the problem I thought that checks can also be added in unwind_exec_insn() function for stack overflow. On Wed, Oct 2, 2013 at 11:41 PM, Catalin Marinas wrote: > On 24 September 2013 07:23, Anurag Aggarwal > wrote: >> While executing unwind backtrace instructions in ARM, in the function >> unwind_exec_insn() >> there are chances that SP overflows from stack. >> >> >> For example while executing instruction with opcode 0xAE, vsp can go >> beyond stack to area that has not been allocated till now. >> >> unsigned long *vsp = (unsigned long *)ctrl->vrs[SP]; >> int reg; >> >> /* pop R4-R[4+bbb] */ >> for (reg = 4; reg <= 4 + (insn & 7); reg++) >> ctrl->vrs[reg] = *vsp++; >> >> The above scenario can happen while executing any of the unwind instruction. >> >> One of the ways to fix the problem is to check for vsp with stack >> limits before we increment it, but doing it for all the instructions >> seems a little bad. >> >> I just want to know that if anyone has faced the problem before > > I haven't seen it but I think with some stack (or unwind bytecode) > corruption it could happen. > > I think we could place some checks only when vsp is assigned and return > -URC_FAILURE, together with some warning. > > -- > Catalin -- 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: [Query] Stack Overflow in arch/arm/kernel/unwind.c while unwinding frame
From what I saw, it happened when the next page is not mapped to physical memory. I don't think that stack corruption can cause this. From what I could understand about of the code there is not check if the memory beyond stack is physically mapped or not. To handle the problem I thought that checks can also be added in unwind_exec_insn() function for stack overflow. On Wed, Oct 2, 2013 at 11:41 PM, Catalin Marinas catalin.mari...@arm.com wrote: On 24 September 2013 07:23, Anurag Aggarwal anurag19aggar...@gmail.com wrote: While executing unwind backtrace instructions in ARM, in the function unwind_exec_insn() there are chances that SP overflows from stack. For example while executing instruction with opcode 0xAE, vsp can go beyond stack to area that has not been allocated till now. unsigned long *vsp = (unsigned long *)ctrl-vrs[SP]; int reg; /* pop R4-R[4+bbb] */ for (reg = 4; reg = 4 + (insn 7); reg++) ctrl-vrs[reg] = *vsp++; The above scenario can happen while executing any of the unwind instruction. One of the ways to fix the problem is to check for vsp with stack limits before we increment it, but doing it for all the instructions seems a little bad. I just want to know that if anyone has faced the problem before I haven't seen it but I think with some stack (or unwind bytecode) corruption it could happen. I think we could place some checks only when vsp is assigned and return -URC_FAILURE, together with some warning. -- Catalin -- 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: [Query] Stack Overflow in "arch/arm/kernel/unwind.c" while unwinding frame
Hi Jean, I don't think that it is related to the warning that you have suggested On Tue, Sep 24, 2013 at 11:58 AM, Jean Pihet wrote: > Hi, > > Adding Russell and l.a.k ML. > > Another question: is this linked to the following build warning? > CC arch/arm/kernel/return_address.o > arch/arm/kernel/return_address.c:63:2: warning: #warning "TODO: > return_address should use unwind tables" > > Regards, > Jean > > On 24 September 2013 07:23, Anurag Aggarwal > wrote: >> Hi All, >> >> While executing unwind backtrace instructions in ARM, in the function >> unwind_exec_insn() >> there are chances that SP overflows from stack. >> >> >> For example while executing instruction with opcode 0xAE, vsp can go >> beyond stack to area that has not been allocated till now. >> >> unsigned long *vsp = (unsigned long *)ctrl->vrs[SP]; >> int reg; >> >> /* pop R4-R[4+bbb] */ >> for (reg = 4; reg <= 4 + (insn & 7); reg++) >> ctrl->vrs[reg] = *vsp++; >> >> The above scenario can happen while executing any of the unwind instruction. >> >> One of the ways to fix the problem is to check for vsp with stack >> limits before we increment it, but doing it for all the instructions >> seems a little bad. >> >> I just want to know that if anyone has faced the problem before >> >> I am working on Linux kernel for Android phones and I saw one case >> when this happened. >> >> I am new to Linux Kernel so not sure if this is the right place to ask >> the question. >> >> >> -- >> 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/ -- 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: [Query] Stack Overflow in arch/arm/kernel/unwind.c while unwinding frame
Hi Jean, I don't think that it is related to the warning that you have suggested On Tue, Sep 24, 2013 at 11:58 AM, Jean Pihet jean.pi...@linaro.org wrote: Hi, Adding Russell and l.a.k ML. Another question: is this linked to the following build warning? CC arch/arm/kernel/return_address.o arch/arm/kernel/return_address.c:63:2: warning: #warning TODO: return_address should use unwind tables Regards, Jean On 24 September 2013 07:23, Anurag Aggarwal anurag19aggar...@gmail.com wrote: Hi All, While executing unwind backtrace instructions in ARM, in the function unwind_exec_insn() there are chances that SP overflows from stack. For example while executing instruction with opcode 0xAE, vsp can go beyond stack to area that has not been allocated till now. unsigned long *vsp = (unsigned long *)ctrl-vrs[SP]; int reg; /* pop R4-R[4+bbb] */ for (reg = 4; reg = 4 + (insn 7); reg++) ctrl-vrs[reg] = *vsp++; The above scenario can happen while executing any of the unwind instruction. One of the ways to fix the problem is to check for vsp with stack limits before we increment it, but doing it for all the instructions seems a little bad. I just want to know that if anyone has faced the problem before I am working on Linux kernel for Android phones and I saw one case when this happened. I am new to Linux Kernel so not sure if this is the right place to ask the question. -- 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/ -- 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/
[Query] Stack Overflow in "arch/arm/kernel/unwind.c" while unwinding frame
Hi All, While executing unwind backtrace instructions in ARM, in the function unwind_exec_insn() there are chances that SP overflows from stack. For example while executing instruction with opcode 0xAE, vsp can go beyond stack to area that has not been allocated till now. unsigned long *vsp = (unsigned long *)ctrl->vrs[SP]; int reg; /* pop R4-R[4+bbb] */ for (reg = 4; reg <= 4 + (insn & 7); reg++) ctrl->vrs[reg] = *vsp++; The above scenario can happen while executing any of the unwind instruction. One of the ways to fix the problem is to check for vsp with stack limits before we increment it, but doing it for all the instructions seems a little bad. I just want to know that if anyone has faced the problem before I am working on Linux kernel for Android phones and I saw one case when this happened. I am new to Linux Kernel so not sure if this is the right place to ask the question. -- 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/
[Query] Stack Overflow in arch/arm/kernel/unwind.c while unwinding frame
Hi All, While executing unwind backtrace instructions in ARM, in the function unwind_exec_insn() there are chances that SP overflows from stack. For example while executing instruction with opcode 0xAE, vsp can go beyond stack to area that has not been allocated till now. unsigned long *vsp = (unsigned long *)ctrl-vrs[SP]; int reg; /* pop R4-R[4+bbb] */ for (reg = 4; reg = 4 + (insn 7); reg++) ctrl-vrs[reg] = *vsp++; The above scenario can happen while executing any of the unwind instruction. One of the ways to fix the problem is to check for vsp with stack limits before we increment it, but doing it for all the instructions seems a little bad. I just want to know that if anyone has faced the problem before I am working on Linux kernel for Android phones and I saw one case when this happened. I am new to Linux Kernel so not sure if this is the right place to ask the question. -- 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/
