linux-kernel - Re: [PATCH] ptrace: Fix error handling in ptrace_hbp_get_initialised

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAN5X4L_p-h+8mb7dvhJEuT04GjfXKt6MMGUU2ah9=ZnQCVomqQ@mail.gmail.com>
Date: Thu, 13 Feb 2025 23:12:51 +0800
From: Junlong li <zhuizhuhaomeng@...il.com>
To: Will Deacon <will@...nel.org>
Cc: oleg@...hat.com, catalin.marinas@....com, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ptrace: Fix error handling in ptrace_hbp_get_initialised_bp

Thank you for your detailed explanation.
I misunderstood the meaning of the return value of the function
ptrace_hbp_get_event.


On Thu, Feb 13, 2025 at 8:17 PM Will Deacon <will@...nel.org> wrote:
>
> On Wed, Feb 12, 2025 at 07:35:46PM +0800, Junlong li wrote:
> >    From b824aece318ed38666621610af7807e70831f964 Mon Sep 17 00:00:00 2001
> >    From: lijunlong <[1]lijunlong@...nresty.com>
> >    Date: Wed, 12 Feb 2025 19:15:46 +0800
> >    Subject: [PATCH] ptrace: Fix error handling in
> >    ptrace_hbp_get_initialised_bp
> >
> >    The function ptrace_hbp_get_event() returns ERR_PTR(-EINVAL) on error,
> >    but ptrace_hbp_get_initialised_bp() was checking for NULL instead of
> >    using IS_ERR(). This could lead to incorrect error handling and
> >    potential issues when trying to create a new breakpoint event.
>
> Can you please give an example of how this goes wrong?
>
> >    Change the condition from:
> >        if (!bp)
> >    to:
> >        if (IS_ERR(bp))
> >
> >    This ensures proper error checking and maintains consistency with
> >    the error handling mechanism used by ptrace_hbp_get_event().
> >
> >    Signed-off-by: lijunlong [2]zhuizhuhaomeng@...il.com
> >    ---
> >     arch/arm64/kernel/ptrace.c | 2 +-
> >     1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >    diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> >    index 0d022599eb61..3bf549b540b1 100644
> >    --- a/arch/arm64/kernel/ptrace.c
> >    +++ b/arch/arm64/kernel/ptrace.c
> >    @@ -414,7 +414,7 @@ static struct perf_event
> >    *ptrace_hbp_get_initialised_bp(unsigned int note_type,
> >     {
> >            struct perf_event *bp = ptrace_hbp_get_event(note_type, tsk, idx);
> >
> >    -       if (!bp)
> >    +       if (IS_ERR(bp))
> >                    bp = ptrace_hbp_create(note_type, tsk, idx);
>
> I think this change actually causes a problem.
>
> In the current code, ptrace_hbp_get_event() can return:
>
>  - An error if the note type is unknown or the index is out-of-bounds
>  - NULL if the relevant breakpoint has not yet been created
>  - The breakpoint pointer if it exists
>
> So, in the case of getting NULL back, we lazily create the breakpoint.
>
> Is it pretty? Hell no! But I'm not entirely sure it's broken, either.
>
> Will



-- 
-