[Crash-utility] Re: [PATCH] x86_64: check bt->bptr before calculate framesize

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(萩尾　一仁)&lt;k-hagio-ab(a)nec.com&gt; > Subject: [Crash-utility] Re: [PATCH] x86_64: check bt->bptr before >     calculate framesize > To: Tao Liu&lt;ltao@redhat.com&gt;,&quot;devel(a)lists.crash-utility.osci.io&quot; >     &lt;devel(a)lists.crash-utility.osci.io&gt; > 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&lt;ltao(a)redhat.com&gt; >> --- >>    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