netdev - Re: [PATCH v10 bpf-next 9/9] selftests/bpf: tests for using dynptrs to parse skb and xdp buffers

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJnrk1YA+-+ZEB3JL=pZS-_NUz+Zvwim9eFFXzoojsqp5ZfZZg@mail.gmail.com>
Date:   Fri, 17 Feb 2023 10:14:02 -0800
From:   Joanne Koong <joannelkoong@...il.com>
To:     Toke Høiland-Jørgensen <toke@...nel.org>
Cc:     bpf@...r.kernel.org, martin.lau@...nel.org, andrii@...nel.org,
        memxor@...il.com, ast@...nel.org, daniel@...earbox.net,
        netdev@...r.kernel.org, kernel-team@...com
Subject: Re: [PATCH v10 bpf-next 9/9] selftests/bpf: tests for using dynptrs
 to parse skb and xdp buffers

On Fri, Feb 17, 2023 at 5:05 AM Toke Høiland-Jørgensen <toke@...nel.org> wrote:
>
> Joanne Koong <joannelkoong@...il.com> writes:
>
> > Test skb and xdp dynptr functionality in the following ways:
>
> One question on one of the usage examples:
>
> [...]
>
> > diff --git a/tools/testing/selftests/bpf/progs/test_xdp_dynptr.c b/tools/testing/selftests/bpf/progs/test_xdp_dynptr.c
> > new file mode 100644
> > index 000000000000..4c49fd42da6f
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/test_xdp_dynptr.c
> > @@ -0,0 +1,257 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2022 Meta */
> > +#include <stddef.h>
> > +#include <string.h>
> > +#include <linux/bpf.h>
> > +#include <linux/if_ether.h>
> > +#include <linux/if_packet.h>
> > +#include <linux/ip.h>
> > +#include <linux/ipv6.h>
> > +#include <linux/in.h>
> > +#include <linux/udp.h>
> > +#include <linux/tcp.h>
> > +#include <linux/pkt_cls.h>
> > +#include <sys/socket.h>
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_endian.h>
> > +#include "test_iptunnel_common.h"
> > +
> > +extern int bpf_dynptr_from_xdp(struct xdp_md *xdp, __u64 flags,
> > +                            struct bpf_dynptr *ptr) __ksym;
> > +extern void *bpf_dynptr_slice(const struct bpf_dynptr *ptr, __u32 offset,
> > +                           void *buffer, __u32 buffer__sz) __ksym;
> > +extern void *bpf_dynptr_slice_rdwr(const struct bpf_dynptr *ptr, __u32 offset,
> > +                           void *buffer, __u32 buffer__sz) __ksym;
> > +
> > +const size_t tcphdr_sz = sizeof(struct tcphdr);
> > +const size_t udphdr_sz = sizeof(struct udphdr);
> > +const size_t ethhdr_sz = sizeof(struct ethhdr);
> > +const size_t iphdr_sz = sizeof(struct iphdr);
> > +const size_t ipv6hdr_sz = sizeof(struct ipv6hdr);
> > +
> > +struct {
> > +     __uint(type, BPF_MAP_TYPE_PERCPU_ARRAY);
> > +     __uint(max_entries, 256);
> > +     __type(key, __u32);
> > +     __type(value, __u64);
> > +} rxcnt SEC(".maps");
> > +
> > +struct {
> > +     __uint(type, BPF_MAP_TYPE_HASH);
> > +     __uint(max_entries, MAX_IPTNL_ENTRIES);
> > +     __type(key, struct vip);
> > +     __type(value, struct iptnl_info);
> > +} vip2tnl SEC(".maps");
> > +
> > +static __always_inline void count_tx(__u32 protocol)
> > +{
> > +     __u64 *rxcnt_count;
> > +
> > +     rxcnt_count = bpf_map_lookup_elem(&rxcnt, &protocol);
> > +     if (rxcnt_count)
> > +             *rxcnt_count += 1;
> > +}
> > +
> > +static __always_inline int get_dport(void *trans_data, __u8 protocol)
> > +{
> > +     struct tcphdr *th;
> > +     struct udphdr *uh;
> > +
> > +     switch (protocol) {
> > +     case IPPROTO_TCP:
> > +             th = (struct tcphdr *)trans_data;
> > +             return th->dest;
> > +     case IPPROTO_UDP:
> > +             uh = (struct udphdr *)trans_data;
> > +             return uh->dest;
> > +     default:
> > +             return 0;
> > +     }
> > +}
> > +
> > +static __always_inline void set_ethhdr(struct ethhdr *new_eth,
> > +                                    const struct ethhdr *old_eth,
> > +                                    const struct iptnl_info *tnl,
> > +                                    __be16 h_proto)
> > +{
> > +     memcpy(new_eth->h_source, old_eth->h_dest, sizeof(new_eth->h_source));
> > +     memcpy(new_eth->h_dest, tnl->dmac, sizeof(new_eth->h_dest));
> > +     new_eth->h_proto = h_proto;
> > +}
> > +
> > +static __always_inline int handle_ipv4(struct xdp_md *xdp, struct bpf_dynptr *xdp_ptr)
> > +{
> > +     __u8 eth_buffer[ethhdr_sz + iphdr_sz + ethhdr_sz];
> > +     __u8 iph_buffer_tcp[iphdr_sz + tcphdr_sz];
> > +     __u8 iph_buffer_udp[iphdr_sz + udphdr_sz];
> > +     struct bpf_dynptr new_xdp_ptr;
> > +     struct iptnl_info *tnl;
> > +     struct ethhdr *new_eth;
> > +     struct ethhdr *old_eth;
> > +     __u32 transport_hdr_sz;
> > +     struct iphdr *iph;
> > +     __u16 *next_iph;
> > +     __u16 payload_len;
> > +     struct vip vip = {};
> > +     int dport;
> > +     __u32 csum = 0;
> > +     int i;
> > +
> > +     __builtin_memset(eth_buffer, 0, sizeof(eth_buffer));
> > +     __builtin_memset(iph_buffer_tcp, 0, sizeof(iph_buffer_tcp));
> > +     __builtin_memset(iph_buffer_udp, 0, sizeof(iph_buffer_udp));
> > +
> > +     if (ethhdr_sz + iphdr_sz + tcphdr_sz > xdp->data_end - xdp->data)
> > +             iph = bpf_dynptr_slice(xdp_ptr, ethhdr_sz, iph_buffer_udp, sizeof(iph_buffer_udp));
> > +     else
> > +             iph = bpf_dynptr_slice(xdp_ptr, ethhdr_sz, iph_buffer_tcp, sizeof(iph_buffer_tcp));
> > +
> > +     if (!iph)
> > +             return XDP_DROP;
> > +
> > +     dport = get_dport(iph + 1, iph->protocol);
> > +     if (dport == -1)
> > +             return XDP_DROP;
> > +
> > +     vip.protocol = iph->protocol;
> > +     vip.family = AF_INET;
> > +     vip.daddr.v4 = iph->daddr;
> > +     vip.dport = dport;
> > +     payload_len = bpf_ntohs(iph->tot_len);
> > +
> > +     tnl = bpf_map_lookup_elem(&vip2tnl, &vip);
> > +     /* It only does v4-in-v4 */
> > +     if (!tnl || tnl->family != AF_INET)
> > +             return XDP_PASS;
> > +
> > +     if (bpf_xdp_adjust_head(xdp, 0 - (int)iphdr_sz))
> > +             return XDP_DROP;
> > +
> > +     bpf_dynptr_from_xdp(xdp, 0, &new_xdp_ptr);
> > +     new_eth = bpf_dynptr_slice_rdwr(&new_xdp_ptr, 0, eth_buffer, sizeof(eth_buffer));
> > +     if (!new_eth)
> > +             return XDP_DROP;
>
> Here the program just gets a new pointer using bpf_dynptr_slice_rdwr()
> and proceeds to write to it directly without any further checks. But
> what happens if the pointer points to the eth_buffer? Presumably the
> program would need to check this and do a bpf_dynptr_write() at the end
> in this case? Would be good to have this in the example, as this is
> basically the only documentation that exists, and people are probably
> going to copy-paste code from here :)
>
> -Toke
>

Ooh good catch, thanks! i forgot to change this for the other examples
as well; I will update this :)

>
>
> > +     iph = (struct iphdr *)(new_eth + 1);
> > +     old_eth = (struct ethhdr *)(iph + 1);
[...]