Re: [Crash-utility] [PATCH 2/2] Fix again segfault in arm64_is_kernel_exception_frame() when corrupt stack pointer address is given
5:48 a.m.
On Wed, Jun 7, 2023 at 8:00 PM <crash-utility-request(a)redhat.com> wrote:
Date: Wed, 7 Jun 2023 18:37:34 +0900
From: HATAYAMA Daisuke <d.hatayama(a)fujitsu.com>
To: crash-utility(a)redhat.com
Cc: d.hatayama(a)fujitsu.com
Subject: [Crash-utility] [PATCH 2/2] Fix again segfault in
arm64_is_kernel_exception_frame() when corrupt stack pointer
address
is given
Message-ID: <20230607093734.247-2-d.hatayama(a)fujitsu.com>
Content-Type: text/plain; charset="US-ASCII"; x-default=true
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;
+
I still have one question: Why does this one only need to be fixed, but the
others are not needed(it won't be out of range)? The STACK_OFFSET_TYPE() is
invoked multiple times in arm64.c, and similar calls can be seen on other
arches(grep -nr "GET_STACK_ULONG" *.c or grep -nr "GET_STACK_DATA"
*.c).
# grep -nr "STACK_OFFSET_TYPE" *.c
arm64.c:2384: regs = (struct arm64_pt_regs
*)&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(stkptr))];
arm64.c:2821: ptregs = (struct arm64_pt_regs
*)&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(orig_sp))];
arm64.c:3476: base = (ulong
*)&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(bt->stackbase))];
arm64.c:3478: start = (ulong
*)&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(bt->stacktop))];
arm64.c:3481: start = (ulong
*)&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(frame->fp))];
arm64.c:3483: start = (ulong
*)&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(bt->stacktop))];
arm64.c:3801: &bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(sp))];
arm64.c:3822: &bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(pt_regs))];
x86.c:1075: if (STACK_OFFSET_TYPE(ep->eframe_addr) > STACKSIZE())
[root@hpe-apollo-cn99xx-13-vm-01 crash]# grep -nr "STACK_OFFSET_TYPE" *.h
defs.h:977:#define STACK_OFFSET_TYPE(OFF) \
defs.h:985: *((ulong *)((char
*)(&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(OFF))])))
defs.h:988: (void *)(&bt->stackbuf[(ulong)STACK_OFFSET_TYPE(OFF)]),
(size_t)(SZ))
Thanks.
Lianbo
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
6:43 a.m.
New subject: [PATCH 2/2] Fix again segfault in arm64_is_kernel_exception_frame() when corrupt stack pointer address is given
I still have one question: Why does this one only need to be fixed,
but the others are not needed(it won't be out of range)? The
STACK_OFFSET_TYPE() is invoked multiple times in arm64.c, and
similar calls can be seen on other arches(grep -nr "GET_STACK_ULONG"
*.c or grep -nr "GET_STACK_DATA" *.c).
# grep -nr "STACK_OFFSET_TYPE" *.c
arm64.c:2384: regs = (struct arm64_pt_regs
*)&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(stkptr))];
arm64.c:2821: ptregs = (struct arm64_pt_regs
*)&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(orig_sp))];
arm64.c:3476: base = (ulong
*)&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(bt->stackbase))];
arm64.c:3478: start = (ulong
*)&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(bt->stacktop))];
arm64.c:3481: start = (ulong
*)&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(frame->fp))];
arm64.c:3483: start = (ulong
*)&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(bt->stacktop))];
arm64.c:3801: &bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(sp))];
arm64.c:3822: &bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(pt_regs))];
x86.c:1075: if (STACK_OFFSET_TYPE(ep->eframe_addr) > STACKSIZE())
[root@hpe-apollo-cn99xx-13-vm-01 crash]# grep -nr "STACK_OFFSET_TYPE" *.h
defs.h:977:#define STACK_OFFSET_TYPE(OFF) \
defs.h:985: *((ulong *)((char
*)(&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(OFF))])))
defs.h:988: (void *)(&bt->stackbuf[(ulong)STACK_OFFSET_TYPE(OFF)]),
(size_t)(SZ))
As explained in the patch descriptions, STACK_OFFSET_TYPE() is used in
each occurrences in the different context. To check if each is
implemented correctly needs to understand what each does. I don't know
whether there is another place where returned value of
STACK_OFFSET_TYPE() is handled incorrectly. My quick look didn't find
the part that might touch invalid range of memory. The reason why I'm
trying to fix the arm64_is_kernel_exception_frame() is I found the
issue there.
Thanks.
HATAYAMA, Daisuke
5:25 a.m.
New subject: [PATCH 2/2] Fix again segfault in arm64_is_kernel_exception_frame() when corrupt stack pointer address is given
On Mon, Jun 12, 2023 at 7:44 PM Daisuke Hatayama (Fujitsu) <
d.hatayama(a)fujitsu.com> wrote:
> I still have one question: Why does this one only need to be
fixed,
> but the others are not needed(it won't be out of range)? The
> STACK_OFFSET_TYPE() is invoked multiple times in arm64.c, and
> similar calls can be seen on other arches(grep -nr "GET_STACK_ULONG"
> *.c or grep -nr "GET_STACK_DATA" *.c).
>
> # grep -nr "STACK_OFFSET_TYPE" *.c
> arm64.c:2384: regs = (struct arm64_pt_regs
*)&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(stkptr))];
> arm64.c:2821: ptregs = (struct arm64_pt_regs
*)&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(orig_sp))];
> arm64.c:3476: base = (ulong
*)&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(bt->stackbase))];
> arm64.c:3478: start = (ulong
*)&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(bt->stacktop))];
> arm64.c:3481: start = (ulong
*)&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(frame->fp))];
> arm64.c:3483: start = (ulong
*)&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(bt->stacktop))];
> arm64.c:3801: &bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(sp))];
> arm64.c:3822: &bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(pt_regs))];
> x86.c:1075: if (STACK_OFFSET_TYPE(ep->eframe_addr) > STACKSIZE())
> [root@hpe-apollo-cn99xx-13-vm-01 crash]# grep -nr "STACK_OFFSET_TYPE"
*.h
> defs.h:977:#define STACK_OFFSET_TYPE(OFF) \
> defs.h:985: *((ulong *)((char
*)(&bt->stackbuf[(ulong)(STACK_OFFSET_TYPE(OFF))])))
> defs.h:988: (void *)(&bt->stackbuf[(ulong)STACK_OFFSET_TYPE(OFF)]),
(size_t)(SZ))
As explained in the patch descriptions, STACK_OFFSET_TYPE() is used in
each occurrences in the different context. To check if each is
implemented correctly needs to understand what each does. I don't know
whether there is another place where returned value of
STACK_OFFSET_TYPE() is handled incorrectly. My quick look didn't find
Got it. Thanks.
the part that might touch invalid range of memory. The reason why
I'm
trying to fix the arm64_is_kernel_exception_frame() is I found the
issue there.
So far I haven't observed this issue on my side. As you mentioned, the
corrupt stack pointer address may be related to any kernel bugs or hardware
issues. At least the real reason for the corrupt stack pointer address is
not very clear, so how about adding some debugging information? Just an
idea. HATAYAMA and Kazu.
+ if (stkptr > STACKSIZE() && !INSTACK(stkptr, bt)) {
+ if (CRASHDEBUG(1))
+ error(WARNING, "The stkptr(0x%lx) is an address
outside the range of kernel stack.\n", stkptr);
+ return FALSE;
+ }
+
Thanks.
Lianbo
Thanks.
HATAYAMA, Daisuke
12:38 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/13 19:25, lijiang wrote:
> the part that might touch invalid range of memory. The reason why
I'm
> trying to fix the arm64_is_kernel_exception_frame() is I found the
> issue there.
>
>
So far I haven't observed this issue on my side. As you mentioned, the
corrupt stack pointer address may be related to any kernel bugs or hardware
issues. At least the real reason for the corrupt stack pointer address is
not very clear, so how about adding some debugging information? Just an
idea. HATAYAMA and Kazu.
+ if (stkptr > STACKSIZE() && !INSTACK(stkptr, bt)) {
+ if (CRASHDEBUG(1))
+ error(WARNING, "The stkptr(0x%lx) is an address
outside the range of kernel stack.\n", stkptr);
+ return FALSE;
+ }
+
I cannot test it, but I'm ok with printing a warning with CRASHDEBUG().
I like a shorter one like:
"stkptr: %lx is outside the kernel stack range\n"
Thanks,
Kazu
1:32 a.m.
New subject: [PATCH 2/2] Fix again segfault in arm64_is_kernel_exception_frame() when corrupt stack pointer address is given
On Wed, Jun 14, 2023 at 1:38 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com>
wrote:
On 2023/06/13 19:25, lijiang wrote:
>> the part that might touch invalid range of memory. The reason why I'm
>> trying to fix the arm64_is_kernel_exception_frame() is I found the
>> issue there.
>>
>>
> So far I haven't observed this issue on my side. As you mentioned, the
> corrupt stack pointer address may be related to any kernel bugs or
hardware
> issues. At least the real reason for the corrupt stack pointer address is
> not very clear, so how about adding some debugging information? Just an
> idea. HATAYAMA and Kazu.
>
> + if (stkptr > STACKSIZE() && !INSTACK(stkptr, bt)) {
> + if (CRASHDEBUG(1))
> + error(WARNING, "The stkptr(0x%lx) is an address
> outside the range of kernel stack.\n", stkptr);
> + return FALSE;
> + }
> +
I cannot test it, but I'm ok with printing a warning with CRASHDEBUG().
I like a shorter one like:
"stkptr: %lx is outside the kernel stack range\n"
Thanks, Kazu. Fine to me, for the patch series: Ack (with the above change).
Lianbo
8:45 p.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/14 15:32, lijiang wrote:
> On 2023/06/13 19:25, lijiang wrote:
>>> the part that might touch invalid range of memory. The reason why I'm
>>> trying to fix the arm64_is_kernel_exception_frame() is I found the
>>> issue there.
>>>
>>>
>> So far I haven't observed this issue on my side. As you mentioned, the
>> corrupt stack pointer address may be related to any kernel bugs or
> hardware
>> issues. At least the real reason for the corrupt stack pointer address is
>> not very clear, so how about adding some debugging information? Just an
>> idea. HATAYAMA and Kazu.
>>
>> + if (stkptr > STACKSIZE() && !INSTACK(stkptr, bt)) {
>> + if (CRASHDEBUG(1))
>> + error(WARNING, "The stkptr(0x%lx) is an address
>> outside the range of kernel stack.\n", stkptr);
>> + return FALSE;
>> + }
>> +
>
> I cannot test it, but I'm ok with printing a warning with CRASHDEBUG().
> I like a shorter one like:
> "stkptr: %lx is outside the kernel stack range\n"
>
>
Thanks, Kazu. Fine to me, for the patch series: Ack (with the above change).
ok, applied the patches.
https://github.com/crash-utility/crash/commit/6c8cd9b5dcf48221e5f75fc5850...
Thanks,
Kazu
530
days inactive
536
days old
devel@lists.crash-utility.osci.io
5 comments
3 participants
participants (3)
-
Daisuke Hatayama (Fujitsu) -
HAGIO KAZUHITO(萩尾 一仁) -
lijiang