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

[Nikhil <nikhilljatt@gmail.com>]

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