On Wed, 30 Oct 2024 20:37:31 -0700 Nathan Chancellor <[email protected]> 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 <[email protected]> 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) <[email protected]>
