[xdp-hints] Re: Accessing XDP packet memory from the end

[Alexander Lobakin <alexandr.lobakin@intel.com>]

From: Jesper Dangaard Brouer <jbrouer@redhat.com>
Date: Thu, 21 Apr 2022 18:27:47 +0200

> On 21/04/2022 17.56, Larysa Zaremba wrote:
> > Dear all,
> > Our team has encountered a need of accessing data_meta in a following way:
> > 
> > int xdp_meta_prog(struct xdp_md *ctx)
> > {
> > 	void *data_meta_ptr = (void *)(long)ctx->data_meta;
> > 	void *data_end = (void *)(long)ctx->data_end;
> > 	void *data = (void *)(long)ctx->data;
> > 	u64 data_size = sizeof(u32);
> > 	u32 magic_meta;
> > 	u8 offset;
> > 
> > 	offset = (u8)((s64)data - (s64)data_meta_ptr);
> 
> I'm not sure the verifier can handle this 'offset' calc. As it cannot
> statically know the sized based on this statement. Maybe this is not the
> issue.
> 
> > 	if (offset < data_size) {
> > 		bpf_printk("invalid offset: %ld\n", offset);
> > 		return XDP_DROP;
> > 	}
> > 
> > 	data_meta_ptr += offset;
> > 	data_meta_ptr -= data_size;
> > 
> > 	if (data_meta_ptr + data_size > data) {
> > 		return XDP_DROP;
> > 	}
> > 		
> > 	magic_meta = *((u32 *)data);
> > 	bpf_printk("Magic: %d\n", magic_meta);
> > 	return XDP_PASS;
> > }
> > 
> > Unfortunately, verifier claims this code attempts to access packet with
> > an offset of -2 (a constant part) and negative offset is generally forbidden.
> 
> Are you forgetting to mention:
>   - Have you modified the NIC driver to adjust data_meta pointer and 
> provide info in this area?

Exactly. Previously, @data_meta == @data prior to running BPF
program in 100% cases. Now, the driver can provide arbitrary
metadata and set @data_meta to be @data - 32, data - 48 or so.

> 
> p.s. this is exactly what I'm also working towards[1], so I'll be happy
> to collaborate.  I'm missing the driver code, as link[1] is focused on
> decoding BTF data_meta area in userspace for AF_XDP.

Yeah, we're almost about to post a first RFC to LKML. This issue is
the last one, the rest just needs to be rebased to fix some minors
and polish the code.
It will contain the kernel core part and the driver part (only ice
for now). Then we could e.g. fuse it with your changes (we weren't
touching AF_XDP part) etc.
But for now, until an RFC is posted, you could take a look at the
code in my GH[0] if you're wish :) The second half of the ice code
is not committed yet tho.

> 
> [1] 
> https://github.com/xdp-project/bpf-examples/tree/master/AF_XDP-interaction
> 
> > For now we have 2 solutions, one is using bpf_xdp_adjust_meta(),
> > which is pretty good, but not ideal for the hot path.
> > The second one is the patch at the end.
> > 
> 
> Are you saying, verifier cannot handle that driver changed data_meta 
> pointer and provided info there (without calling bpf_xdp_adjust_meta)?

Correct. I suspect the verifier just assumes that @data_meta always
equals @data when executing BPF prog.
Let's assume:

	offset = data - data_meta; // 64 bytes
	data_meta += offset; // equals to data now
	/* Let's say xdp_meta_generic is 48 bytes long, then */
	data_meta -= sizeof(struct xdp_meta_generic);
	/* data_meta is now 16 bytes past the original data_meta,
	 * or data - 48.
	 */
	bpf_printk("magic: 0x%04x\n",
		   ((struct xdp_meta_generic)data_meta)->magic);

So in fact, this code is absolutely correct, it doesn't go past the
bounds in either direction, but the verifier claims it goes out of
bounds to the left by 48 bytes (not counting the offsetof).
OTOH,

	data_meta = (void *)ctx->data_meta;
	bpf_printk("magic: 0x%04x\n",
		   ((struct xdp_meta_generic)data_meta)->magic);

works with no issues. The verifier still thinks @data_meta == @data,
but this code effectively accesses the metadata, not the frame
itself.

> 
> 
> > Do you see any other way of accessing memory from the end of data_meta/data?
> > What do you think about both suggested solutions?
> > 
> > Best regards,
> > Larysa Zaremba
> > 
> > ---
> > 
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -3576,8 +3576,11 @@ static int check_packet_access(struct bpf_verifier_env *env, u32 regno, int off,
> >   	}
> >   
> >   	err = reg->range < 0 ? -EINVAL :
> > -	      __check_mem_access(env, regno, off, size, reg->range,
> > -				 zero_size_allowed);
> > +	      __check_mem_access(env, regno, off + reg->smin_value, size,
> > +				 reg->range + reg->smin_value, zero_size_allowed);
> > +	err = err ? :
> > +	      __check_mem_access(env, regno, off + reg->umax_value, size,
> > +				 reg->range + reg->umax_value, zero_size_allowed);
> >   	if (err) {
> >   		verbose(env, "R%d offset is outside of the packet\n", regno);
> >   		return err;

[0] https://github.com/alobakin/linux/commits/xdp_hints

Thanks,
Al