On Tue, May 12, 2026 at 2:03 AM Nikolay Aleksandrov <[email protected]> wrote: > > On 12/05/2026 11:57, Yuan Tan wrote: > > On Tue, May 12, 2026 at 1:29 AM Nikolay Aleksandrov <[email protected]> > > wrote: > >> > >> On 12/05/2026 07:31, Ren Wei wrote: > >>> 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. > >>> > >>> 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") > > > > > > 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. > > > > 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?
I see. I was wrong. Thanks for pointing that out. > > > 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 > > >> > >> 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. > >> > >>> Cc: [email protected] > >>> 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(+) > >>> > >>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c > >>> index 2cbae0f9ae1f..5b0d7450de5f 100644 > >>> --- a/net/bridge/br_input.c > >>> +++ b/net/bridge/br_input.c > >>> @@ -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) && > >> > >> >
