On Wed, 2025-08-06 at 16:04 -0700, Eduard Zingerman wrote:
> On Wed, 2025-08-06 at 13:09 -0700, Eduard Zingerman wrote:
>
> [...]
>
> > @@ -20712,22 +20711,19 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env,
> > * (cnt == 1) is taken or not. There is no guarantee INSN at OFF is the
> > * original insn at old prog.
> > */
> > - old_data[off].zext_dst = insn_has_def32(insn + off + cnt - 1);
> > + data[off].zext_dst = insn_has_def32(insn + off + cnt - 1);
> >
> > if (cnt == 1)
> > return;
> > prog_len = new_prog->len;
> >
> > - memcpy(new_data, old_data, sizeof(struct bpf_insn_aux_data) * off);
> > - memcpy(new_data + off + cnt - 1, old_data + off,
> > - sizeof(struct bpf_insn_aux_data) * (prog_len - off - cnt + 1));
> > + memmove(data + off + cnt - 1, data + off,
> > + sizeof(struct bpf_insn_aux_data) * (prog_len - off - cnt + 1));
> > for (i = off; i < off + cnt - 1; i++) {
> > /* Expand insni[off]'s seen count to the patched range. */
> > - new_data[i].seen = old_seen;
> > - new_data[i].zext_dst = insn_has_def32(insn + i);
> > + data[i].seen = old_seen;
> > + data[i].zext_dst = insn_has_def32(insn + i);
> > }
> > - env->insn_aux_data = new_data;
> > - vfree(old_data);
> > }
>
> veristat-meta job failed on the CI [1] because the following piece is missing:
>
> @@ -20719,6 +20719,7 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env,
>
> memmove(data + off + cnt - 1, data + off,
> sizeof(struct bpf_insn_aux_data) * (prog_len - off - cnt + 1));
> + memset(data + off, 0, sizeof(struct bpf_insn_aux_data) * (cnt - 1));
> for (i = off; i < off + cnt - 1; i++) {
> /* Expand insni[off]'s seen count to the patched range. */
> data[i].seen = old_seen;
>
> I'm trying to figure out if I can add a selftest for this.
>
> [1] https://github.com/kernel-patches/bpf/actions/runs/16787563163/job/47542309875
>
> [...]
The error reported by verifier is "verifier bug: error during ctx access conversion (0)",
signaled from convert_ctx_accesses(). The rewrite is attempted because
`env->insn_aux_data[i + delta].ptr_type` is set to 12 (PTR_TO_SOCK_COMMON).
The instruction for which rewrite is attempted is a load or store
instruction introduced as a result of inline_bpf_loop() call.
It has a wrong offset for bpf_sock_convert_ctx_access() rewrite,
hence rewrite attempt is unsuccessful and the above mentioned error is reported.
`env->insn_aux_data[i + delta].ptr_type` is set for the instruction in question
because of missing memset(0). It is a value of the insn_aux_data inherited
from an instruction occurring at a small offset after bpf_loop call.
Here is a similar reproducer, but for PTR_TO_CTX (== 2):
struct { ... } map0 SEC(".maps"); // any valid map definition
struct { ... } map1 SEC(".maps");
struct { ... } map2 SEC(".maps");
SEC("xdp")
__success
__naked void bug1(void)
{
asm volatile (" \
r0 = %[map0] ll; /* 0 */ \
r0 = %[map1] ll; /* 2 */ \
r1 = 1; /* 4 */ \
r2 = dummy ll; /* 5 */ \
r3 = 0; /* 7 */ \
r4 = 0; /* 8 */ \
call %[bpf_loop]; /* 9 */ \
r0 = 0; /* 10 */ \
r0 = 0; /* 11 */ \
r0 = %[map2] ll; /* 12 */ \
exit; \
" :
: __imm(bpf_loop),
__imm_addr(map0),
__imm_addr(map1),
__imm_addr(map2)
: __clobber_all);
}
Instruction `call %[bpf_loop]` is replaced by a sequence:
9: if r1 <= 0x800000 goto pc+2
10: w0 = -7
11: goto pc+16
12: *(u64 *)(r10 -24) = r6
...
Note the store at offset (12). Because of the missing memset(0) it
inherits insn_aux_data fields from original instruction #12: `r0 = %[map2] ll`.
`struct bpf_insn_aux_data` is defined as follows:
struct bpf_insn_aux_data {
union {
enum bpf_reg_type ptr_type;
...
struct {
u32 map_index; /* index into used_maps[] */
...
};
...
};
}
Here fields .ptr_type and .map_index have same offset.
The example above forces convert_ctx_accesses() to interpret .map_index as a .ptr_type.
The .map_index at offset 12 happens to be 2, which corresponds to PTR_TO_CTX.
convert_ctx_accesses() attempts to rewrite `12: *(u64 *)(r10 -24) = r6` and fails.
All in all, I think this test is fragile, so I'll post v2 w/o a selftest.
