| 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: <e7b992e3-8059-4058-8561-cb017c200c8d@app.fastmail.com>
Date: Thu, 05 Oct 2023 22:16:53 +0200
From: Martynas <m@...bda.lt>
To: "Martin KaFai Lau" <martin.lau@...ux.dev>
Cc: "Daniel Borkmann" <daniel@...earbox.net>, netdev <netdev@...r.kernel.org>,
"Nikolay Aleksandrov" <razor@...ckwall.org>, bpf@...r.kernel.org
Subject: Re: [PATCH bpf v2 1/2] bpf: Derive source IP addr via bpf_*_fib_lookup()
Thanks for the review.
On Wed, Oct 4, 2023, at 10:09 PM, Martin KaFai Lau wrote:
> On 10/3/23 12:10 AM, Martynas Pumputis wrote:
>> Extend the bpf_fib_lookup() helper by making it to return the source
>> IPv4/IPv6 address if the BPF_FIB_LOOKUP_SET_SRC flag is set.
>>
>> For example, the following snippet can be used to derive the desired
>> source IP address:
>>
>> struct bpf_fib_lookup p = { .ipv4_dst = ip4->daddr };
>>
>> ret = bpf_skb_fib_lookup(skb, p, sizeof(p),
>> BPF_FIB_LOOKUP_SET_SRC | BPF_FIB_LOOKUP_SKIP_NEIGH);
>> if (ret != BPF_FIB_LKUP_RET_SUCCESS)
>> return TC_ACT_SHOT;
>>
>> /* the p.ipv4_src now contains the source address */
>>
>> The inability to derive the proper source address may cause malfunctions
>> in BPF-based dataplanes for hosts containing netdevs with more than one
>> routable IP address or for multi-homed hosts.
>>
>> For example, Cilium implements packet masquerading in BPF. If an
>> egressing netdev to which the Cilium's BPF prog is attached has
>> multiple IP addresses, then only one [hardcoded] IP address can be used for
>> masquerading. This breaks connectivity if any other IP address should have
>> been selected instead, for example, when a public and private addresses
>> are attached to the same egress interface.
>>
>> The change was tested with Cilium [1].
>>
>> Nikolay Aleksandrov helped to figure out the IPv6 addr selection.
>>
>> [1]: https://github.com/cilium/cilium/pull/28283
>>
>> Signed-off-by: Martynas Pumputis <m@...bda.lt>
>> ---
>> include/net/ipv6_stubs.h | 5 +++++
>> include/uapi/linux/bpf.h | 9 +++++++++
>> net/core/filter.c | 19 ++++++++++++++++++-
>> net/ipv6/af_inet6.c | 1 +
>> tools/include/uapi/linux/bpf.h | 10 ++++++++++
>> 5 files changed, 43 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/net/ipv6_stubs.h b/include/net/ipv6_stubs.h
>> index c48186bf4737..21da31e1dff5 100644
>> --- a/include/net/ipv6_stubs.h
>> +++ b/include/net/ipv6_stubs.h
>> @@ -85,6 +85,11 @@ struct ipv6_bpf_stub {
>> sockptr_t optval, unsigned int optlen);
>> int (*ipv6_getsockopt)(struct sock *sk, int level, int optname,
>> sockptr_t optval, sockptr_t optlen);
>> + int (*ipv6_dev_get_saddr)(struct net *net,
>> + const struct net_device *dst_dev,
>> + const struct in6_addr *daddr,
>> + unsigned int prefs,
>> + struct in6_addr *saddr);
>> };
>> extern const struct ipv6_bpf_stub *ipv6_bpf_stub __read_mostly;
>>
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 0448700890f7..a6bf686eecbc 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -3257,6 +3257,10 @@ union bpf_attr {
>> * and *params*->smac will not be set as output. A common
>> * use case is to call **bpf_redirect_neigh**\ () after
>> * doing **bpf_fib_lookup**\ ().
>> + * **BPF_FIB_LOOKUP_SET_SRC**
>> + * Derive and set source IP addr in *params*->ipv{4,6}_src
>> + * for the nexthop. If the src addr cannot be derived,
>> + * **BPF_FIB_LKUP_RET_NO_SRC_ADDR** is returned.
>> *
>> * *ctx* is either **struct xdp_md** for XDP programs or
>> * **struct sk_buff** tc cls_act programs.
>> @@ -6953,6 +6957,7 @@ enum {
>> BPF_FIB_LOOKUP_OUTPUT = (1U << 1),
>> BPF_FIB_LOOKUP_SKIP_NEIGH = (1U << 2),
>> BPF_FIB_LOOKUP_TBID = (1U << 3),
>> + BPF_FIB_LOOKUP_SET_SRC = (1U << 4),
>
> bikeshedding: Shorten it to BPF_FIB_LOOKUP_SRC?
SGTM.
>
>> };
>>
>> enum {
>> @@ -6965,6 +6970,7 @@ enum {
>> BPF_FIB_LKUP_RET_UNSUPP_LWT, /* fwd requires encapsulation */
>> BPF_FIB_LKUP_RET_NO_NEIGH, /* no neighbor entry for nh */
>> BPF_FIB_LKUP_RET_FRAG_NEEDED, /* fragmentation required to fwd */
>> + BPF_FIB_LKUP_RET_NO_SRC_ADDR, /* failed to derive IP src addr */
>> };
>>
>> struct bpf_fib_lookup {
>> @@ -6999,6 +7005,9 @@ struct bpf_fib_lookup {
>> __u32 rt_metric;
>> };
>>
>> + /* input: source address to consider for lookup
>> + * output: source address result from lookup
>> + */
>> union {
>> __be32 ipv4_src;
>> __u32 ipv6_src[4]; /* in6_addr; network order */
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index a094694899c9..f3777ef1840b 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -5850,6 +5850,9 @@ static int bpf_ipv4_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
>> params->rt_metric = res.fi->fib_priority;
>> params->ifindex = dev->ifindex;
>>
>> + if (flags & BPF_FIB_LOOKUP_SET_SRC)
>> + params->ipv4_src = fib_result_prefsrc(net, &res);
>
> Does it need to check 0 and return BPF_FIB_LKUP_RET_NO_SRC_ADDR for v4 also,
> or the fib_result_prefsrc does not fail?
>From inspecting the other usages of the function - it does not fail.
>
>> +
>> /* xdp and cls_bpf programs are run in RCU-bh so
>> * rcu_read_lock_bh is not needed here
>> */
>> @@ -5992,6 +5995,19 @@ static int bpf_ipv6_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
>> params->rt_metric = res.f6i->fib6_metric;
>> params->ifindex = dev->ifindex;
>>
>> + if (flags & BPF_FIB_LOOKUP_SET_SRC) {
>> + if (res.f6i->fib6_prefsrc.plen) {
>> + *(struct in6_addr *)params->ipv6_src = res.f6i->fib6_prefsrc.addr;
>> + } else {
>> + err = ipv6_bpf_stub->ipv6_dev_get_saddr(net, dev,
>> + &fl6.daddr, 0,
>> + (struct in6_addr *)
>> + params->ipv6_src);
>> + if (err)
>> + return BPF_FIB_LKUP_RET_NO_SRC_ADDR;
>
> This error also implies BPF_FIB_LKUP_RET_NO_NEIGH. I don't have a clean way of
> improving the API. May be others have some ideas.
>
> Considering dev has no saddr is probably (?) an unlikely case, it should be ok
> to leave it as is but at least a comment in the uapi will be needed. Otherwise,
> the bpf prog may use the 0 dmac as-is.
I expect that a user of the helper checks that err == 0 before using any of the output params.
>
> I feel the current bpf_ipv[46]_fib_lookup helper is doing many things
> in one
> function and then requires different BPF_FIB_LOOKUP_* bits to select
> what/how to
> do. In the future, it may be worth to consider breaking it into smaller
> kfunc(s). e.g. the __ipv[46]_neigh_lookup could be in its own kfunc.
>
Yep, good idea. At least it seems that the neigh lookup could live in its own function.
>> + }
>> + }
>> +
>> if (flags & BPF_FIB_LOOKUP_SKIP_NEIGH)
>> goto set_fwd_params;
>>
>> @@ -6010,7 +6026,8 @@ static int bpf_ipv6_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
>> #endif
>>
>> #define BPF_FIB_LOOKUP_MASK (BPF_FIB_LOOKUP_DIRECT | BPF_FIB_LOOKUP_OUTPUT | \
>> - BPF_FIB_LOOKUP_SKIP_NEIGH | BPF_FIB_LOOKUP_TBID)
>> + BPF_FIB_LOOKUP_SKIP_NEIGH | BPF_FIB_LOOKUP_TBID | \
>> + BPF_FIB_LOOKUP_SET_SRC)
>>
>> BPF_CALL_4(bpf_xdp_fib_lookup, struct xdp_buff *, ctx,
>> struct bpf_fib_lookup *, params, int, plen, u32, flags)
Powered by blists - more mailing lists