On Thu, Oct 31, 2024 at 10:58:27AM +0900, Masami Hiramatsu wrote:

On Wed, 30 Oct 2024 09:14:48 -0700
Nathan Chancellor [off-list ref] wrote:

quoted

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?

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

Cheers,
Nathan