Skip to content

Commit 8626c56

Browse files
Florian Westphaldavem330
authored andcommitted
bridge: fix potential use-after-free when hook returns QUEUE or STOLEN verdict
Zefir Kurtisi reported kernel panic with an openwrt specific patch. However, it turns out that mainline has a similar bug waiting to happen. Once NF_HOOK() returns the skb is in undefined state and must not be used. Moreover, the okfn must consume the skb to support async processing (NF_QUEUE). Current okfn in this spot doesn't consume it and caller assumes that NF_HOOK return value tells us if skb was freed or not, but thats wrong. It "works" because no in-tree user registers a NFPROTO_BRIDGE hook at LOCAL_IN that returns STOLEN or NF_QUEUE verdicts. Once we add NF_QUEUE support for nftables bridge this will break -- NF_QUEUE holds the skb for async processing, caller will erronoulsy return RX_HANDLER_PASS and on reinject netfilter will access free'd skb. Fix this by pushing skb up the stack in the okfn instead. NB: It also seems dubious to use LOCAL_IN while bypassing PRE_ROUTING completely in this case but this is how its been forever so it seems preferable to not change this. Cc: Felix Fietkau <nbd@openwrt.org> Cc: Zefir Kurtisi <zefir.kurtisi@neratec.com> Signed-off-by: Florian Westphal <fw@strlen.de> Tested-by: Zefir Kurtisi <zefir.kurtisi@neratec.com> Signed-off-by: David S. Miller <davem@davemloft.net>
1 parent 180a2c5 commit 8626c56

File tree

1 file changed

+7
-9
lines changed

1 file changed

+7
-9
lines changed

net/bridge/br_input.c

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -222,7 +222,10 @@ static int br_handle_local_finish(struct net *net, struct sock *sk, struct sk_bu
222222
/* check if vlan is allowed, to avoid spoofing */
223223
if (p->flags & BR_LEARNING && br_should_learn(p, skb, &vid))
224224
br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid, false);
225-
return 0; /* process further */
225+
226+
BR_INPUT_SKB_CB(skb)->brdev = p->br->dev;
227+
br_pass_frame_up(skb);
228+
return 0;
226229
}
227230

228231
/*
@@ -284,14 +287,9 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
284287
}
285288

286289
/* Deliver packet to local host only */
287-
if (NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_IN,
288-
dev_net(skb->dev), NULL, skb, skb->dev, NULL,
289-
br_handle_local_finish)) {
290-
return RX_HANDLER_CONSUMED; /* consumed by filter */
291-
} else {
292-
*pskb = skb;
293-
return RX_HANDLER_PASS; /* continue processing */
294-
}
290+
NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_IN, dev_net(skb->dev),
291+
NULL, skb, skb->dev, NULL, br_handle_local_finish);
292+
return RX_HANDLER_CONSUMED;
295293
}
296294

297295
forward:

0 commit comments

Comments
 (0)