linux-kernel - Re: [PATCH 1/2] kprobes: Fix __get_insn_slot() after __counted

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: <20241101105306.d198e543f9b60270cdcd570c@kernel.org>
Date: Fri, 1 Nov 2024 10:53:06 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Nathan Chancellor <nathan@...nel.org>
Cc: Naveen N Rao <naveen@...nel.org>, Anil S Keshavamurthy
 <anil.s.keshavamurthy@...el.com>, "David S. Miller" <davem@...emloft.net>,
 Kees Cook <kees@...nel.org>, "Gustavo A. R. Silva" <gustavoars@...nel.org>,
 Jinjie Ruan <ruanjinjie@...wei.com>, linux-kernel@...r.kernel.org,
 linux-trace-kernel@...r.kernel.org, linux-hardening@...r.kernel.org,
 patches@...ts.linux.dev
Subject: Re: [PATCH 1/2] kprobes: Fix __get_insn_slot() after __counted_by
 annotation

On Wed, 30 Oct 2024 20:37:31 -0700
Nathan Chancellor <nathan@...nel.org> wrote:

> On Thu, Oct 31, 2024 at 10:58:27AM +0900, Masami Hiramatsu wrote:
> > On Wed, 30 Oct 2024 09:14:48 -0700
> > Nathan Chancellor <nathan@...nel.org> wrote:
> > 
> > > Commit 0888460c9050 ("kprobes: Annotate structs with __counted_by()")
> > > added a __counted_by annotation without adjusting the code for the
> > > __counted_by requirements, resulting in a panic when UBSAN_BOUNDS and
> > > FORTIFY_SOURCE are enabled:
> > > 
> > >   | memset: detected buffer overflow: 512 byte write of buffer size 0
> > >   | WARNING: CPU: 0 PID: 1 at lib/string_helpers.c:1032 __fortify_report+0x64/0x80
> > >   | Call Trace:
> > >   |  __fortify_report+0x60/0x80 (unreliable)
> > >   |  __fortify_panic+0x18/0x1c
> > >   |  __get_insn_slot+0x33c/0x340
> > > 
> > > __counted_by requires that the counter be set before accessing the
> > > flexible array but ->nused is not set until after ->slot_used is
> > > accessed via memset(). Even if the current ->nused assignment were moved
> > > up before memset(), the value of 1 would be incorrect because the entire
> > > array is being accessed, not just one element.
> > 
> > Ah, I think I misunderstood the __counted_by(). If so, ->nused can be
> > smaller than the accessing element of slot_used[]. I should revert it.
> > The accessing index and ->nused should have no relationship.
> > 
> > for example, slots_per_page(c) is 10, and 10 kprobes are registered
> > and then, the 1st and 2nd kprobes are unregistered. At this moment,
> > ->nused is 8 but slot_used[9] is still used. To unregister this 10th
> > kprobe, we have to access slot_used[9].
> 
> Ah, I totally missed that bit of the code, sorry about that. Thanks for
> the explanation!
> 
> > So let's just revert the commit 0888460c9050.
> 
> Reverting that change sounds totally reasonable to me based on the
> above. Will you take care of that?

Yeah, probes/for-next is a working branch. So I just dropped it.

> 
> For what it's worth, I think patch #2 should still be applicable, if you
> are okay with that one.

Yes, other patches look good to me.

Thank you,

> 
> Cheers,
> Nathan


-- 
Masami Hiramatsu (Google) <mhiramat@...nel.org>