Page MenuHomeFreeBSD

if_bridge: fix potential panic
ClosedPublic

Authored by kp on May 18 2023, 6:38 PM.
Tags
None
Referenced Files
F102016734: D40147.diff
Wed, Nov 6, 2:28 PM
Unknown Object (File)
Mon, Oct 21, 10:11 AM
Unknown Object (File)
Sat, Oct 19, 7:40 AM
Unknown Object (File)
Oct 4 2024, 10:27 PM
Unknown Object (File)
Sep 26 2024, 6:07 AM
Unknown Object (File)
Sep 22 2024, 5:42 AM
Unknown Object (File)
Sep 19 2024, 8:36 AM
Unknown Object (File)
Sep 18 2024, 6:04 AM

Details

Summary

When a new bridge_rtnode is added it is added with a NULL brt_dst. The
brt_dst is set after the entry is added. This means there's a small
window where another core could also attempt to add this node, leading
to the code attempting to log that the MAC addresses moved to a new
interface.
Aside from that being a spurious log entry it also panics, because
obif is NULL (and we attempt to dereference it).

Avoid this by settings brt_dst before we insert the bridge_rtnode.
Assert that obif is non-NULL, as an extra precaution.

Reported by: olivier@

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kp requested review of this revision.May 18 2023, 6:38 PM

Generally looks good to me.

sys/net/if_bridge.c
2944

I think the increasing bif_addrcnt should be kept after bridge_rtnode_insert() succeed .

This revision is now accepted and ready to land.May 19 2023, 7:23 AM
sys/net/if_bridge.c
2945

A second thought, as an alternate we can place a decreasing if bridge_rtnode_insert() fails.

bif->bif_addrcnt--;

Both should be OK, as write operations on bif_addrcnt is protected by BRIDGE_RT_LOCK. Only one read operation may have a small inconsistency window.

static int
bridge_ioctl_gifflags(struct bridge_softc *sc, void *arg)
{
...
    req->ifbr_addrcnt = bif->bif_addrcnt;
...
}
sys/net/if_bridge.c
2944

That is a very good point. I think I like having the increment after the insert more than reducing it again, so I'll do that.

This revision was automatically updated to reflect the committed changes.