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-next>] [day] [month] [year] [list]

Message-ID: <20250213121738.GA11596@willie-the-truck>
Date: Thu, 13 Feb 2025 12:17:40 +0000
From: Will Deacon <will@...nel.org>
To: Junlong li <zhuizhuhaomeng@...il.com>
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

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