RE: [PATCH] bpf/arm64: support packet data load instructions

["Morten Brørup" <mb@smartsharesystems.com>]

> > > In Classic BPF, negative "k" has special meaning for both BPF_ABS
> and BPF_IND.
> > > So we should consider it invalid for both cases.
> > >
> > > That prevents applications from using it the way you describe.
> > > And it will allow us to add BPF library support for Linux-
> compatible special meanings later, without
> > breaking the ABI.
> > >
> >
> > Aren't these invalid offsets already taken care during the syntax
> > check when we validate the BPF program ?
> > in bpf_validate.c +1499:
> >         /* load absolute instructions */
> >         [(BPF_LD | BPF_ABS | BPF_B)] = {
> >                 .mask = {. dreg = ZERO_REG, .sreg = ZERO_REG},
> >                 .off = { .min = 0, .max = 0},
> >                 .imm = { .min = 0, .max = INT32_MAX},
> >                 .eval = eval_ld_mbuf,
> >         },
> >
> > IIUC, as __rte_bpf_validate fails when we cal rte_bpf_load ( in
> > bpf_load.c +113), we can't even interpret the cBPF program.
> 
> Good point, we can probably consider BPF_ABS case covered by this.

Agree.

> 
> For BPF_IND however it does not seem to exclude any values, and even if
> it did
> we don't know what's in the register. Speaking of which, I just noticed
> that
> we're truncating it.
> 
> I suggest the following logic in pseudo-code:
> 
> static void
> emit_ld_mbuf(struct a64_jit_ctx *ctx, uint32_t op, uint8_t tmp1,
> uint8_t tmp2,
> 	uint8_t src, int32_t imm)
> {
> 	// ...
> 
> 	/* r1 = off: for ABS use imm, for IND use src + imm */
> 	if (mode == BPF_ABS) {
> 		assert imm >= 0, "verified by verifier"
> 		emit MOV W1, #<imm>
> 	} else {
> 		/* add signed imm to the source register */
> 		emit(s) X1 = src + #<imm>
> 		/* verify dynamically that offset is within the domain of
> __rte_pktmbuf_read */
> 		emit(s) jump_to_epilogue if X1 <s 0 || X1 > INT32_MAX
> 	}
> 
> 	// ...
> }

I don't know if it's the sum of src+imm that determines special meaning, or it's the imm itself.
If it's the imm itself, a simple fix would be to update the validator's .imm.max values for BPF_IND from UINT32_MAX to INT32_MAX.