On Wed, Dec 20, 2023 at 1:40 PM Maxim Mikityanskiy maxtram95@gmail.com wrote:
From: Maxim Mikityanskiy maxim@isovalent.com
Currently, when a scalar bounded register is spilled to the stack, its ID is preserved, but only if was already assigned, i.e. if this register was MOVed before.
Assign an ID on spill if none is set, so that equal scalars could be tracked if a register is spilled to the stack and filled into another register.
One test is adjusted to reflect the change in register IDs.
Signed-off-by: Maxim Mikityanskiy maxim@isovalent.com
kernel/bpf/verifier.c | 8 +++++++- .../selftests/bpf/progs/verifier_direct_packet_access.c | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index b757fdbbbdd2..caa768f1e369 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -4503,9 +4503,15 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
mark_stack_slot_scratched(env, spi); if (reg && !(off % BPF_REG_SIZE) && register_is_bounded(reg) && env->bpf_capable) {
bool reg_value_fits;reg_value_fits = get_reg_width(reg) <= BITS_PER_BYTE * size;/* Make sure that reg had an ID to build a relation on spill. */if (reg_value_fits)assign_scalar_id_before_mov(env, reg);
Thanks. I just debugged this issue as part of my bpf_cmp series.
llvm generated:
1093: (7b) *(u64 *)(r10 -96) = r0 ; R0_w=scalar(smin=smin32=-4095,smax=smax32=256) R10=fp0 fp-96_w=scalar(smin=smin32=-4095,smax=smax32=256) ; if (bpf_cmp(filepart_length, >, MAX_PATH)) 1094: (25) if r0 > 0x100 goto pc+903 ; R0_w=scalar(id=53,smin=smin32=0,smax=umax=smax32=umax32=256,var_off=(0x0; 0x1ff))
the verifier refined the range of 'r0' here, but the code just read spilled value from stack:
1116: (79) r1 = *(u64 *)(r10 -64) ; R1_w=map_value ; payload += filepart_length; 1117: (79) r2 = *(u64 *)(r10 -96) ; R2_w=scalar(smin=smin32=-4095,smax=smax32=256) R10=fp0 fp-96=scalar(smin=smin32=-4095,smax=smax32=256) 1118: (0f) r1 += r2 ; R1_w=map_value(map=data_heap,ks=4,vs=23040,off=148,smin=smin32=-4095,smax=smax32=3344)
And later errors as: "R1 min value is negative, either use unsigned index or do a if (index
=0) check."
This verifier improvement is certainly necessary.
Since you've analyzed this issue did you figure out a workaround for C code on existing and older kernels?