| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87a6cbd0q4.fsf@toke.dk>
Date: Sat, 23 Apr 2022 22:05:39 +0200
From: Toke Høiland-Jørgensen <toke@...hat.com>
To: Alexander Lobakin <alexandr.lobakin@...el.com>
Cc: Larysa Zaremba <larysa.zaremba@...el.com>,
bpf <bpf@...r.kernel.org>, netdev <netdev@...r.kernel.org>,
Andrii Nakryiko <andrii@...nel.org>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Jesper Dangaard Brouer <brouer@...hat.com>,
Magnus Karlsson <magnus.karlsson@...el.com>,
Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
Alexander Lobakin <alexandr.lobakin@...el.com>
Subject: Re: Accessing XDP packet memory from the end
Alexander Lobakin <alexandr.lobakin@...el.com> writes:
> From: Toke Høiland-Jørgensen <toke@...hat.com>
> Date: Thu, 21 Apr 2022 19:17:11 +0200
>
>> Larysa Zaremba <larysa.zaremba@...el.com> writes:
>>
>> > 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);
>> > 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.
>> >
>> > 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.
>> >
>> > Do you see any other way of accessing memory from the end of data_meta/data?
>> > What do you think about both suggested solutions?
>>
>> The problem is that the compiler is generating code that the verifier
>> doesn't understand. It's notoriously hard to get LLVM to produce code
>> that preserves the right bounds checks which is why projects like Cilium
>> use helpers with inline ASM to produce the right loads, like in [0].
>>
>> Adapting that cilium helper to load from the metadata area, your example
>> can be rewritten as follows (which works just fine with no verifier
>> changes):
>>
>> static __always_inline int
>> xdp_load_meta_bytes(const struct xdp_md *ctx, __u64 off, void *to, const __u64 len)
>> {
>> void *from;
>> int ret;
>> /* LLVM tends to generate code that verifier doesn't understand,
>> * so force it the way we want it in order to open up a range
>> * on the reg.
>> */
>> asm volatile("r1 = *(u32 *)(%[ctx] +8)\n\t"
>> "r2 = *(u32 *)(%[ctx] +0)\n\t"
>> "%[off] &= %[offmax]\n\t"
>> "r1 += %[off]\n\t"
>> "%[from] = r1\n\t"
>> "r1 += %[len]\n\t"
>> "if r1 > r2 goto +2\n\t"
>> "%[ret] = 0\n\t"
>> "goto +1\n\t"
>> "%[ret] = %[errno]\n\t"
>> : [ret]"=r"(ret), [from]"=r"(from)
>> : [ctx]"r"(ctx), [off]"r"(off), [len]"ri"(len),
>> [offmax]"i"(__CTX_OFF_MAX), [errno]"i"(-EINVAL)
>> : "r1", "r2");
>> if (!ret)
>> __builtin_memcpy(to, from, len);
>> return ret;
>> }
>>
>>
>> SEC("xdp")
>> int xdp_meta_prog(struct xdp_md *ctx)
>> {
>> void *data_meta_ptr = (void *)(long)ctx->data_meta;
>> void *data = (void *)(long)ctx->data;
>> __u32 magic_meta;
>> __u8 offset;
>> int ret;
>>
>> offset = (__u8)((__s64)data - (__s64)data_meta_ptr);
>> ret = xdp_load_meta_bytes(ctx, offset - 4, &magic_meta, sizeof(magic_meta));
>> if (ret) {
>> bpf_printk("load bytes failed: %d\n", ret);
>> return XDP_DROP;
>> }
>>
>> bpf_printk("Magic: %d\n", magic_meta);
>> return XDP_PASS;
>> }
>
> At the moment, we use this (based on Cilium's and your), it works
> just like we want C code to work previously:
>
> #define __CTX_OFF_MAX 0xff
>
> static __always_inline void *
> can_i_access_meta_please(const struct xdp_md *ctx, __u64 off, const __u64 len)
> {
> void *ret;
>
> /* LLVM tends to generate code that verifier doesn't understand,
> * so force it the way we want it in order to open up a range
> * on the reg.
> */
> asm volatile("r1 = *(u32 *)(%[ctx] +8)\n\t"
> "r2 = *(u32 *)(%[ctx] +0)\n\t"
> "%[off] &= %[offmax]\n\t"
> "r1 += %[off]\n\t"
> "%[ret] = r1\n\t"
> "r1 += %[len]\n\t"
> "if r1 > r2 goto +1\n\t"
> "goto +1\n\t"
> "%[ret] = %[null]\n\t"
> : [ret]"=r"(ret)
> : [ctx]"r"(ctx), [off]"r"(off), [len]"ri"(len),
> [offmax]"i"(__CTX_OFF_MAX), [null]"i"(NULL)
> : "r1", "r2");
>
> return ret;
> }
>
> SEC("xdp")
> int xdp_prognum_n0_meta(struct xdp_md *ctx)
> {
> void *data_meta = (void *)(__s64)ctx->data_meta;
> void *data = (void *)(__s64)ctx->data;
> struct xdp_meta_generic *md;
> __u64 offset;
>
> offset = (__u64)((__s64)data - (__s64)data_meta);
>
> md = can_i_access_meta_please(ctx, offset, sizeof(*md));
> if (__builtin_expect(!md, 0)) {
> bpf_printk("No you can't\n");
> return XDP_DROP;
> }
>
> bpf_printk("Magic: 0x%04x\n", md->magic_id);
> return XDP_PASS;
> }
>
> Thanks for the help!
Great! You're welcome! :)
> It's a shame LLVM still suck on generating correct object code from C.
> I guess we'll define a helper above in one of the headers to not
> copy-paste it back and forth between each program wanting to access
> only the generic part of the metadata (which is always being placed at
> the end).
Yeah, it would be nice if LLVM could just generate code that works, but
in the meantime we'll just have to define a helper. I suspect we'll need
to define some helper functions to work with xdp-hints style metadata
field anyway, so wrapping the reader into that somewhere would probably
make sense, no?
-Toke
Powered by blists - more mailing lists