[Crash-utility] Re: [PATCH] x86_64: check bt->bptr before calculate framesize
Hi Kazu and Lianbo,
On Wed, Dec 27, 2023 at 4:27 PM HAGIO KAZUHITO(萩尾 一仁)
<k-hagio-ab(a)nec.com> wrote:
On 2023/12/26 20:55, Lianbo Jiang wrote:
> On 12/26/23 14:43, devel-request(a)lists.crash-utility.osci.io wrote:
>
>> Date: Tue, 26 Dec 2023 06:42:55 +0000
>> From: HAGIO KAZUHITO(萩尾 一仁)<k-hagio-ab(a)nec.com>
>> Subject: [Crash-utility] Re: [PATCH] x86_64: check bt->bptr before
>> calculate framesize
>> To: Tao
Liu<ltao@redhat.com>,"devel(a)lists.crash-utility.osci.io"
>> <devel(a)lists.crash-utility.osci.io>
>> Message-ID:<58d63b19-0d8a-3be0-234a-703a4c25dcb9@nec.com>
>> Content-Type: text/plain; charset="utf-8"
>>
>> On 2023/12/26 10:19, Tao Liu wrote:
>>> Previously the value of bt->bptr is not checked, which may led to a
>>> wrong prev_sp and framesize. As a result, bt->stackbuf[] will be
>>> accessed out of range, and segfault.
>>>
>>> Before:
>>> crash> set debug 1
>>> crash> bt
>>> ...snip...
>>> --- <NMI exception stack> ---
>>> #8 [ffffffff9a603e10] __switch_to_asm at ffffffff99800214
>>> rsp: ffffffff9a603e10 textaddr: ffffffff99800214 -> spo: 0 bpo: 0 spr: 0
bpr: 0 type: 0 end: 0
>>> #9 [ffffffff9a603e40] __schedule at ffffffff9960dfb1
>>> rsp: ffffffff9a603e40 textaddr: ffffffff9960dfb1 -> spo: 16 bpo: -16 spr:
4 bpr: 1 type: 0 end: 0
>>> rsp: ffffffff9a603e40 rbp: ffffb9ca076e7ca8 prev_sp: ffffb9ca076e7cb8
framesize: 1829650024
>>> Segmentation fault (core dumped)
>>>
>>> (gdb) p/x bt->stackbase
>>> $1 = 0xffffffff9a600000
>>> (gdb) p/x bt->stacktop
>>> $2 = 0xffffffff9a604000
>>>
>>> After:
>>> crash> set debug 1
>>> crash> bt
>>> ...snip...
>>> --- <NMI exception stack> ---
>>> #8 [ffffffff9a603e10] __switch_to_asm at ffffffff99800214
>>> rsp: ffffffff9a603e10 textaddr: ffffffff99800214 -> spo: 0 bpo: 0 spr: 0
bpr: 0 type: 0 end: 0
>>> #9 [ffffffff9a603e40] __schedule at ffffffff9960dfb1
>>> rsp: ffffffff9a603e40 textaddr: ffffffff9960dfb1 -> spo: 16 bpo: -16 spr:
4 bpr: 1 type: 0 end: 0
>>> #10 [ffffffff9a603e98] schedule_idle at ffffffff9960e87c
>>> rsp: ffffffff9a603e98 textaddr: ffffffff9960e87c -> spo: 8 bpo: 0 spr: 5
bpr: 0 type: 0 end: 0
>>> rsp: ffffffff9a603e98 prev_sp: ffffffff9a603ea8 framesize: 0
>>> ...snip...
>>>
>>> This patch will check bt->bptr value before calculate framesize. Only
bt->bptr
>>> falls into the range of bt->stackbase and bt->stacktop will be
>>> regarded as valid.
>>>
>>> Signed-off-by: Tao Liu<ltao(a)redhat.com>
>>> ---
>>> x86_64.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/x86_64.c b/x86_64.c
>>> index 8abe39d..ff1ba6e 100644
>>> --- a/x86_64.c
>>> +++ b/x86_64.c
>>> @@ -8707,7 +8707,8 @@ x86_64_get_framesize(struct bt_info *bt, ulong
textaddr, ulong rsp, char *stack_
>>> if (CRASHDEBUG(1))
>>> fprintf(fp, "rsp: %lx prev_sp: %lx framesize:
%d\n",
>>> rsp, prev_sp, framesize);
>>> - } else if ((korc->sp_reg == ORC_REG_BP) &&
bt->bptr) {
>>> + } else if ((korc->sp_reg == ORC_REG_BP) &&
bt->bptr &&
>>> + bt->bptr >= bt->stackbase &&
bt->bptr <= bt->stacktop) {
>> Thank you for looking into this! Looks good to me.
>>
>> Just a nit, INSTACK() can be used?
>
> The INSTACK(bt->bptr, bt) is better, let's go with this, Kazu. Tao is on
PTO.
>
> For the patch with the above change: Ack
Thanks for the patch improvement, I wasn't aware of the INSTACK(), so
the improvement is better!
Thanks,
Tao Liu
Thanks, applied with INSTACK().
https://github.com/crash-utility/crash/commit/53d2577cef98b76b122aade9434...
Kazu