net: bridge: guard local finish against missing port
Subsystems (3)
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/1 net: bridge: guard local finish against missing port
From: Nan Li <[email protected]>
The bridge local receive path may be deferred by netfilter and resumed later. By the time br_handle_local_finish() runs, skb->dev may still be valid while its bridge port association has already been removed.
br_handle_local_finish() unconditionally looks up the bridge port from skb->dev and dereferences it for source learning. If the port is no longer attached to the bridge, the lookup returns NULL and the deferred local receive path can no longer rely on the port state being present.
Skip the learning step when the bridge port lookup fails. In that case there is no port state left to learn on, so returning early preserves the normal behavior for existing ports while avoiding access to stale state.
--- net/bridge/br_input.c | 3 +++ 1 file changed, 3 insertions(+)
net/bridge/br_input.c +3 -0
@@ -247,6 +247,9 @@ static void __br_handle_local_finish(struct sk_buff *skb)struct net_bridge_port *p =br_port_get_rcu (skb->dev);u16 vid =0 ; +if (unlikely (!p)) +return ; +/* check if vlan is allowed, to avoid spoofing */ if ((p->flags & BR_LEARNING) && nbp_state_should_learn(p) &&
Signature · 1 line
2.43.0
Nikolay Aleksandrov
maintainerQuoted 11 lines · Ren Wei
From: Nan Li <[email protected]>
The bridge local receive path may be deferred by netfilter and resumed later. By the time
br_handle_local_finish()runs,skb->devmay still be valid while its bridge port association has already been removed.
br_handle_local_finish()unconditionally looks up the bridge port fromskb->devand dereferences it for source learning. If the port is no longer attached to the bridge, the lookup returns NULL and the deferred local receive path can no longer rely on the port state being present.
Quoted 6 lines
Skip the learning step when the bridge port lookup fails. In that case there is no port state left to learn on, so returning early preserves the normal behavior for existing ports while avoiding access to stale state.
Fixes 8626c56c8279 ("bridge: fix potential use-after-free when hook returns QUEUE or STOLEN verdict")
I don't think that is the correct commit, it seems to me this bug has existed for a very long time. From a quick search I think (Florian please correct me if I'm wrong, but I think NF_QUEUE existed back then) it was introduced in 2010 by:
f350a0a87374 ("bridge: use rx_handler_data pointer to store net_bridge_port
pointer")
because that commit removed the same check for a NULL port.
The patch itself is ok, it restores the check that was there before the commit I mentioned.
Quoted 25 lines
Reported-by Yuan Tan <[email protected]>Reported-by Yifan Wu <[email protected]>Reported-by Juefei Pu <[email protected]>Reported-by Xin Liu <[email protected]>Signed-off-by Nan Li <[email protected]>Signed-off-by Ren Wei <[email protected]>--- net/bridge/br_input.c | 3 +++ 1 file changed, 3 insertions(+)net/bridge/br_input.c +3
-0@@ -247,6 +247,9 @@ static void __br_handle_local_finish(struct sk_buff *skb)struct net_bridge_port *p =br_port_get_rcu (skb->dev);u16 vid =0 ; +if (unlikely (!p)) +return ; +/* check if vlan is allowed, to avoid spoofing */ if ((p->flags & BR_LEARNING) && nbp_state_should_learn(p) &&
Quoted 18 lines · Nikolay Aleksandrov
Quoted 16 lines · Ren Wei
From: Nan Li <[email protected]>
The bridge local receive path may be deferred by netfilter and resumed later. By the time
br_handle_local_finish()runs,skb->devmay still be valid while its bridge port association has already been removed.
br_handle_local_finish()unconditionally looks up the bridge port fromskb->devand dereferences it for source learning. If the port is no longer attached to the bridge, the lookup returns NULL and the deferred local receive path can no longer rely on the port state being present.Skip the learning step when the bridge port lookup fails. In that case there is no port state left to learn on, so returning early preserves the normal behavior for existing ports while avoiding access to stale state.
Quoted 8 lines
Quoted 1 line
Fixes 8626c56c8279 ("bridge: fix potential use-after-free when hook returns QUEUE or STOLEN verdict")I don't think that is the correct commit, it seems to me this bug has existed for a very long time. From a quick search I think (Florian please correct me if I'm wrong, but I think
NF_QUEUEexisted back then) it was introduced in 2010 by:
f350a0a87374("bridge: userx_handler_datapointer to storenet_bridge_port
pointer")
After checking the history, I believe f350a0a87374 is indeed the commit that introduced the underlying root cause.
The 8626c56c8279 commit in the patch is the one that actually made the bug reachable in practice. I am a bit unsure which commit should be used in the Fixes: tag. We have run into this situation several times already, where the commit that introduced the root cause is different from the commit that actually made the bug triggerable/reachable.
What do you think would be best?
Also, if the Fixes: tag should be changed, would you prefer that we resend the patch, or would you rather have the committer adjust the
Fixes: tag when applying it in order to reduce traffic on netdev?
Quoted 32 lines
because that commit removed the same check for a NULL port.
The patch itself is ok, it restores the check that was there before the commit I mentioned.Quoted 25 lines
Reported-by Yuan Tan <[email protected]>Reported-by Yifan Wu <[email protected]>Reported-by Juefei Pu <[email protected]>Reported-by Xin Liu <[email protected]>Signed-off-by Nan Li <[email protected]>Signed-off-by Ren Wei <[email protected]>--- net/bridge/br_input.c | 3 +++ 1 file changed, 3 insertions(+)net/bridge/br_input.c +3
-0@@ -247,6 +247,9 @@ static void __br_handle_local_finish(struct sk_buff *skb)struct net_bridge_port *p =br_port_get_rcu (skb->dev);u16 vid =0 ; +if (unlikely (!p)) +return ; +/* check if vlan is allowed, to avoid spoofing */ if ((p->flags & BR_LEARNING) && nbp_state_should_learn(p) &&
Nikolay Aleksandrov
maintainerQuoted 29 lines · Yuan Tan
Quoted 18 lines · Nikolay Aleksandrov
Quoted 16 lines · Ren Wei
From: Nan Li <[email protected]>
The bridge local receive path may be deferred by netfilter and resumed later. By the time
br_handle_local_finish()runs,skb->devmay still be valid while its bridge port association has already been removed.
br_handle_local_finish()unconditionally looks up the bridge port fromskb->devand dereferences it for source learning. If the port is no longer attached to the bridge, the lookup returns NULL and the deferred local receive path can no longer rely on the port state being present.Skip the learning step when the bridge port lookup fails. In that case there is no port state left to learn on, so returning early preserves the normal behavior for existing ports while avoiding access to stale state.
Quoted 8 lines
Quoted 1 line
Fixes 8626c56c8279 ("bridge: fix potential use-after-free when hook returns QUEUE or STOLEN verdict")I don't think that is the correct commit, it seems to me this bug has existed for a very long time. From a quick search I think (Florian please correct me if I'm wrong, but I think
NF_QUEUEexisted back then) it was introduced in 2010 by:
f350a0a87374("bridge: userx_handler_datapointer to storenet_bridge_port
pointer")
Quoted 9 lines
After checking the history, I believe
f350a0a87374is indeed the commit that introduced the underlying root cause.The
8626c56c8279commit in the patch is the one that actually made the bug reachable in practice. I am a bit unsure which commit should be used in the Fixes: tag. We have run into this situation several times already, where the commit that introduced the root cause is different from the commit that actually made the bug triggerable/reachable.
Hmm could you please elaborate? How did that commit make it reachable? I can see the call was done before it as well:
/* Deliver packet to local host only */
- if (NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_IN,
- dev_net(skb->dev), NULL, skb, skb->dev, NULL,
- br_handle_local_finish)) {
- return RX_HANDLER_CONSUMED; /* consumed by filter */
- } else {
- *pskb = skb;
- return RX_HANDLER_PASS; /* continue processing */
- }
+ NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_IN, dev_net(skb->dev),
+ NULL, skb, skb->dev, NULL, br_handle_local_finish);
+ return RX_HANDLER_CONSUMED;
NF_HOOK() would return 1 for NF_QUEUEed packet so essentially it was doing the same, how would did it make the bug triggerable?
Quoted 7 lines
What do you think would be best?
Also, if the Fixes: tag should be changed, would you prefer that we resend the patch, or would you rather have the committer adjust the
Fixes: tag when applying it in order to reduce traffic on netdev?
You should update the Fixes: tag but also wait 24h before re-posting another patch version.
Quoted 32 lines
Quoted 32 lines
because that commit removed the same check for a NULL port.
The patch itself is ok, it restores the check that was there before the commit I mentioned.Quoted 25 lines
Reported-by Yuan Tan <[email protected]>Reported-by Yifan Wu <[email protected]>Reported-by Juefei Pu <[email protected]>Reported-by Xin Liu <[email protected]>Signed-off-by Nan Li <[email protected]>Signed-off-by Ren Wei <[email protected]>--- net/bridge/br_input.c | 3 +++ 1 file changed, 3 insertions(+)net/bridge/br_input.c +3
-0@@ -247,6 +247,9 @@ static void __br_handle_local_finish(struct sk_buff *skb)struct net_bridge_port *p =br_port_get_rcu (skb->dev);u16 vid =0 ; +if (unlikely (!p)) +return ; +/* check if vlan is allowed, to avoid spoofing */ if ((p->flags & BR_LEARNING) && nbp_state_should_learn(p) &&
Quoted 56 lines · Nikolay Aleksandrov
Quoted 29 lines · Yuan Tan
Quoted 18 lines · Nikolay Aleksandrov
Quoted 16 lines · Ren Wei
From: Nan Li <[email protected]>
The bridge local receive path may be deferred by netfilter and resumed later. By the time
br_handle_local_finish()runs,skb->devmay still be valid while its bridge port association has already been removed.
br_handle_local_finish()unconditionally looks up the bridge port fromskb->devand dereferences it for source learning. If the port is no longer attached to the bridge, the lookup returns NULL and the deferred local receive path can no longer rely on the port state being present.Skip the learning step when the bridge port lookup fails. In that case there is no port state left to learn on, so returning early preserves the normal behavior for existing ports while avoiding access to stale state.
Quoted 8 lines
Quoted 1 line
Fixes 8626c56c8279 ("bridge: fix potential use-after-free when hook returns QUEUE or STOLEN verdict")I don't think that is the correct commit, it seems to me this bug has existed for a very long time. From a quick search I think (Florian please correct me if I'm wrong, but I think
NF_QUEUEexisted back then) it was introduced in 2010 by:
f350a0a87374("bridge: userx_handler_datapointer to storenet_bridge_port
pointer")Quoted 9 lines
After checking the history, I believe
f350a0a87374is indeed the commit that introduced the underlying root cause.The
8626c56c8279commit in the patch is the one that actually made the bug reachable in practice. I am a bit unsure which commit should be used in the Fixes: tag. We have run into this situation several times already, where the commit that introduced the root cause is different from the commit that actually made the bug triggerable/reachable.Hmm could you please elaborate? How did that commit make it reachable? I can see the call was done before it as well:
/* Deliver packet to local host only */(quoted diff) +3
-8- if (NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_IN, - dev_net(skb->dev), NULL, skb, skb->dev, NULL, - br_handle_local_finish)) { - return RX_HANDLER_CONSUMED; /* consumed by filter */ - } else { - *pskb = skb; - return RX_HANDLER_PASS; /* continue processing */ - } + NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_IN, dev_net(skb->dev), + NULL, skb, skb->dev, NULL, br_handle_local_finish); + return RX_HANDLER_CONSUMED;
Quoted 2 lines
NF_HOOK()would return 1 forNF_QUEUEedpacket so essentially it was doing the same, how would did it make the bug triggerable?
I see. I was wrong. Thanks for pointing that out.
Quoted 11 lines
Quoted 8 lines
What do you think would be best?
Also, if the Fixes: tag should be changed, would you prefer that we resend the patch, or would you rather have the committer adjust the
Fixes: tag when applying it in order to reduce traffic on netdev?You should update the Fixes: tag but also wait 24h before re-posting another patch version.
Ok we will send the v2 with fix tag f350a0a87374 after 24h
Quoted 34 lines
Quoted 33 lines
Quoted 33 lines
because that commit removed the same check for a NULL port.
The patch itself is ok, it restores the check that was there before the commit I mentioned.Quoted 25 lines
Reported-by Yuan Tan <[email protected]>Reported-by Yifan Wu <[email protected]>Reported-by Juefei Pu <[email protected]>Reported-by Xin Liu <[email protected]>Signed-off-by Nan Li <[email protected]>Signed-off-by Ren Wei <[email protected]>--- net/bridge/br_input.c | 3 +++ 1 file changed, 3 insertions(+)net/bridge/br_input.c +3
-0@@ -247,6 +247,9 @@ static void __br_handle_local_finish(struct sk_buff *skb)struct net_bridge_port *p =br_port_get_rcu (skb->dev);u16 vid =0 ; +if (unlikely (!p)) +return ; +/* check if vlan is allowed, to avoid spoofing */ if ((p->flags & BR_LEARNING) && nbp_state_should_learn(p) &&
Quoted 4 lines · Yuan Tan
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
The bridge local receive path may be deferred by netfilter and resumed later. By the time
br_handle_local_finish()runs,skb->devmay still be valid while its bridge port association has already been removed.
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
br_handle_local_finish()unconditionally looks up the bridge port fromskb->devand dereferences it for source learning. If the port is no longer attached to the bridge, the lookup returns NULL and the deferred local receive path can no longer rely on the port state being present.You should update the Fixes: tag but also wait 24h before re-posting another patch version.
Ok we will send the v2 with fix tag
f350a0a87374after 24h
Would you mind exploring an alternative fix for this?
As nfnetlink_queue'd skbs leave rcu read locked section, great care has to be taken on reinject.
Either bridge could call nf_queue_nf_hook_drop() on bridge port removal, or nfqnl_reinject() could revalidate that skb->dev is part of a bridge and munge verdict to NF_DROP in case the assocication was removed while packet was out.
static bool nfqnl_bridge_port_removed(const struct nf_queue_entry *e)
{
return e->state.pf == NFPROTO_BRIDGE &&
!br_port_get_rcu(e->skb->dev) == NULL;
}static void nfqnl_reinject(struct nf_queue_entry *entry, unsigned int verdict)
{
const struct nf_ct_hook *ct_hook;
if (verdict == NF_ACCEPT ||
verdict == NF_REPEAT ||
verdict == NF_STOP) {
unsigned int ct_verdict = verdict;
....
if (nfqnl_bridge_port_removed(entry))
verdict = NF_DROP;Its more work, but it places the extra checks to where they are really needed.
Also see related bug fix: At this time, entire bridge device can go away while packet is out.