Thank you for pointing out this issue, Kazu.

Tao and Kazu, Can I add "Reported-by" from Kazu?

On Fri, Jan 24, 2025 at 10:39 AM <devel-requ...@lists.crash-utility.osci.io>
wrote:

> Date: Fri, 24 Jan 2025 00:11:19 +1300
> From: Tao Liu <l...@redhat.com>
> Subject: [Crash-utility] [PATCH] x86_64: Fix 'bt -S/-I' segfault issue
> To: devel@lists.crash-utility.osci.io
> Cc: Tao Liu <l...@redhat.com>
> Message-ID: <20250123111119.139008-1-l...@redhat.com>
> Content-Type: text/plain; charset="US-ASCII"; x-default=true
>
> The "pcp" and "spp" can be NULL for "bt -S/-I", but the value is not
> checked before pointer dereference, which lead to segfault error.
> This patch fix this by delay the pointer dereference after
> x86_64_get_dumpfile_stack_frame().
>
>
This looks good to me, so: Ack.

Thanks
Lianbo


> Before:
> crash> bt -S ffffbccee0087ab8
> PID: 2179     TASK: ffffa0f78912a3c0  CPU: 43   COMMAND: "bash"
> Segmentation fault (core dumped)
>
> After:
> crash> bt -S ffffbccee0087ab8
> PID: 2179     TASK: ffffa0f78912a3c0  CPU: 43   COMMAND: "bash"
>  #0 [ffffbccee0087b10] __crash_kexec at ffffffffad20c30a
>  #1 [ffffbccee0087bd0] panic at ffffffffadcbce30
>  #2 [ffffbccee0087c50] sysrq_handle_crash at ffffffffad802b86
>  ...
>
> Signed-off-by: Tao Liu <l...@redhat.com>
> ---
>  x86_64.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/x86_64.c b/x86_64.c
> index c254c6e..d4bbd15 100644
> --- a/x86_64.c
> +++ b/x86_64.c
> @@ -5009,16 +5009,11 @@ static void
>  x86_64_get_stack_frame(struct bt_info *bt, ulong *pcp, ulong *spp)
>  {
>         struct user_regs_bitmap_struct *ur_bitmap;
> -       ulong pcp_save = *pcp;
> -       ulong spp_save = *spp;
> -       ulong sp;
> +       ulong sp = 0;
>
>         if (bt->flags & BT_SKIP_IDLE)
>                 bt->flags &= ~BT_SKIP_IDLE;
> -       if (pcp)
> -               *pcp = x86_64_get_pc(bt);
> -       if (spp)
> -               sp = *spp = x86_64_get_sp(bt);
> +
>         ur_bitmap = (struct user_regs_bitmap_struct
> *)GETBUF(sizeof(*ur_bitmap));
>         memset(ur_bitmap, 0, sizeof(*ur_bitmap));
>
> @@ -5091,31 +5086,36 @@ x86_64_get_stack_frame(struct bt_info *bt, ulong
> *pcp, ulong *spp)
>                         if (bt->flags & BT_DUMPFILE_SEARCH) {
>                                 FREEBUF(ur_bitmap);
>                                 bt->need_free = FALSE;
> -                               *pcp = pcp_save;
> -                               *spp = spp_save;
>                                 return x86_64_get_dumpfile_stack_frame(bt,
> pcp, spp);
>                         }
>                 }
>         } else {
>                 if (!is_task_active(bt->task)) {
> -                       readmem(*spp, KVADDR, &(ur_bitmap->ur.bp),
> -                               sizeof(ulong), "ur_bitmap->ur.bp",
> FAULT_ON_ERROR);
> -                       SET_REG_BITMAP(ur_bitmap->bitmap,
> x86_64_user_regs_struct, bp);
> +                       if (spp) {
> +                               *spp = x86_64_get_sp(bt);
> +                               readmem(*spp, KVADDR, &(ur_bitmap->ur.bp),
> +                                       sizeof(ulong), "ur_bitmap->ur.bp",
> FAULT_ON_ERROR);
> +                               SET_REG_BITMAP(ur_bitmap->bitmap,
> x86_64_user_regs_struct, bp);
> +                       }
>                 } else {
>                         if (bt->flags & BT_DUMPFILE_SEARCH) {
>                                 FREEBUF(ur_bitmap);
>                                 bt->need_free = FALSE;
> -                               *pcp = pcp_save;
> -                               *spp = spp_save;
>                                 return x86_64_get_dumpfile_stack_frame(bt,
> pcp, spp);
>                         }
>                 }
>         }
>
> -       ur_bitmap->ur.ip = *pcp;
> -       ur_bitmap->ur.sp = sp;
> -       SET_REG_BITMAP(ur_bitmap->bitmap, x86_64_user_regs_struct, ip);
> -       SET_REG_BITMAP(ur_bitmap->bitmap, x86_64_user_regs_struct, sp);
> +       if (pcp) {
> +               *pcp = x86_64_get_pc(bt);
> +               ur_bitmap->ur.ip = *pcp;
> +               SET_REG_BITMAP(ur_bitmap->bitmap, x86_64_user_regs_struct,
> ip);
> +       }
> +       if (spp) {
> +               *spp = x86_64_get_sp(bt);
> +               ur_bitmap->ur.sp = sp + *spp;
> +               SET_REG_BITMAP(ur_bitmap->bitmap, x86_64_user_regs_struct,
> sp);
> +       }
>
>         bt->machdep = ur_bitmap;
>         bt->need_free = TRUE;
> --
> 2.47.0
>
--
Crash-utility mailing list -- devel@lists.crash-utility.osci.io
To unsubscribe send an email to devel-le...@lists.crash-utility.osci.io
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki

Reply via email to