1. Patchew
  2. linux
  3. View series

[PATCH bpf] bpf: fix reading neigh ha in bpf_fib_lookup()

Nikhil posted 1 patch 2 weeks, 2 days ago 
net/core/filter.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
[PATCH bpf] bpf: fix reading neigh ha in bpf_fib_lookup()
Posted by Nikhil 2 weeks, 2 days ago 
bpf_ipv4_fib_lookup() and bpf_ipv6_fib_lookup() copy the neighbour's
link layer address into params->dmac without any synchronisation, but
neigh_update() writes neigh->ha under write_seqlock(&neigh->ha_lock)
exactly because there are lockless readers.  A BPF program calling
bpf_fib_lookup() while the neighbour is being updated can therefore be
handed a torn address, and XDP/tc then forwards the packet to a bogus
L2 destination.

neigh_ha_snapshot() cannot be used here because it copies dev->addr_len
bytes while params->dmac is only ETH_ALEN long (an IPoIB egress device
has addr_len 20 and would overflow into params->smac), so open-code the
seqlock loop around the ETH_ALEN copy.

Same problem and same fix as commit 57549ab90791 ("net: bridge: arp/nd
proxy: fix reading neigh ha") and commit b824059a673b ("vxlan: fix
reading neigh ha").

Reproduced on x86_64 under qemu: a dummy device holds a permanent
neighbour whose lladdr is flipped between aa:aa:aa:aa:aa:aa and
bb:bb:bb:bb:bb:bb with RTM_NEWNEIGH, while an XDP program driven by
BPF_PROG_TEST_RUN calls bpf_fib_lookup() in a loop and checks that all
six bytes of the returned dmac are equal.  Before this patch: 211 torn
addresses out of 16300000 lookups (last one aa:aa:aa:aa:bb:bb).  After
this patch: 0 out of 38060000 lookups.

Fixes: 87f5fc7e48dd ("bpf: Provide helper to do forwarding lookups in kernel FIB table")
Cc: stable@vger.kernel.org
Signed-off-by: Nikhil <nikhilljatt@gmail.com>
---
 net/core/filter.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 61940e753552..6fdb85c9af44 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -6298,6 +6298,20 @@ static const struct bpf_func_proto bpf_skb_get_xfrm_state_proto = {
 #endif
 
 #if IS_ENABLED(CONFIG_INET) || IS_ENABLED(CONFIG_IPV6)
+/* Take a stable snapshot of the neighbour's link layer address.
+ * neigh_ha_snapshot() can not be used here because it copies dev->addr_len
+ * bytes while params->dmac is only ETH_ALEN long.
+ */
+static void bpf_fib_dmac_snapshot(u8 *dmac, const struct neighbour *neigh)
+{
+	unsigned int seq;
+
+	do {
+		seq = read_seqbegin(&neigh->ha_lock);
+		memcpy(dmac, neigh->ha, ETH_ALEN);
+	} while (read_seqretry(&neigh->ha_lock, seq));
+}
+
 static int bpf_fib_set_fwd_params(struct net_device *dev,
 				  struct bpf_fib_lookup *params,
 				  u32 flags, u32 mtu, u32 in_ifindex)
@@ -6491,7 +6505,7 @@ static int bpf_ipv4_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
 
 	if (!neigh || !(READ_ONCE(neigh->nud_state) & NUD_VALID))
 		return BPF_FIB_LKUP_RET_NO_NEIGH;
-	memcpy(params->dmac, neigh->ha, ETH_ALEN);
+	bpf_fib_dmac_snapshot(params->dmac, neigh);
 	memcpy(params->smac, dev->dev_addr, ETH_ALEN);
 
 set_fwd_params:
@@ -6644,7 +6658,7 @@ static int bpf_ipv6_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
 	neigh = __ipv6_neigh_lookup_noref(dev, dst);
 	if (!neigh || !(READ_ONCE(neigh->nud_state) & NUD_VALID))
 		return BPF_FIB_LKUP_RET_NO_NEIGH;
-	memcpy(params->dmac, neigh->ha, ETH_ALEN);
+	bpf_fib_dmac_snapshot(params->dmac, neigh);
 	memcpy(params->smac, dev->dev_addr, ETH_ALEN);
 
 set_fwd_params:
-- 
2.43.0
Re: [PATCH bpf] bpf: fix reading neigh ha in bpf_fib_lookup()
Posted by Emil Tsalapatis 2 weeks, 2 days ago 
On Tue, Sep 8, 2026 at 10:40 PM Nikhil <nikhilljatt@gmail.com> wrote:
>
> bpf_ipv4_fib_lookup() and bpf_ipv6_fib_lookup() copy the neighbour's
> link layer address into params->dmac without any synchronisation, but
> neigh_update() writes neigh->ha under write_seqlock(&neigh->ha_lock)
> exactly because there are lockless readers.  A BPF program calling
> bpf_fib_lookup() while the neighbour is being updated can therefore be
> handed a torn address, and XDP/tc then forwards the packet to a bogus
> L2 destination.
>
> neigh_ha_snapshot() cannot be used here because it copies dev->addr_len
> bytes while params->dmac is only ETH_ALEN long (an IPoIB egress device
> has addr_len 20 and would overflow into params->smac), so open-code the
> seqlock loop around the ETH_ALEN copy.
>
> Same problem and same fix as commit 57549ab90791 ("net: bridge: arp/nd
> proxy: fix reading neigh ha") and commit b824059a673b ("vxlan: fix
> reading neigh ha").
>
> Reproduced on x86_64 under qemu: a dummy device holds a permanent
> neighbour whose lladdr is flipped between aa:aa:aa:aa:aa:aa and
> bb:bb:bb:bb:bb:bb with RTM_NEWNEIGH, while an XDP program driven by
> BPF_PROG_TEST_RUN calls bpf_fib_lookup() in a loop and checks that all
> six bytes of the returned dmac are equal.  Before this patch: 211 torn
> addresses out of 16300000 lookups (last one aa:aa:aa:aa:bb:bb).  After
> this patch: 0 out of 38060000 lookups.
>
> Fixes: 87f5fc7e48dd ("bpf: Provide helper to do forwarding lookups in kernel FIB table")
> Cc: stable@vger.kernel.org
> Signed-off-by: Nikhil <nikhilljatt@gmail.com>
> ---

Bot is right wrt possible lockups, please adjust the seqlock
accordingly. Also please
add your full name in the SOB.

pw-bot: cr

>  net/core/filter.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 61940e753552..6fdb85c9af44 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -6298,6 +6298,20 @@ static const struct bpf_func_proto bpf_skb_get_xfrm_state_proto = {
>  #endif
>
>  #if IS_ENABLED(CONFIG_INET) || IS_ENABLED(CONFIG_IPV6)
> +/* Take a stable snapshot of the neighbour's link layer address.
> + * neigh_ha_snapshot() can not be used here because it copies dev->addr_len
> + * bytes while params->dmac is only ETH_ALEN long.
> + */
> +static void bpf_fib_dmac_snapshot(u8 *dmac, const struct neighbour *neigh)
> +{
> +       unsigned int seq;
> +
> +       do {
> +               seq = read_seqbegin(&neigh->ha_lock);
> +               memcpy(dmac, neigh->ha, ETH_ALEN);
> +       } while (read_seqretry(&neigh->ha_lock, seq));
> +}
> +
>  static int bpf_fib_set_fwd_params(struct net_device *dev,
>                                   struct bpf_fib_lookup *params,
>                                   u32 flags, u32 mtu, u32 in_ifindex)
> @@ -6491,7 +6505,7 @@ static int bpf_ipv4_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
>
>         if (!neigh || !(READ_ONCE(neigh->nud_state) & NUD_VALID))
>                 return BPF_FIB_LKUP_RET_NO_NEIGH;
> -       memcpy(params->dmac, neigh->ha, ETH_ALEN);
> +       bpf_fib_dmac_snapshot(params->dmac, neigh);
>         memcpy(params->smac, dev->dev_addr, ETH_ALEN);
>
>  set_fwd_params:
> @@ -6644,7 +6658,7 @@ static int bpf_ipv6_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
>         neigh = __ipv6_neigh_lookup_noref(dev, dst);
>         if (!neigh || !(READ_ONCE(neigh->nud_state) & NUD_VALID))
>                 return BPF_FIB_LKUP_RET_NO_NEIGH;
> -       memcpy(params->dmac, neigh->ha, ETH_ALEN);
> +       bpf_fib_dmac_snapshot(params->dmac, neigh);
>         memcpy(params->smac, dev->dev_addr, ETH_ALEN);
>
>  set_fwd_params:
> --
> 2.43.0
>
>
Re: [PATCH bpf] bpf: fix reading neigh ha in bpf_fib_lookup()
Posted by bot+bpf-ci@kernel.org 2 weeks, 2 days ago 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 8513167a858a..a7c7bbfd90e3 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c

[ ... ]

> @@ -6297,6 +6297,20 @@ static const struct bpf_func_proto bpf_skb_get_xfrm_state_proto = {
>  #endif
>
>  #if IS_ENABLED(CONFIG_INET) || IS_ENABLED(CONFIG_IPV6)
> +/* Take a stable snapshot of the neighbour's link layer address.
> + * neigh_ha_snapshot() can not be used here because it copies dev->addr_len
> + * bytes while params->dmac is only ETH_ALEN long.
> + */
> +static void bpf_fib_dmac_snapshot(u8 *dmac, const struct neighbour *neigh)
> +{
> +	unsigned int seq;
> +
> +	do {
> +		seq = read_seqbegin(&neigh->ha_lock);
> +		memcpy(dmac, neigh->ha, ETH_ALEN);
> +	} while (read_seqretry(&neigh->ha_lock, seq));
> +}
> +

The commit message states:

>     neigh_ha_snapshot() cannot be used here because it copies dev->addr_len
>     bytes while params->dmac is only ETH_ALEN long (an IPoIB egress device
>     has addr_len 20 and would overflow into params->smac), so open-code the
>     seqlock loop around the ETH_ALEN copy.

Is the overflow destination accurate? Looking at struct bpf_fib_lookup in
include/uapi/linux/bpf.h, smac precedes dmac in the final union:

    union {
        struct { __u32 mark; };
        struct {
            __u8 smac[6];    /* ETH_ALEN */
            __u8 dmac[6];    /* ETH_ALEN */
        };
    };

That union is the last member of the struct, so smac sits at offset 52 and
dmac at offset 58 in the 64-byte struct.

Calling neigh_ha_snapshot(params->dmac, neigh, dev) with an IPoIB device
(addr_len == 20) would write bytes 58..77, which is 6 bytes into dmac and
14 bytes past the end of struct bpf_fib_lookup. It cannot reach params->smac
at the lower offset.

Because params points at BPF-program memory that the verifier only sized to
sizeof(struct bpf_fib_lookup), the actual consequence is an out-of-bounds
write past the program-supplied buffer, not a clobbered sibling field.

The conclusion that neigh_ha_snapshot() is unusable here is correct, and
the in-code comment is accurate. Could the changelog's claim about where the
overflow lands be corrected before this goes to stable, since the
parenthetical is the sole stated justification for open-coding the loop?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/34305501194

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.