[PATCH 1/2] Revert "Fix segfault in arm64_is_kernel_exception_frame() when corrupt stack pointer address is given"
11:37 a.m.
This reverts commit 9868ebc8e648e5791764a51567a23efae7170d9b.
The commit 9868ebc8e648e5791764a51567a23efae7170d9b causes the issue
that bt command fails to show backtraces for the tasks that is running
in the user mode at the moment of the kernel panic as follows:
crash> bt 1734
PID: 1734 TASK: ffff000000392200 CPU: 4 COMMAND: "insmod"
bt: invalid stack pointer is given
The root cause is that while the commit added a sanity check into
STACK_OFFSET_TYPE() to validate if a given candidate address of any
interrupt or exception frame is contained within the range of the
corresponding kernel stack, the premise that the STACK_OFFSET_TYPE()
should not return out-of-the-buffer address, is wrong.
Reexamining the relevant surrounding part of the backtracing code, it
looks to me now that the STACK_OFFSET_TYPE() is originally expected to
return an out-of-the-buffer address, like the address of the top of
the corresponding kernel stack, e.g. at here:
static int
arm64_in_kdump_text(struct bt_info *bt, struct arm64_stackframe *frame)
{
...
if (bt->flags & BT_USER_SPACE)
start = (ulong
*)&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(bt->stacktop))];
else {
Note that the above bt 1734 aborts here.
Hence, the current implementation policy around STACK_OFFSET_TYPE()
looks that the caller side is responsible for understanding the fact
in advance and for avoiding making buffer overrun carefully.
To fix this issue, revert the commit.
Signed-off-by: HATAYAMA Daisuke <d.hatayama(a)fujitsu.com>
---
defs.h | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/defs.h b/defs.h
index bfda0c4..3e7d6cf 100644
--- a/defs.h
+++ b/defs.h
@@ -976,10 +976,7 @@ struct bt_info {
#define STACK_OFFSET_TYPE(OFF) \
(((ulong)(OFF) > STACKSIZE()) ? \
- (((ulong)(OFF) < (ulong)(bt->stackbase) || (ulong)(OFF) >=
(ulong)(bt->stackbase) + STACKSIZE()) ? \
- error(FATAL, "invalid stack pointer is given\n") : \
- (ulong)((ulong)(OFF) - (ulong)(bt->stackbase))) : \
- (ulong)(OFF))
+ (ulong)((ulong)(OFF) - (ulong)(bt->stackbase)) : (ulong)(OFF))
#define GET_STACK_ULONG(OFF) \
*((ulong *)((char *)(&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(OFF))])))
--
2.25.1
11:37 a.m.
New subject: [PATCH 2/2] Fix again segfault in arm64_is_kernel_exception_frame() when corrupt stack pointer address is given
This is the second trial from the commit
9868ebc8e648e5791764a51567a23efae7170d9b that was reverted at the
previous commit.
As described in the previous commit, result of STACK_OFFSET_TYPE() can
be an address out of bt->stackbuf and hence the address needs to be
checked prior to being referred to as an pt_regs object.
So, to fix the issue, let's check if stkptr points to within the range
of the kernel stack first.
Signed-off-by: HATAYAMA Daisuke <d.hatayama(a)fujitsu.com>
---
arm64.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arm64.c b/arm64.c
index efbdccb..ca63fb5 100644
--- a/arm64.c
+++ b/arm64.c
@@ -2381,6 +2381,9 @@ arm64_is_kernel_exception_frame(struct bt_info *bt, ulong stkptr)
struct arm64_pt_regs *regs;
struct machine_specific *ms = machdep->machspec;
+ if (stkptr > STACKSIZE() && !INSTACK(stkptr, bt))
+ return FALSE;
+
regs = (struct arm64_pt_regs
*)&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(stkptr))];
if (INSTACK(regs->sp, bt) && INSTACK(regs->regs[29], bt) &&
--
2.25.1
3:42 a.m.
New subject: [PATCH 2/2] Fix again segfault in arm64_is_kernel_exception_frame() when corrupt stack pointer address is given
On 2023/06/07 18:37, HATAYAMA Daisuke wrote:
This is the second trial from the commit
9868ebc8e648e5791764a51567a23efae7170d9b that was reverted at the
previous commit.
As described in the previous commit, result of STACK_OFFSET_TYPE() can
be an address out of bt->stackbuf and hence the address needs to be
checked prior to being referred to as an pt_regs object.
So, to fix the issue, let's check if stkptr points to within the range
of the kernel stack first.
Signed-off-by: HATAYAMA Daisuke <d.hatayama(a)fujitsu.com>
Looks more cautious than before. For the series,
Acked-by: Kazuhito Hagio <k-hagio-ab(a)nec.com>
Thanks,
Kazu
> ---
> arm64.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arm64.c b/arm64.c
> index efbdccb..ca63fb5 100644
> --- a/arm64.c
> +++ b/arm64.c
> @@ -2381,6 +2381,9 @@ arm64_is_kernel_exception_frame(struct bt_info *bt, ulong
stkptr)
> struct arm64_pt_regs *regs;
> struct machine_specific *ms = machdep->machspec;
>
> + if (stkptr > STACKSIZE() && !INSTACK(stkptr, bt))
> + return FALSE;
> +
> regs = (struct arm64_pt_regs
*)&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(stkptr))];
>
> if (INSTACK(regs->sp, bt) && INSTACK(regs->regs[29], bt) &&
464
days inactive
470
days old
devel@lists.crash-utility.osci.io
2 comments
2 participants
participants (2)
-
HAGIO KAZUHITO(萩尾 一仁) -
HATAYAMA Daisuke