ARM: backtrace: avoid crash on large invalid fp value
PATCH 1/1 ARM: backtrace: avoid crash on large invalid fp value
Invalid frame pointer (signed) -4 <= fp <= -1 defeats check for too high on overflow.
--- arch/arm/kernel/stacktrace.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
arch/arm/kernel/stacktrace.c +1 -1
@@ -31,7 +31,7 @@ int notrace unwind_frame(struct stackframe *frame) high =ALIGN (low,THREAD_SIZE );/* check current frame pointer is within bounds */ -if (fp < (low +12 ) || fp +4 >= high) +if (fp < (low +12 ) || fp >= high -4 )return -EINVAL ;/* restore the registers from the stack frame */
Signature · 1 line
1.7.7.3
PATCH 1/1 ARM: backtrace: avoid crash on large invalid fp value
Quoted 13 lines · Todd Poynor
Invalid frame pointer (signed) -4 <= fp <= -1 defeats check for too high on overflow.
Signed-off-by Todd Poynor <[email protected]>--- arch/arm/kernel/stacktrace.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)arch/arm/kernel/stacktrace.c +0
-0
arch/arm/kernel/stacktrace.c int notrace unwind_frame(struct stackframe *frame) · line 37
Quoted 8 lines
arch/arm/kernel/stacktrace.c +1
-1@@ -31,7 +31,7 @@ int notrace unwind_frame(struct stackframe *frame) high =ALIGN (low,THREAD_SIZE );/* check current frame pointer is within bounds */ -if (fp < (low +12 ) || fp +4 >= high) +if (fp < (low +12 ) || fp >= high -4 )return -EINVAL ;/* restore the registers from the stack frame */
sp and fp can still be complete garbage in the case of a corrupted frame, so low + 12 can still overflow and cause us to read beyond the stack base.
A more robust patch might be as follows. This also checks for misaligned fp and sp values, since those indicate corruption and there can be no sensible way to interpret the resulting frame in that case.
Also, according to the definition of current_thread_info(), IS_ALIGNED(sp, THREAD_SIZE) indicates a full stack extending from sp to sp + THREAD_SIZE, and not an empty stack extending from sp - THREAD_SIZE to sp. We cannot backtrace this situation anyway, since that would imply that the frame record extends beyond the stack... but this patch tidies it up in the interest of clarity.
Cheers
---Dave
(untested)
arch/arm/kernel/stacktrace.c +16 -2
@@ -28,10 +28,20 @@ int notrace unwind_frame(struct stackframe *frame)/* only go to a higher address on the stack */ low = frame->sp; - high =ALIGN (low,THREAD_SIZE ); +if (!IS_ALIGNED (fp,4 )) +return -EINVAL ; + +/* +* low + 1 here ensures that high > sp, consistent with the +* definition of current_thread_info(). +* We subtract 1 to compute the highest allowable byte address. +* Otherwise, we might get high == 0 which would confuse our +* comparisons. +*/ + high =ALIGN (low +1 ,THREAD_SIZE ) -1 ;/* check current frame pointer is within bounds */ -if (fp < (low +12 ) || fp +4 >= high) +if (fp <12 || fp -12 < low || fp > high)return -EINVAL ;/* restore the registers from the stack frame */
@@ -39,6 +49,10 @@ int notrace unwind_frame(struct stackframe *frame) frame->sp = *(unsigned long *)(fp -8 ); frame->pc = *(unsigned long *)(fp -4 ); +/* Do not claim the frame is valid if if is obviously corrupt: */ +if (!IS_ALIGNED (frame->fp,4 )) +return -EINVAL ; + return 0; }#endif
PATCH 1/1 ARM: backtrace: avoid crash on large invalid fp value
Quoted 82 lines · Dave Martin
Quoted 13 lines · Todd Poynor
Invalid frame pointer (signed) -4 <= fp <= -1 defeats check for too high on overflow.
Signed-off-by Todd Poynor <[email protected]>--- arch/arm/kernel/stacktrace.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)arch/arm/kernel/stacktrace.c +0
-0arch/arm/kernel/stacktrace.c int notrace unwind_frame(struct stackframe *frame) · line 37
Quoted 8 lines
arch/arm/kernel/stacktrace.c +1
-1@@ -31,7 +31,7 @@ int notrace unwind_frame(struct stackframe *frame) high =ALIGN (low,THREAD_SIZE );/* check current frame pointer is within bounds */ -if (fp < (low +12 ) || fp +4 >= high) +if (fp < (low +12 ) || fp >= high -4 )return -EINVAL ;/* restore the registers from the stack frame */ sp and fp can still be complete garbage in the case of a corrupted frame, so low + 12 can still overflow and cause us to read beyond the stack base.
A more robust patch might be as follows. This also checks for misaligned fp and sp values, since those indicate corruption and there can be no sensible way to interpret the resulting frame in that case.
Also, according to the definition of
current_thread_info(),IS_ALIGNED(sp, THREAD_SIZE)indicates a full stack extending from sp to sp +THREAD_SIZE, and not an empty stack extending from sp -THREAD_SIZEto sp. We cannot backtrace this situation anyway, since that would imply that the frame record extends beyond the stack... but this patch tidies it up in the interest of clarity.Cheers
---Dave(untested)
arch/arm/kernel/stacktrace.c +16
-2@@ -28,10 +28,20 @@ int notrace unwind_frame(struct stackframe *frame)/* only go to a higher address on the stack */ low = frame->sp; - high =ALIGN (low,THREAD_SIZE ); +if (!IS_ALIGNED (fp,4 )) +return -EINVAL ; + +/* +* low + 1 here ensures that high > sp, consistent with the +* definition of current_thread_info(). +* We subtract 1 to compute the highest allowable byte address. +* Otherwise, we might get high == 0 which would confuse our +* comparisons. +*/ + high =ALIGN (low +1 ,THREAD_SIZE ) -1 ;/* check current frame pointer is within bounds */ -if (fp < (low +12 ) || fp +4 >= high) +if (fp <12 || fp -12 < low || fp > high)return -EINVAL ;/* restore the registers from the stack frame */ @@ -39,6 +49,10 @@ int notrace unwind_frame(struct stackframe *frame) frame->sp = *(unsigned long *)(fp -8 ); frame->pc = *(unsigned long *)(fp -4 ); +/* Do not claim the frame is valid if if is obviously corrupt: */ +if (!IS_ALIGNED (frame->fp,4 )) +return -EINVAL ; + return 0; }#endif
Quoted 4 lines
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Dave or Todd, mind reposting this, or should I squash it into my CONFIG_SMP stacktrace series?
PATCH 1/1 ARM: backtrace: avoid crash on large invalid fp value
Quoted 83 lines · Colin Cross
Quoted 82 lines · Dave Martin
Quoted 13 lines · Todd Poynor
Invalid frame pointer (signed) -4 <= fp <= -1 defeats check for too high on overflow.
Signed-off-by Todd Poynor <[email protected]>--- arch/arm/kernel/stacktrace.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)arch/arm/kernel/stacktrace.c +0
-0arch/arm/kernel/stacktrace.c int notrace unwind_frame(struct stackframe *frame) · line 37
Quoted 8 lines
arch/arm/kernel/stacktrace.c +1
-1@@ -31,7 +31,7 @@ int notrace unwind_frame(struct stackframe *frame) high =ALIGN (low,THREAD_SIZE );/* check current frame pointer is within bounds */ -if (fp < (low +12 ) || fp +4 >= high) +if (fp < (low +12 ) || fp >= high -4 )return -EINVAL ;/* restore the registers from the stack frame */ sp and fp can still be complete garbage in the case of a corrupted frame, so low + 12 can still overflow and cause us to read beyond the stack base.
A more robust patch might be as follows. This also checks for misaligned fp and sp values, since those indicate corruption and there can be no sensible way to interpret the resulting frame in that case.
Also, according to the definition of
current_thread_info(),IS_ALIGNED(sp, THREAD_SIZE)indicates a full stack extending from sp to sp +THREAD_SIZE, and not an empty stack extending from sp -THREAD_SIZEto sp. We cannot backtrace this situation anyway, since that would imply that the frame record extends beyond the stack... but this patch tidies it up in the interest of clarity.Cheers
---Dave(untested)
arch/arm/kernel/stacktrace.c +16
-2@@ -28,10 +28,20 @@ int notrace unwind_frame(struct stackframe *frame)/* only go to a higher address on the stack */ low = frame->sp; - high =ALIGN (low,THREAD_SIZE ); +if (!IS_ALIGNED (fp,4 )) +return -EINVAL ; + +/* +* low + 1 here ensures that high > sp, consistent with the +* definition of current_thread_info(). +* We subtract 1 to compute the highest allowable byte address. +* Otherwise, we might get high == 0 which would confuse our +* comparisons. +*/ + high =ALIGN (low +1 ,THREAD_SIZE ) -1 ;/* check current frame pointer is within bounds */ -if (fp < (low +12 ) || fp +4 >= high) +if (fp <12 || fp -12 < low || fp > high)return -EINVAL ;/* restore the registers from the stack frame */ @@ -39,6 +49,10 @@ int notrace unwind_frame(struct stackframe *frame) frame->sp = *(unsigned long *)(fp -8 ); frame->pc = *(unsigned long *)(fp -4 ); +/* Do not claim the frame is valid if if is obviously corrupt: */ +if (!IS_ALIGNED (frame->fp,4 )) +return -EINVAL ; + return 0; }#endif
Quoted 7 lines
Quoted 4 lines
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernelDave or Todd, mind reposting this, or should I squash it into my
CONFIG_SMPstacktrace series?
I'm happy for you to fold my patch into your series if you agree with it. Ideally, please fix my typo in the final comment ("if IT is obviously corrupt").
Do I assume correctly that you are already testing this stuff?
Cheers
---Dave
PATCH 1/1 ARM: backtrace: avoid crash on large invalid fp value
Quoted 50 lines · Dave Martin
Quoted 49 lines · Colin Cross
Quoted 48 lines · Dave Martin
Quoted 13 lines · Todd Poynor
Invalid frame pointer (signed) -4 <= fp <= -1 defeats check for too high on overflow.
Signed-off-by Todd Poynor <[email protected]>--- arch/arm/kernel/stacktrace.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)arch/arm/kernel/stacktrace.c +0
-0arch/arm/kernel/stacktrace.c int notrace unwind_frame(struct stackframe *frame) · line 37
Quoted 8 lines
arch/arm/kernel/stacktrace.c +1
-1@@ -31,7 +31,7 @@ int notrace unwind_frame(struct stackframe *frame) high =ALIGN (low,THREAD_SIZE );/* check current frame pointer is within bounds */ -if (fp < (low +12 ) || fp +4 >= high) +if (fp < (low +12 ) || fp >= high -4 )return -EINVAL ;/* restore the registers from the stack frame */ sp and fp can still be complete garbage in the case of a corrupted frame, so low + 12 can still overflow and cause us to read beyond the stack base.
A more robust patch might be as follows. This also checks for misaligned fp and sp values, since those indicate corruption and there can be no sensible way to interpret the resulting frame in that case.
Also, according to the definition of
current_thread_info(),IS_ALIGNED(sp, THREAD_SIZE)indicates a full stack extending from sp to sp +THREAD_SIZE, and not an empty stack extending from sp -THREAD_SIZEto sp. We cannot backtrace this situation anyway, since that would imply that the frame record extends beyond the stack... but this patch tidies it up in the interest of clarity.Cheers
---Dave(untested)
arch/arm/kernel/stacktrace.c +0
-0@@ -28,10 +28,20 @@ int notrace unwind_frame(struct stackframe *frame)
Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
arch/arm/kernel/stacktrace.c +3
-1/* only go to a higher address on the stack */ low = frame->sp; - high =ALIGN (low,THREAD_SIZE ); +if (!IS_ALIGNED (fp,4 )) +return -EINVAL ; +Quoted 8 lines
arch/arm/kernel/stacktrace.c int notrace unwind_frame(struct stackframe *frame) · line 41
Quoted 8 lines
arch/arm/kernel/stacktrace.c +8
-0+/* +* low + 1 here ensures that high > sp, consistent with the +* definition of current_thread_info(). +* We subtract 1 to compute the highest allowable byte address. +* Otherwise, we might get high == 0 which would confuse our +* comparisons. +*/ + high =ALIGN (low +1 ,THREAD_SIZE ) -1 ;
ARM eabi stacks are full-descending, meaning that if the sp is a multiple of THREAD_SIZE, the stack is empty. current_thread_info takes a short-cut and assumes it can never be called on an empty stack, but better not to propagate that anywhere else.
Quoted 28 lines
Quoted 20 lines
Quoted 20 lines
arch/arm/kernel/stacktrace.c +5
-1/* check current frame pointer is within bounds */ -if (fp < (low +12 ) || fp +4 >= high) +if (fp <12 || fp -12 < low || fp > high)return -EINVAL ;/* restore the registers from the stack frame */ @@ -39,6 +49,10 @@ int notrace unwind_frame(struct stackframe *frame) frame->sp = *(unsigned long *)(fp -8 ); frame->pc = *(unsigned long *)(fp -4 ); +/* Do not claim the frame is valid if if is obviously corrupt: */ +if (!IS_ALIGNED (frame->fp,4 )) +return -EINVAL ; + return 0; }#endif Quoted 7 lines
Quoted 4 lines
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernelDave or Todd, mind reposting this, or should I squash it into my
CONFIG_SMPstacktrace series?
Quoted 5 lines
I'm happy for you to fold my patch into your series if you agree with it. Ideally, please fix my typo in the final comment ("if IT is obviously corrupt").
Do I assume correctly that you are already testing this stuff?
I've been testing it by repeatedly dumping the stack of a running thread (cat /dev/urandom > /dev/null) and making sure it doesn't panic, and by dumping all the threads in a idle system and making sure they all end at the normal user or kernel thread initial frames (do_exit, kernel_thread_exit, or ret_fast_syscall).
PATCH 1/1 ARM: backtrace: avoid crash on large invalid fp value
Quoted 62 lines · Colin Cross
Quoted 61 lines · Dave Martin
Quoted 60 lines · Colin Cross
Quoted 59 lines · Dave Martin
Quoted 13 lines · Todd Poynor
Invalid frame pointer (signed) -4 <= fp <= -1 defeats check for too high on overflow.
Signed-off-by Todd Poynor <[email protected]>--- arch/arm/kernel/stacktrace.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)arch/arm/kernel/stacktrace.c +0
-0arch/arm/kernel/stacktrace.c int notrace unwind_frame(struct stackframe *frame) · line 37
Quoted 8 lines
arch/arm/kernel/stacktrace.c +1
-1@@ -31,7 +31,7 @@ int notrace unwind_frame(struct stackframe *frame) high =ALIGN (low,THREAD_SIZE );/* check current frame pointer is within bounds */ -if (fp < (low +12 ) || fp +4 >= high) +if (fp < (low +12 ) || fp >= high -4 )return -EINVAL ;/* restore the registers from the stack frame */ sp and fp can still be complete garbage in the case of a corrupted frame, so low + 12 can still overflow and cause us to read beyond the stack base.
A more robust patch might be as follows. This also checks for misaligned fp and sp values, since those indicate corruption and there can be no sensible way to interpret the resulting frame in that case.
Also, according to the definition of
current_thread_info(),IS_ALIGNED(sp, THREAD_SIZE)indicates a full stack extending from sp to sp +THREAD_SIZE, and not an empty stack extending from sp -THREAD_SIZEto sp. We cannot backtrace this situation anyway, since that would imply that the frame record extends beyond the stack... but this patch tidies it up in the interest of clarity.Cheers
---Dave(untested)
arch/arm/kernel/stacktrace.c +8
-1@@ -28,10 +28,20 @@ int notrace unwind_frame(struct stackframe *frame)/* only go to a higher address on the stack */ low = frame->sp; - high =ALIGN (low,THREAD_SIZE ); +if (!IS_ALIGNED (fp,4 )) +return -EINVAL ; + +/* +* low + 1 here ensures that high > sp, consistent with the +* definition of current_thread_info(). +* We subtract 1 to compute the highest allowable byte address. +* Otherwise, we might get high == 0 which would confuse our
Quoted 8 lines
Quoted 3 lines
Quoted 3 lines
arch/arm/kernel/stacktrace.c int notrace unwind_frame(struct stackframe *frame) · line 41
Quoted 3 lines
arch/arm/kernel/stacktrace.c +3
-0+* comparisons. +*/ + high =ALIGN (low +1 ,THREAD_SIZE ) -1 ;ARM eabi stacks are full-descending, meaning that if the sp is a multiple of
THREAD_SIZE, the stack is empty.current_thread_infotakes a short-cut and assumes it can never be called on an empty stack, but better not to propagate that anywhere else.
The effect of the code is consistent with current_thread_info():
low = THREAD_SIZE * X --> high = THREAD_SIZE * (X + 1) - 1 low = THREAD_SIZE * (X + 1) - 1 --> high = THREAD_SIZE * (X + 1) - 1
i.e., low = THREAD_SIZE * X is treated as a full stack.
The comment relates to the case where the stack is right at the top of the address space: if we define high as ALIGN(low + 1, THREAD_SIZE), then high overflow to zero in this case, giving unexpected results for comparisons "some_address >= high".
Definig high as the address of the last byte of the stack (instead of the first byte after the stack) avoids this kind of problem, providing that "some_address >= high" is rewritten as "some_address > high" in our comparisons.
I don't know whether any stack will be at the top of the address space in practice, but I prefer to avoid unnecessary assumptions where possible.
Do you agree with the code as-is, or does something need to be changed/ clarified?
Quoted 32 lines
Quoted 32 lines
Quoted 20 lines
Quoted 20 lines
arch/arm/kernel/stacktrace.c +5
-1/* check current frame pointer is within bounds */ -if (fp < (low +12 ) || fp +4 >= high) +if (fp <12 || fp -12 < low || fp > high)return -EINVAL ;/* restore the registers from the stack frame */ @@ -39,6 +49,10 @@ int notrace unwind_frame(struct stackframe *frame) frame->sp = *(unsigned long *)(fp -8 ); frame->pc = *(unsigned long *)(fp -4 ); +/* Do not claim the frame is valid if if is obviously corrupt: */ +if (!IS_ALIGNED (frame->fp,4 )) +return -EINVAL ; + return 0; }#endif Quoted 7 lines
Quoted 4 lines
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernelDave or Todd, mind reposting this, or should I squash it into my
CONFIG_SMPstacktrace series?I'm happy for you to fold my patch into your series if you agree with it. Ideally, please fix my typo in the final comment ("if IT is obviously corrupt").
Quoted 7 lines
Quoted 1 line
Do I assume correctly that you are already testing this stuff?
I've been testing it by repeatedly dumping the stack of a running thread (cat
/dev/urandom>/dev/null) and making sure it doesn't panic, and by dumping all the threads in a idle system and making sure they all end at the normal user or kernel thread initial frames (do_exit,kernel_thread_exit, orret_fast_syscall).
OK -- that's good to know.
I'm still assuming that you're rolling this into your series. Let me know if you want me to post a separate patch.
Cheers
---Dave
PATCH 1/1 ARM: backtrace: avoid crash on large invalid fp value
Quoted 72 lines · Dave Martin
Quoted 62 lines · Colin Cross
Quoted 61 lines · Dave Martin
Quoted 60 lines · Colin Cross
Quoted 59 lines · Dave Martin
Quoted 13 lines · Todd Poynor
Invalid frame pointer (signed) -4 <= fp <= -1 defeats check for too high on overflow.
Signed-off-by Todd Poynor <[email protected]>--- arch/arm/kernel/stacktrace.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)arch/arm/kernel/stacktrace.c +0
-0arch/arm/kernel/stacktrace.c int notrace unwind_frame(struct stackframe *frame) · line 37
Quoted 8 lines
arch/arm/kernel/stacktrace.c +1
-1@@ -31,7 +31,7 @@ int notrace unwind_frame(struct stackframe *frame) high =ALIGN (low,THREAD_SIZE );/* check current frame pointer is within bounds */ -if (fp < (low +12 ) || fp +4 >= high) +if (fp < (low +12 ) || fp >= high -4 )return -EINVAL ;/* restore the registers from the stack frame */ sp and fp can still be complete garbage in the case of a corrupted frame, so low + 12 can still overflow and cause us to read beyond the stack base.
A more robust patch might be as follows. This also checks for misaligned fp and sp values, since those indicate corruption and there can be no sensible way to interpret the resulting frame in that case.
Also, according to the definition of
current_thread_info(),IS_ALIGNED(sp, THREAD_SIZE)indicates a full stack extending from sp to sp +THREAD_SIZE, and not an empty stack extending from sp -THREAD_SIZEto sp. We cannot backtrace this situation anyway, since that would imply that the frame record extends beyond the stack... but this patch tidies it up in the interest of clarity.Cheers
---Dave(untested)
arch/arm/kernel/stacktrace.c +8
-1@@ -28,10 +28,20 @@ int notrace unwind_frame(struct stackframe *frame)/* only go to a higher address on the stack */ low = frame->sp; - high =ALIGN (low,THREAD_SIZE ); +if (!IS_ALIGNED (fp,4 )) +return -EINVAL ; + +/* +* low + 1 here ensures that high > sp, consistent with the +* definition of current_thread_info(). +* We subtract 1 to compute the highest allowable byte address. +* Otherwise, we might get high == 0 which would confuse our Quoted 8 lines
Quoted 3 lines
Quoted 3 lines
arch/arm/kernel/stacktrace.c int notrace unwind_frame(struct stackframe *frame) · line 41
Quoted 3 lines
arch/arm/kernel/stacktrace.c +3
-0+* comparisons. +*/ + high =ALIGN (low +1 ,THREAD_SIZE ) -1 ;ARM eabi stacks are full-descending, meaning that if the sp is a multiple of
THREAD_SIZE, the stack is empty.current_thread_infotakes a short-cut and assumes it can never be called on an empty stack, but better not to propagate that anywhere else.
Quoted 6 lines
The effect of the code is consistent with
current_thread_info():low = THREAD_SIZE * X --> high = THREAD_SIZE * (X + 1) - 1 low = THREAD_SIZE * (X + 1) - 1 --> high = THREAD_SIZE * (X + 1) - 1i.e., low =
THREAD_SIZE* X is treated as a full stack.
current_thread_info() is assuming a sane stack, where the sp is between [THREAD_SIZE * X + sizeof(struct thread_info), THREAD_SIZE * (X + 1) - 8] (see THREAD_START_SP). It should never see sp = THREAD_SIZE * X, so we shouldn't be copying its behavior in that case.
sp = THREAD_SIZE * x being a full stack would mean that the stack has passed all the way through the struct thread_info stored at the lower addresses of the stack, corrupting the task struct, saved registers, and likely the stack too. On the other hand, sp = THREAD_SIZE * x being an empty stack would mean somebody started a stack higher than THREAD_START_SP. Neither one really makes sense, maybe I should just validate the sp above the thread_info and below THREAD_START_SP.
Quoted 9 lines
The comment relates to the case where the stack is right at the top of the address space: if we define high as
ALIGN(low + 1, THREAD_SIZE), then high overflow to zero in this case, giving unexpected results for comparisons "some_address>= high".Definig high as the address of the last byte of the stack (instead of the first byte after the stack) avoids this kind of problem, providing that "
some_address>= high" is rewritten as "some_address> high" in our comparisons.
I agree with using - 1 (or - 4) to prevent high wrapping, but maybe capping at THREAD_START_SP would simplify the code.
Quoted 48 lines
I don't know whether any stack will be at the top of the address space in practice, but I prefer to avoid unnecessary assumptions where possible.
Do you agree with the code as-is, or does something need to be changed/ clarified?
Quoted 32 lines
Quoted 32 lines
Quoted 20 lines
Quoted 20 lines
arch/arm/kernel/stacktrace.c +5
-1/* check current frame pointer is within bounds */ -if (fp < (low +12 ) || fp +4 >= high) +if (fp <12 || fp -12 < low || fp > high)return -EINVAL ;/* restore the registers from the stack frame */ @@ -39,6 +49,10 @@ int notrace unwind_frame(struct stackframe *frame) frame->sp = *(unsigned long *)(fp -8 ); frame->pc = *(unsigned long *)(fp -4 ); +/* Do not claim the frame is valid if if is obviously corrupt: */ +if (!IS_ALIGNED (frame->fp,4 )) +return -EINVAL ; + return 0; }#endif Quoted 7 lines
Quoted 4 lines
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernelDave or Todd, mind reposting this, or should I squash it into my
CONFIG_SMPstacktrace series?I'm happy for you to fold my patch into your series if you agree with it. Ideally, please fix my typo in the final comment ("if IT is obviously corrupt").
Quoted 7 lines
Quoted 1 line
Do I assume correctly that you are already testing this stuff?
I've been testing it by repeatedly dumping the stack of a running thread (cat
/dev/urandom>/dev/null) and making sure it doesn't panic, and by dumping all the threads in a idle system and making sure they all end at the normal user or kernel thread initial frames (do_exit,kernel_thread_exit, orret_fast_syscall).
Quoted 4 lines
OK -- that's good to know.
I'm still assuming that you're rolling this into your series. Let me know if you want me to post a separate patch.
I'll squash it in to mine.
PATCH 1/1 ARM: backtrace: avoid crash on large invalid fp value
Quoted 98 lines · Colin Cross
Quoted 72 lines · Dave Martin
Quoted 62 lines · Colin Cross
Quoted 61 lines · Dave Martin
Quoted 60 lines · Colin Cross
Quoted 59 lines · Dave Martin
Quoted 13 lines · Todd Poynor
Invalid frame pointer (signed) -4 <= fp <= -1 defeats check for too high on overflow.
Signed-off-by Todd Poynor <[email protected]>--- arch/arm/kernel/stacktrace.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)arch/arm/kernel/stacktrace.c +0
-0arch/arm/kernel/stacktrace.c int notrace unwind_frame(struct stackframe *frame) · line 37
Quoted 8 lines
arch/arm/kernel/stacktrace.c +1
-1@@ -31,7 +31,7 @@ int notrace unwind_frame(struct stackframe *frame) high =ALIGN (low,THREAD_SIZE );/* check current frame pointer is within bounds */ -if (fp < (low +12 ) || fp +4 >= high) +if (fp < (low +12 ) || fp >= high -4 )return -EINVAL ;/* restore the registers from the stack frame */ sp and fp can still be complete garbage in the case of a corrupted frame, so low + 12 can still overflow and cause us to read beyond the stack base.
A more robust patch might be as follows. This also checks for misaligned fp and sp values, since those indicate corruption and there can be no sensible way to interpret the resulting frame in that case.
Also, according to the definition of
current_thread_info(),IS_ALIGNED(sp, THREAD_SIZE)indicates a full stack extending from sp to sp +THREAD_SIZE, and not an empty stack extending from sp -THREAD_SIZEto sp. We cannot backtrace this situation anyway, since that would imply that the frame record extends beyond the stack... but this patch tidies it up in the interest of clarity.Cheers
---Dave(untested)
arch/arm/kernel/stacktrace.c +8
-1@@ -28,10 +28,20 @@ int notrace unwind_frame(struct stackframe *frame)/* only go to a higher address on the stack */ low = frame->sp; - high =ALIGN (low,THREAD_SIZE ); +if (!IS_ALIGNED (fp,4 )) +return -EINVAL ; + +/* +* low + 1 here ensures that high > sp, consistent with the +* definition of current_thread_info(). +* We subtract 1 to compute the highest allowable byte address. +* Otherwise, we might get high == 0 which would confuse our Quoted 8 lines
Quoted 3 lines
Quoted 3 lines
arch/arm/kernel/stacktrace.c int notrace unwind_frame(struct stackframe *frame) · line 41
Quoted 3 lines
arch/arm/kernel/stacktrace.c +3
-0+* comparisons. +*/ + high =ALIGN (low +1 ,THREAD_SIZE ) -1 ;ARM eabi stacks are full-descending, meaning that if the sp is a multiple of
THREAD_SIZE, the stack is empty.current_thread_infotakes a short-cut and assumes it can never be called on an empty stack, but better not to propagate that anywhere else.Quoted 6 lines
The effect of the code is consistent with
current_thread_info():low = THREAD_SIZE * X --> high = THREAD_SIZE * (X + 1) - 1 low = THREAD_SIZE * (X + 1) - 1 --> high = THREAD_SIZE * (X + 1) - 1i.e., low =
THREAD_SIZE* X is treated as a full stack.
current_thread_info()is assuming a sane stack, where the sp is between [THREAD_SIZE* X +sizeof(struct thread_info),THREAD_SIZE* (X + 1) - 8] (seeTHREAD_START_SP). It should never see sp =THREAD_SIZE* X, so we shouldn't be copying its behavior in that case.sp =
THREAD_SIZE* x being a full stack would mean that the stack has passed all the way through the structthread_infostored at the lower addresses of the stack, corrupting the task struct, saved registers, and likely the stack too. On the other hand, sp =THREAD_SIZE* x being an empty stack would mean somebody started a stack higher thanTHREAD_START_SP. Neither one really makes sense, maybe I should just validate the sp above thethread_infoand belowTHREAD_START_SP.Quoted 5 lines
The comment relates to the case where the stack is right at the top of the address space: if we define high as
ALIGN(low + 1, THREAD_SIZE), then high overflow to zero in this case, giving unexpected results for comparisons "some_address>= high".
Quoted 7 lines
Quoted 4 lines
Definig high as the address of the last byte of the stack (instead of the first byte after the stack) avoids this kind of problem, providing that "
some_address>= high" is rewritten as "some_address> high" in our comparisons.I agree with using - 1 (or - 4) to prevent high wrapping, but maybe capping at
THREAD_START_SPwould simplify the code.
Having a more precise check as you describe seems to be a good thing.
I'm happy to go with your judgement.
[...]
Cheers
---Dave