ARM: perf/oprofile: fix off-by-one in stack check
Subsystems (3)
PATCH 1/1 ARM: perf/oprofile: fix off-by-one in stack check
Since it's fp - 1 that gets passed back in as tail in the next iteration, we need to ensure that fp - 1 is not the same as tail in order to avoid a potential infinite loop in the perf interrupt handler (which has been observed to occur). A similar fix seems to be needed in the OProfile code.
---
Do we need to explicitly check for overflow (buftail.fp - 1 > buftail.fp) also? Though this should be already caught by the access check in the next iteration of the loop.
arch/arm/kernel/perf_event.c | 2 +- arch/arm/oprofile/common.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
arch/arm/kernel/perf_event.c +1 -1
@@ -700,7 +700,7 @@ user_backtrace(struct frame_tail __user *tail,* Frame pointers should strictly progress back up the stack * (towards higher addresses). */ -if (tail >= buftail.fp) +if (tail >= buftail.fp -1 )return NULL ;return buftail.fp -1 ;
arch/arm/oprofile/common.c +1 -1
@@ -85,7 +85,7 @@ static struct frame_tail* user_backtrace(struct frame_tail *tail)/* frame pointers should strictly progress back up the stack * (towards higher addresses) */ -if (tail >= buftail[0 ].fp) +if (tail >= buftail[0 ].fp -1 )return NULL ;return buftail[0 ].fp-1 ;
Signature · 1 line
1.7.2.dirty
PATCH 1/1 ARM: perf/oprofile: fix off-by-one in stack check
Hi Rabin,
Quoted 4 lines
Since it's fp - 1 that gets passed back in as tail in the next iteration, we need to ensure that fp - 1 is not the same as tail in order to avoid a potential infinite loop in the perf interrupt handler (which has been observed to occur). A similar fix seems to be needed in the OProfile code.
Hehe, that's a nasty loop to hit!
Quoted 3 lines
Do we need to explicitly check for overflow (buftail.fp - 1 > buftail.fp) also? Though this should be already caught by the access check in the next iteration of the loop.
I don't think we need to worry about overflow for user backtracing because the permissions should fail before we get that far.
arch/arm/kernel/perf_event.c user_backtrace(struct frame_tail __user *tail, · line 704
Quoted 11 lines
arch/arm/kernel/perf_event.c +1
-1@@ -700,7 +700,7 @@ user_backtrace(struct frame_tail __user *tail,* Frame pointers should strictly progress back up the stack * (towards higher addresses). */ -if (tail >= buftail.fp) +if (tail >= buftail.fp -1 )return NULL ;
For a well formed fp chain, the terminating frame should have a saved NULL frame pointer so it might be more obvious to do tail + 1 >= buftail.fp (although I think it will work either way).
Quoted 6 lines
arch/arm/kernel/perf_event.c +0
-0return buftail.fp -1 ;arch/arm/oprofile/common.c +0
-0
arch/arm/oprofile/common.c static struct frame_tail* user_backtrace(struct frame_tail *tail) · line 91
Quoted 8 lines
arch/arm/oprofile/common.c +1
-1@@ -85,7 +85,7 @@ static struct frame_tail* user_backtrace(struct frame_tail *tail)/* frame pointers should strictly progress back up the stack * (towards higher addresses) */ -if (tail >= buftail[0 ].fp) +if (tail >= buftail[0 ].fp -1 )return NULL ;return buftail[0 ].fp-1 ;
Same here.
Thanks,
Will
PATCH v2 1/1 ARM: perf/oprofile: fix off-by-one in stack check
Since tail is the previous fp - 1, we need to compare the new fp with tail + 1 to ensure that we don't end up passing in the same tail again, in order to avoid a potential infinite loop in the perf interrupt handler (which has been observed to occur). A similar fix seems to be needed in the OProfile code.
---
v2: refined check as per Will's comment
arch/arm/kernel/perf_event.c | 2 +- arch/arm/oprofile/common.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
arch/arm/kernel/perf_event.c +1 -1
@@ -700,7 +700,7 @@ user_backtrace(struct frame_tail __user *tail,* Frame pointers should strictly progress back up the stack * (towards higher addresses). */ -if (tail >= buftail.fp) +if (tail +1 >= buftail.fp)return NULL ;return buftail.fp -1 ;
arch/arm/oprofile/common.c +1 -1
@@ -85,7 +85,7 @@ static struct frame_tail* user_backtrace(struct frame_tail *tail)/* frame pointers should strictly progress back up the stack * (towards higher addresses) */ -if (tail >= buftail[0 ].fp) +if (tail +1 >= buftail[0 ].fp)return NULL ;return buftail[0 ].fp-1 ;
Signature · 1 line
1.7.2.dirty
PATCH v2 1/1 ARM: perf/oprofile: fix off-by-one in stack check
Hi Rabin,
Quoted 5 lines
Since tail is the previous fp - 1, we need to compare the new fp with tail + 1 to ensure that we don't end up passing in the same tail again, in order to avoid a potential infinite loop in the perf interrupt handler (which has been observed to occur). A similar fix seems to be needed in the OProfile code.
Quoted 7 lines
Signed-off-by Rabin Vincent <[email protected]>---
v2: refined check as per Will's commentarch/arm/kernel/perf_event.c | 2 +- arch/arm/oprofile/common.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
Looks good, thanks.
Will