| Message ID | 20241210140654.108998-1-jonas.gorski@bisdn.de (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC] net: bridge: handle ports in locked mode for ll learning | expand |
On Tue, Dec 10, 2024 at 03:06:53PM +0100, Jonas Gorski wrote: > > When support for locked ports was added with commit a21d9a670d81 ("net: > bridge: Add support for bridge port in locked mode"), learning is > inhibited when the port is locked in br_handle_frame_finish(). > > It was later extended in commit a35ec8e38cdd ("bridge: Add MAC > Authentication Bypass (MAB) support") where optionally learning is done > with locked entries. > > Unfortunately both missed that learning may also happen on frames to > link local addresses (01:80:c2:00:00:0X) in br_handle_frame(), which > will call __br_handle_local_finish(), which may update the fdb unless > (ll) learning is disabled as well. > > This can be easily observed by e.g. EAPOL frames to 01:80:c2:00:00:03 on > a port causing the source mac to be learned, which is then forwarded > normally, essentially bypassing any authentication. > > Fix this by moving the BR_PORT_LOCKED handling into its own function, > and call it from both places. > > Fixes: a21d9a670d81 ("net: bridge: Add support for bridge port in locked mode") > Fixes: a35ec8e38cdd ("bridge: Add MAC Authentication Bypass (MAB) support") > Signed-off-by: Jonas Gorski <jonas.gorski@bisdn.de> > --- > Sent as RFC since I'm not 100% sure this is the right way to fix. It was decided that this is expected behavior. https://man7.org/linux/man-pages/man8/bridge.8.html locked on or locked off Controls whether a port is locked or not. When locked, non-link-local frames received through the port are dropped unless an FDB entry with the MAC source address points to the port. The common use case is IEEE 802.1X where hosts can authenticate themselves by exchanging EAPOL frames with an authenticator. After authentication is complete, the user space control plane can install a matching FDB entry to allow traffic from the host to be forwarded by the bridge. When learning is enabled on a ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ locked port, the no_linklocal_learn bridge option needs to ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ be on to prevent the bridge from learning from received ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ EAPOL frames. By default this flag is off. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Am Di., 10. Dez. 2024 um 15:34 Uhr schrieb Vladimir Oltean <vladimir.oltean@nxp.com>: > > On Tue, Dec 10, 2024 at 03:06:53PM +0100, Jonas Gorski wrote: > > > > When support for locked ports was added with commit a21d9a670d81 ("net: > > bridge: Add support for bridge port in locked mode"), learning is > > inhibited when the port is locked in br_handle_frame_finish(). > > > > It was later extended in commit a35ec8e38cdd ("bridge: Add MAC > > Authentication Bypass (MAB) support") where optionally learning is done > > with locked entries. > > > > Unfortunately both missed that learning may also happen on frames to > > link local addresses (01:80:c2:00:00:0X) in br_handle_frame(), which > > will call __br_handle_local_finish(), which may update the fdb unless > > (ll) learning is disabled as well. > > > > This can be easily observed by e.g. EAPOL frames to 01:80:c2:00:00:03 on > > a port causing the source mac to be learned, which is then forwarded > > normally, essentially bypassing any authentication. > > > > Fix this by moving the BR_PORT_LOCKED handling into its own function, > > and call it from both places. > > > > Fixes: a21d9a670d81 ("net: bridge: Add support for bridge port in locked mode") > > Fixes: a35ec8e38cdd ("bridge: Add MAC Authentication Bypass (MAB) support") > > Signed-off-by: Jonas Gorski <jonas.gorski@bisdn.de> > > --- > > Sent as RFC since I'm not 100% sure this is the right way to fix. > > It was decided that this is expected behavior. > https://man7.org/linux/man-pages/man8/bridge.8.html > locked on or locked off > Controls whether a port is locked or not. When locked, > non-link-local frames received through the port are > dropped unless an FDB entry with the MAC source address > points to the port. The common use case is IEEE 802.1X > where hosts can authenticate themselves by exchanging > EAPOL frames with an authenticator. After authentication > is complete, the user space control plane can install a > matching FDB entry to allow traffic from the host to be > forwarded by the bridge. When learning is enabled on a > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > locked port, the no_linklocal_learn bridge option needs to > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > be on to prevent the bridge from learning from received > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > EAPOL frames. By default this flag is off. > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Huh, indeed. Unexpected decision, didn't think that this was intentional. I wonder what the use case for that is. Ah well, then disregard my patch. Best Regards, Jonas
On Tue, Dec 10, 2024 at 03:47:11PM +0100, Jonas Gorski wrote: > Huh, indeed. Unexpected decision, didn't think that this was > intentional. I wonder what the use case for that is. > > Ah well, then disregard my patch. The discussion was here, I don't remember the details: https://lore.kernel.org/all/20220630111634.610320-1-hans@kapio-technology.com/
Am Di., 10. Dez. 2024 um 15:55 Uhr schrieb Vladimir Oltean <vladimir.oltean@nxp.com>: > > On Tue, Dec 10, 2024 at 03:47:11PM +0100, Jonas Gorski wrote: > > Huh, indeed. Unexpected decision, didn't think that this was > > intentional. I wonder what the use case for that is. > > > > Ah well, then disregard my patch. > > The discussion was here, I don't remember the details: > https://lore.kernel.org/all/20220630111634.610320-1-hans@kapio-technology.com/ Thanks for the pointer. Reading the discussion, it seems this was before the explicit BR_PORT_MAB option and locked learning support, so there was some ambiguity around whether learning on locked ports is desired or not, and this was needed(?) for the out-of-tree(?) MAB implementation. But now that we do have an explicit flag for MAB, maybe this should be revisited? Especially since with BR_PORT_MAB enabled, entries are supposed to be learned as locked. But link local learned entries are still learned unlocked. So no_linklocal_learn still needs to be enabled for +locked, +learning, +mab. AFACT at least Hans thought that this should be done when there an explicit MAB opt in in https://lore.kernel.org/all/CAKUejP6wCaOKiafvbxYqQs0-RibC0FMKtvkiG=R2Ts0Xfa3-tg@mail.gmail.com/ and https://lore.kernel.org/all/CAKUejP6xR81p1QeSCnDP_3uh9owafdYr1pifeCzekzUvU3_dPw@mail.gmail.com/. Best Regards, Jonas
On Tue, Dec 10, 2024 at 04:28:54PM +0100, Jonas Gorski wrote: > Thanks for the pointer. Reading the discussion, it seems this was > before the explicit BR_PORT_MAB option and locked learning support, so > there was some ambiguity around whether learning on locked ports is > desired or not, and this was needed(?) for the out-of-tree(?) MAB > implementation. There is a use case for learning on a locked port even without MAB. If user space is granting access via dynamic FDB entires, then you need learning enabled to refresh these entries. > But now that we do have an explicit flag for MAB, maybe this should be > revisited? Especially since with BR_PORT_MAB enabled, entries are > supposed to be learned as locked. But link local learned entries are > still learned unlocked. So no_linklocal_learn still needs to be > enabled for +locked, +learning, +mab. I mentioned this in the man page and added "no_linklocal_learn" to iproute2, but looks like it is not enough. You can try reposting the original patch (skip learning from link-local frames on a locked port) with a Fixes tag and see how it goes. I think it is unfortunate to change the behavior when there is already a dedicated knob for what you want to achieve, but I suspect the change will not introduce regressions so maybe people will find it acceptable.
Am Mi., 11. Dez. 2024 um 09:42 Uhr schrieb Ido Schimmel <idosch@nvidia.com>: > > On Tue, Dec 10, 2024 at 04:28:54PM +0100, Jonas Gorski wrote: > > Thanks for the pointer. Reading the discussion, it seems this was > > before the explicit BR_PORT_MAB option and locked learning support, so > > there was some ambiguity around whether learning on locked ports is > > desired or not, and this was needed(?) for the out-of-tree(?) MAB > > implementation. > > There is a use case for learning on a locked port even without MAB. If > user space is granting access via dynamic FDB entires, then you need > learning enabled to refresh these entries. AFAICT this would still work with my patch, as long learning is enabled for the port. The difference would be that new dynamic entries won't be created anymore from link local learning, so userspace would now have to add them themselves. But any existing dynamic entries will be refreshed via the normal input paths. Though I see that this would break offloading these, since USER dynamic entries are ignored in br_switchdev_fdb_notify() since 927cdea5d209 ("net: bridge: switchdev: don't notify FDB entries with "master dynamic""). Side note, br_switchdev_fdb_replay() seems to still pass them on. Do I miss something or shouldn't replay also need to ignore/skip them? > > But now that we do have an explicit flag for MAB, maybe this should be > > revisited? Especially since with BR_PORT_MAB enabled, entries are > > supposed to be learned as locked. But link local learned entries are > > still learned unlocked. So no_linklocal_learn still needs to be > > enabled for +locked, +learning, +mab. > > I mentioned this in the man page and added "no_linklocal_learn" to > iproute2, but looks like it is not enough. You can try reposting the > original patch (skip learning from link-local frames on a locked port) > with a Fixes tag and see how it goes. I think it is unfortunate to > change the behavior when there is already a dedicated knob for what you > want to achieve, but I suspect the change will not introduce regressions > so maybe people will find it acceptable. Absolutely not your fault; my reference was the original cover letters for BR_PORT_LOCKED and BR_PORT_MAB and reading br_input.c where the flags are handled (not even looking at if_link.h's doc comments). And there the constraint/side effect isn't mentioned anywhere, so I assumed it was unintentional. And I never looked at any man pages, just used bridge link help to find out what the arguments are to (un)set those port flags. So I looked everywhere except where this constraint is pointed out. Anyway, I understand your concern about already having a knob to avoid the issue, my concern here is that the knob isn't quite obvious, and that you do need an additional knob to have a "secure" default. So IMHO it's easy to miss as an inexperienced user. Though at least in the !MAB case, disabling learning on the port is also enough to avoid that (and keeps learning via link local enabled for unlocked ports). At least in the case of having enabled BR_PORT_MAB, I would consider it a bug that the entries learned via link local traffic aren't marked as BR_FDB_LOCKED. If you agree, I can send in a reduced patch for that, so that the entries are initially locked regardless the source of learning. Best Regards, Jonas
On Wed, Dec 11, 2024 at 11:32:38AM +0100, Jonas Gorski wrote: > Am Mi., 11. Dez. 2024 um 09:42 Uhr schrieb Ido Schimmel <idosch@nvidia.com>: > > > > On Tue, Dec 10, 2024 at 04:28:54PM +0100, Jonas Gorski wrote: > > > Thanks for the pointer. Reading the discussion, it seems this was > > > before the explicit BR_PORT_MAB option and locked learning support, so > > > there was some ambiguity around whether learning on locked ports is > > > desired or not, and this was needed(?) for the out-of-tree(?) MAB > > > implementation. > > > > There is a use case for learning on a locked port even without MAB. If > > user space is granting access via dynamic FDB entires, then you need > > learning enabled to refresh these entries. > > AFAICT this would still work with my patch, as long learning is > enabled for the port. The difference would be that new dynamic entries > won't be created anymore from link local learning, so userspace would > now have to add them themselves. But any existing dynamic entries will > be refreshed via the normal input paths. > > Though I see that this would break offloading these, since USER > dynamic entries are ignored in br_switchdev_fdb_notify() since > 927cdea5d209 ("net: bridge: switchdev: don't notify FDB entries with > "master dynamic""). Side note, br_switchdev_fdb_replay() seems to > still pass them on. Do I miss something or shouldn't replay also need > to ignore/skip them? > > > > But now that we do have an explicit flag for MAB, maybe this should be > > > revisited? Especially since with BR_PORT_MAB enabled, entries are > > > supposed to be learned as locked. But link local learned entries are > > > still learned unlocked. So no_linklocal_learn still needs to be > > > enabled for +locked, +learning, +mab. > > > > I mentioned this in the man page and added "no_linklocal_learn" to > > iproute2, but looks like it is not enough. You can try reposting the > > original patch (skip learning from link-local frames on a locked port) > > with a Fixes tag and see how it goes. I think it is unfortunate to > > change the behavior when there is already a dedicated knob for what you > > want to achieve, but I suspect the change will not introduce regressions > > so maybe people will find it acceptable. > > Absolutely not your fault; my reference was the original cover letters > for BR_PORT_LOCKED and BR_PORT_MAB and reading br_input.c where the > flags are handled (not even looking at if_link.h's doc comments). And > there the constraint/side effect isn't mentioned anywhere, so I > assumed it was unintentional. And I never looked at any man pages, > just used bridge link help to find out what the arguments are to > (un)set those port flags. So I looked everywhere except where this > constraint is pointed out. > > Anyway, I understand your concern about already having a knob to avoid > the issue, my concern here is that the knob isn't quite obvious, and > that you do need an additional knob to have a "secure" default. So > IMHO it's easy to miss as an inexperienced user. Though at least in > the !MAB case, disabling learning on the port is also enough to avoid > that (and keeps learning via link local enabled for unlocked ports). > > At least in the case of having enabled BR_PORT_MAB, I would consider > it a bug that the entries learned via link local traffic aren't marked > as BR_FDB_LOCKED. If you agree, I can send in a reduced patch for > that, so that the entries are initially locked regardless the source > of learning. I will give a bit of background so that my answer will make more sense. AFAICT, there are three different ways to deploy 802.1X / MAB: 1. 802.1X with static FDB entries. In this case learning can be disabled. 2. 802.1X with dynamic FDB entries. In this case learning needs to be enabled so that entries will be refreshed by incoming traffic. 3. MAB. In this case learning needs to be enabled so that user space will be notified about hosts that are trying to communicate through the bridge. When the original patch was posted I was not aware of the last two use cases that require learning to be enabled. In any scenario where you have +learning +locked (regardless of +/-mab) you need to have +no_linklocal_learn for things to work correctly, so the potential for regressions from the original patch seems low to me. The original patch also provides a more comprehensive solution to the problem than marking entries learned from link local traffic with BR_FDB_LOCKED. It applies regardless of +/-mab (i.e., it covers both cases 2 and 3 and not only 3). That is why I prefer the original patch over the proposed approach.
On 11.12.24 15:50, Ido Schimmel wrote: > On Wed, Dec 11, 2024 at 11:32:38AM +0100, Jonas Gorski wrote: >> Am Mi., 11. Dez. 2024 um 09:42 Uhr schrieb Ido Schimmel <idosch@nvidia.com>: >>> >>> On Tue, Dec 10, 2024 at 04:28:54PM +0100, Jonas Gorski wrote: >>>> Thanks for the pointer. Reading the discussion, it seems this was >>>> before the explicit BR_PORT_MAB option and locked learning support, so >>>> there was some ambiguity around whether learning on locked ports is >>>> desired or not, and this was needed(?) for the out-of-tree(?) MAB >>>> implementation. >>> >>> There is a use case for learning on a locked port even without MAB. If >>> user space is granting access via dynamic FDB entires, then you need >>> learning enabled to refresh these entries. >> >> AFAICT this would still work with my patch, as long learning is >> enabled for the port. The difference would be that new dynamic entries >> won't be created anymore from link local learning, so userspace would >> now have to add them themselves. But any existing dynamic entries will >> be refreshed via the normal input paths. >> >> Though I see that this would break offloading these, since USER >> dynamic entries are ignored in br_switchdev_fdb_notify() since >> 927cdea5d209 ("net: bridge: switchdev: don't notify FDB entries with >> "master dynamic""). Side note, br_switchdev_fdb_replay() seems to >> still pass them on. Do I miss something or shouldn't replay also need >> to ignore/skip them? >> >>>> But now that we do have an explicit flag for MAB, maybe this should be >>>> revisited? Especially since with BR_PORT_MAB enabled, entries are >>>> supposed to be learned as locked. But link local learned entries are >>>> still learned unlocked. So no_linklocal_learn still needs to be >>>> enabled for +locked, +learning, +mab. >>> >>> I mentioned this in the man page and added "no_linklocal_learn" to >>> iproute2, but looks like it is not enough. You can try reposting the >>> original patch (skip learning from link-local frames on a locked port) >>> with a Fixes tag and see how it goes. I think it is unfortunate to >>> change the behavior when there is already a dedicated knob for what you >>> want to achieve, but I suspect the change will not introduce regressions >>> so maybe people will find it acceptable. >> >> Absolutely not your fault; my reference was the original cover letters >> for BR_PORT_LOCKED and BR_PORT_MAB and reading br_input.c where the >> flags are handled (not even looking at if_link.h's doc comments). And >> there the constraint/side effect isn't mentioned anywhere, so I >> assumed it was unintentional. And I never looked at any man pages, >> just used bridge link help to find out what the arguments are to >> (un)set those port flags. So I looked everywhere except where this >> constraint is pointed out. >> >> Anyway, I understand your concern about already having a knob to avoid >> the issue, my concern here is that the knob isn't quite obvious, and >> that you do need an additional knob to have a "secure" default. So >> IMHO it's easy to miss as an inexperienced user. Though at least in >> the !MAB case, disabling learning on the port is also enough to avoid >> that (and keeps learning via link local enabled for unlocked ports). >> >> At least in the case of having enabled BR_PORT_MAB, I would consider >> it a bug that the entries learned via link local traffic aren't marked >> as BR_FDB_LOCKED. If you agree, I can send in a reduced patch for >> that, so that the entries are initially locked regardless the source >> of learning. > > I will give a bit of background so that my answer will make more sense. > > AFAICT, there are three different ways to deploy 802.1X / MAB: > > 1. 802.1X with static FDB entries. In this case learning can be > disabled. > > 2. 802.1X with dynamic FDB entries. In this case learning needs to be > enabled so that entries will be refreshed by incoming traffic. > > 3. MAB. In this case learning needs to be enabled so that user space > will be notified about hosts that are trying to communicate through the > bridge. > > When the original patch was posted I was not aware of the last two use > cases that require learning to be enabled. > > In any scenario where you have +learning +locked (regardless of +/-mab) > you need to have +no_linklocal_learn for things to work correctly, so > the potential for regressions from the original patch seems low to me. > > The original patch also provides a more comprehensive solution to the > problem than marking entries learned from link local traffic with > BR_FDB_LOCKED. It applies regardless of +/-mab (i.e., it covers both > cases 2 and 3 and not only 3). That is why I prefer the original patch > over the proposed approach. The original patch (just disabling LL learning if port is locked) has the same issue as mine, it will indirectly break switchdev offloading for case 2 when not using MAB (the kernel feature). Once we disable creating dynamic entries in the kernel, userspace needs to create them, and userspace dynamic entries have the user bit set, which makes them get ignored by switchdev. Ofc enabling MAB and then unlocking the locked entries hosts that successfully authenticated should still work for 2, as long as the host sent something other than link local traffic to create a (locked) dynamic entry AFAIU. FWIW, my proposed change/fix would be: diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c index ceaa5a89b947..41b69ea300bf 100644 --- a/net/bridge/br_input.c +++ b/net/bridge/br_input.c @@ -238,7 +238,8 @@ static void __br_handle_local_finish(struct sk_buff *skb) nbp_state_should_learn(p) && !br_opt_get(p->br, BROPT_NO_LL_LEARN) && br_should_learn(p, skb, &vid)) - br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid, 0); + br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid, + p->flags & BR_PORT_MAB ? BIT(BR_FDB_LOCKED) : 0); } /* note: already called with rcu_read_lock */ which just makes sure that when MAB is enabled, link local learned entries are also locked. This relies on br_fdb_update() ignoring most flags for existing entries, not sure if this is a good idea though. Best Regards, Jonas
On Thu, Dec 12, 2024 at 10:50:10AM +0100, Jonas Gorski wrote: > The original patch (just disabling LL learning if port is locked) has > the same issue as mine, it will indirectly break switchdev offloading > for case 2 when not using MAB (the kernel feature). > > Once we disable creating dynamic entries in the kernel, userspace needs > to create them, But the whole point of the "locked" feature is to defer the installation of FDB entries to user space so that the control plane will be able to decide which hosts can communicate through the bridge. Having the kernel auto-populate the FDB based on incoming packets defeats this purpose, which is why the man page mentions the "no_linklocal_learn" option and why I think there is a very low risk of regressions from the original patch. > and userspace dynamic entries have the user bit set, which makes them > get ignored by switchdev. The second use case never worked correctly in the offload case. It is not a regression. > > Ofc enabling MAB and then unlocking the locked entries hosts that > successfully authenticated should still work for 2, as long as the host > sent something other than link local traffic to create a (locked) > dynamic entry AFAIU. > > FWIW, my proposed change/fix would be: > > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c > index ceaa5a89b947..41b69ea300bf 100644 > --- a/net/bridge/br_input.c > +++ b/net/bridge/br_input.c > @@ -238,7 +238,8 @@ static void __br_handle_local_finish(struct sk_buff *skb) > nbp_state_should_learn(p) && > !br_opt_get(p->br, BROPT_NO_LL_LEARN) && > br_should_learn(p, skb, &vid)) > - br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid, 0); > + br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid, > + p->flags & BR_PORT_MAB ? BIT(BR_FDB_LOCKED) : 0); IIUC, this will potentially roam FDB entries to unauthorized ports, unlike the implementation in br_handle_frame_finish(). I documented it in commit a35ec8e38cdd ("bridge: Add MAC Authentication Bypass (MAB) support") in "1. Roaming". > } > > /* note: already called with rcu_read_lock */ > > which just makes sure that when MAB is enabled, link local learned > entries are also locked. This relies on br_fdb_update() ignoring most > flags for existing entries, not sure if this is a good idea though. > > > Best Regards, > Jonas > > -- > BISDN GmbH > Körnerstraße 7-10 > 10785 Berlin > Germany > > > Phone: > +49-30-6108-1-6100 > > > Managing Directors: > Dr.-Ing. Hagen Woesner, Andreas > Köpsel > > > Commercial register: > Amtsgericht Berlin-Charlottenburg HRB 141569 > B > VAT ID No: DE283257294 >
On 12.12.24 13:25, Ido Schimmel wrote: > On Thu, Dec 12, 2024 at 10:50:10AM +0100, Jonas Gorski wrote: >> The original patch (just disabling LL learning if port is locked) has >> the same issue as mine, it will indirectly break switchdev offloading >> for case 2 when not using MAB (the kernel feature). >> >> Once we disable creating dynamic entries in the kernel, userspace needs >> to create them, > > But the whole point of the "locked" feature is to defer the installation > of FDB entries to user space so that the control plane will be able to > decide which hosts can communicate through the bridge. Having the kernel > auto-populate the FDB based on incoming packets defeats this purpose, > which is why the man page mentions the "no_linklocal_learn" option and > why I think there is a very low risk of regressions from the original > patch. > >> and userspace dynamic entries have the user bit set, which makes them >> get ignored by switchdev. > > The second use case never worked correctly in the offload case. It is > not a regression. Ah, I just noticed I incorrectly assumed that unlocking an entry just means removing the locked flag, but any change from user space also implicitly adds the user bit. So they would never get offloaded. So there currently is no way to get offloaded dynamic entries with locked ports, regardless of MAB or not. And implementing that is definitely not a oneliner. >> FWIW, my proposed change/fix would be: >> >> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c >> index ceaa5a89b947..41b69ea300bf 100644 >> --- a/net/bridge/br_input.c >> +++ b/net/bridge/br_input.c >> @@ -238,7 +238,8 @@ static void __br_handle_local_finish(struct sk_buff *skb) >> nbp_state_should_learn(p) && >> !br_opt_get(p->br, BROPT_NO_LL_LEARN) && >> br_should_learn(p, skb, &vid)) >> - br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid, 0); >> + br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid, >> + p->flags & BR_PORT_MAB ? BIT(BR_FDB_LOCKED) : 0); > > IIUC, this will potentially roam FDB entries to unauthorized ports, > unlike the implementation in br_handle_frame_finish(). I documented it > in commit a35ec8e38cdd ("bridge: Add MAC Authentication Bypass (MAB) > support") in "1. Roaming". Good point, missed that br_fdb_update() will automatically clear the locked flag on roaming regardless of flags including it. So disregard that idea as well, and we would need to do what I originally proposed to allow locked learning via link local traffic. But since there is no difference between kernel entries that userspace unlocked and dynamic entries userspace added as I now found out, it also isn't needed at all. Userspace can just add dynamic entries if needed. So in conclusion, I agree with the original patch. Shall I resend it? Should I extend the commit message? Best Regards, Jonas
On Fri, Dec 13, 2024 at 12:25:20PM +0100, Jonas Gorski wrote: > So in conclusion, I agree with the original patch. Shall I resend it? Should > I extend the commit message? Yes. I would use something like: " There are legitimate use cases for enabling learning on a locked bridge port such as MAC Authentication Bypass (MAB) or when user space authorizes hosts using dynamic FDB entries. Currently, by default, the bridge will autonomously populate its FDB with addresses learned from link-local frames. This is true even when a port is locked which defeats the purpose of the "locked" bridge port option. The behavior can be controlled by the "no_linklocal_learn" bridge option, but it is easy to miss which leads to insecure configurations. Fix this by skipping learning from link-local frames when a port is locked. Fixes: a21d9a670d81 ("net: bridge: Add support for bridge port in locked mode") "
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c index ceaa5a89b947..f269a9f1b871 100644 --- a/net/bridge/br_input.c +++ b/net/bridge/br_input.c @@ -72,6 +72,46 @@ static int br_pass_frame_up(struct sk_buff *skb, bool promisc) br_netif_receive_skb); } +static int br_fdb_locked_learn(struct net_bridge_port *p, struct sk_buff *skb, + u16 vid, bool mark) +{ + struct net_bridge *br = p->br; + + if (p->flags & BR_PORT_LOCKED) { + struct net_bridge_fdb_entry *fdb_src = + br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid); + if (!fdb_src) { + /* FDB miss. Create locked FDB entry if MAB is enabled + * and drop the packet. + */ + if (p->flags & BR_PORT_MAB) + br_fdb_update(br, p, eth_hdr(skb)->h_source, + vid, BIT(BR_FDB_LOCKED)); + return NET_RX_DROP; + } else if (READ_ONCE(fdb_src->dst) != p || + test_bit(BR_FDB_LOCAL, &fdb_src->flags)) { + /* FDB mismatch. Drop the packet without roaming. */ + return NET_RX_DROP; + } else if (test_bit(BR_FDB_LOCKED, &fdb_src->flags)) { + /* FDB match, but entry is locked. Refresh it and drop + * the packet. + */ + br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, + BIT(BR_FDB_LOCKED)); + return NET_RX_DROP; + } + } + + if (mark) + nbp_switchdev_frame_mark(p, skb); + + /* insert into forwarding database after filtering to avoid spoofing */ + if (p->flags & BR_LEARNING) + br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, 0); + + return NET_RX_SUCCESS; +} + /* note: already called with rcu_read_lock */ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb) { @@ -108,37 +148,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb &state, &vlan)) goto out; - if (p->flags & BR_PORT_LOCKED) { - struct net_bridge_fdb_entry *fdb_src = - br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid); - - if (!fdb_src) { - /* FDB miss. Create locked FDB entry if MAB is enabled - * and drop the packet. - */ - if (p->flags & BR_PORT_MAB) - br_fdb_update(br, p, eth_hdr(skb)->h_source, - vid, BIT(BR_FDB_LOCKED)); - goto drop; - } else if (READ_ONCE(fdb_src->dst) != p || - test_bit(BR_FDB_LOCAL, &fdb_src->flags)) { - /* FDB mismatch. Drop the packet without roaming. */ - goto drop; - } else if (test_bit(BR_FDB_LOCKED, &fdb_src->flags)) { - /* FDB match, but entry is locked. Refresh it and drop - * the packet. - */ - br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, - BIT(BR_FDB_LOCKED)); - goto drop; - } - } - - nbp_switchdev_frame_mark(p, skb); - - /* insert into forwarding database after filtering to avoid spoofing */ - if (p->flags & BR_LEARNING) - br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, 0); + if (br_fdb_locked_learn(p, skb, vid, true) == NET_RX_DROP) + goto drop; promisc = !!(br->dev->flags & IFF_PROMISC); local_rcv = promisc; @@ -234,11 +245,10 @@ static void __br_handle_local_finish(struct sk_buff *skb) u16 vid = 0; /* check if vlan is allowed, to avoid spoofing */ - if ((p->flags & BR_LEARNING) && - nbp_state_should_learn(p) && + if (nbp_state_should_learn(p) && !br_opt_get(p->br, BROPT_NO_LL_LEARN) && br_should_learn(p, skb, &vid)) - br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid, 0); + br_fdb_locked_learn(p, skb, vid, false); } /* note: already called with rcu_read_lock */
When support for locked ports was added with commit a21d9a670d81 ("net: bridge: Add support for bridge port in locked mode"), learning is inhibited when the port is locked in br_handle_frame_finish(). It was later extended in commit a35ec8e38cdd ("bridge: Add MAC Authentication Bypass (MAB) support") where optionally learning is done with locked entries. Unfortunately both missed that learning may also happen on frames to link local addresses (01:80:c2:00:00:0X) in br_handle_frame(), which will call __br_handle_local_finish(), which may update the fdb unless (ll) learning is disabled as well. This can be easily observed by e.g. EAPOL frames to 01:80:c2:00:00:03 on a port causing the source mac to be learned, which is then forwarded normally, essentially bypassing any authentication. Fix this by moving the BR_PORT_LOCKED handling into its own function, and call it from both places. Fixes: a21d9a670d81 ("net: bridge: Add support for bridge port in locked mode") Fixes: a35ec8e38cdd ("bridge: Add MAC Authentication Bypass (MAB) support") Signed-off-by: Jonas Gorski <jonas.gorski@bisdn.de> --- Sent as RFC since I'm not 100% sure this is the right way to fix. net/bridge/br_input.c | 78 ++++++++++++++++++++++++------------------- 1 file changed, 44 insertions(+), 34 deletions(-)