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

[bot+bpf-ci]

> 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