lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZGM64ODoVwK8J4u2@u-jnixdorf.ads.avm.de> Date: Tue, 16 May 2023 10:12:16 +0200 From: Johannes Nixdorf <jnixdorf-oss@....de> To: Nikolay Aleksandrov <razor@...ckwall.org> Cc: netdev@...r.kernel.org, bridge@...ts.linux-foundation.org, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Roopa Prabhu <roopa@...dia.com> Subject: Re: [PATCH net-next 1/2] bridge: Add a limit on FDB entries Hi, On Mon, May 15, 2023 at 12:35:03PM +0300, Nikolay Aleksandrov wrote: > On 15/05/2023 11:50, Johannes Nixdorf wrote: > > A malicious actor behind one bridge port may spam the kernel with packets > > with a random source MAC address, each of which will create an FDB entry, > > each of which is a dynamic allocation in the kernel. > > > > There are roughly 2^48 different MAC addresses, further limited by the > > rhashtable they are stored in to 2^31. Each entry is of the type struct > > net_bridge_fdb_entry, which is currently 128 bytes big. This means the > > maximum amount of memory allocated for FDB entries is 2^31 * 128B = > > 256GiB, which is too much for most computers. > > > > Mitigate this by adding a bridge netlink setting IFLA_BR_FDB_MAX_ENTRIES, > > which, if nonzero, limits the amount of entries to a user specified > > maximum. > > > > For backwards compatibility the default setting of 0 disables the limit. > > > > All changes to fdb_n_entries are under br->hash_lock, which means we do > > not need additional locking. The call paths are (✓ denotes that > > br->hash_lock is taken around the next call): > > > > - fdb_delete <-+- fdb_delete_local <-+- br_fdb_changeaddr ✓ > > | +- br_fdb_change_mac_address ✓ > > | +- br_fdb_delete_by_port ✓ > > +- br_fdb_find_delete_local ✓ > > +- fdb_add_local <-+- br_fdb_changeaddr ✓ > > | +- br_fdb_change_mac_address ✓ > > | +- br_fdb_add_local ✓ > > +- br_fdb_cleanup ✓ > > +- br_fdb_flush ✓ > > +- br_fdb_delete_by_port ✓ > > +- fdb_delete_by_addr_and_port <--- __br_fdb_delete ✓ > > +- br_fdb_external_learn_del ✓ > > - fdb_create <-+- fdb_add_local <-+- br_fdb_changeaddr ✓ > > | +- br_fdb_change_mac_address ✓ > > | +- br_fdb_add_local ✓ > > +- br_fdb_update ✓ > > +- fdb_add_entry <--- __br_fdb_add ✓ > > +- br_fdb_external_learn_add ✓ > > > > Signed-off-by: Johannes Nixdorf <jnixdorf-oss@....de> > > --- > > include/uapi/linux/if_link.h | 1 + > > net/bridge/br_device.c | 2 ++ > > net/bridge/br_fdb.c | 6 ++++++ > > net/bridge/br_netlink.c | 9 ++++++++- > > net/bridge/br_private.h | 2 ++ > > 5 files changed, 19 insertions(+), 1 deletion(-) > > > > Hi, > If you're sending a patch series please add a cover letter (--cover-letter) which > explains what the series are trying to do and why. Thanks for the hint. I'll do that in the future, including a potential v2. > I've had a patch that implements this feature for a while but didn't get to upstreaming it. :) I'm not too attached to my name on it, so if your patch is further along, I'd be happy if you submitted your version instead. > Anyway more comments below, > > diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h > > index 4ac1000b0ef2..27cf5f2d8790 100644 > > --- a/include/uapi/linux/if_link.h > > +++ b/include/uapi/linux/if_link.h > > @@ -510,6 +510,7 @@ enum { > > IFLA_BR_VLAN_STATS_PER_PORT, > > IFLA_BR_MULTI_BOOLOPT, > > IFLA_BR_MCAST_QUERIER_STATE, > > + IFLA_BR_FDB_MAX_ENTRIES, > > __IFLA_BR_MAX, > > }; > > > > diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c > > index 8eca8a5c80c6..d455a28df7c9 100644 > > --- a/net/bridge/br_device.c > > +++ b/net/bridge/br_device.c > > @@ -528,6 +528,8 @@ void br_dev_setup(struct net_device *dev) > > br->bridge_hello_time = br->hello_time = 2 * HZ; > > br->bridge_forward_delay = br->forward_delay = 15 * HZ; > > br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME; > > + br->fdb_n_entries = 0; > > + br->fdb_max_entries = 0; > > Unnecessary, the private area is already cleared. This will be taken out in a v2. > > dev->max_mtu = ETH_MAX_MTU; > > > > br_netfilter_rtable_init(br); > > diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c > > index e69a872bfc1d..8a833e6dee92 100644 > > --- a/net/bridge/br_fdb.c > > +++ b/net/bridge/br_fdb.c > > @@ -329,6 +329,8 @@ static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f, > > hlist_del_init_rcu(&f->fdb_node); > > rhashtable_remove_fast(&br->fdb_hash_tbl, &f->rhnode, > > br_fdb_rht_params); > > + if (!WARN_ON(!br->fdb_n_entries)) > > + br->fdb_n_entries--; > > This is pointless, just put the WARN_ON(!br->fdb_n_entries) above decrementing, if we > hit that we are already in trouble and not decrementing doesn't help us. This will now always be decremented in a v2. > > fdb_notify(br, f, RTM_DELNEIGH, swdev_notify); > > call_rcu(&f->rcu, fdb_rcu_free); > > } > > @@ -391,6 +393,9 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br, > > struct net_bridge_fdb_entry *fdb; > > int err; > > > > + if (unlikely(br->fdb_max_entries && br->fdb_n_entries >= br->fdb_max_entries)) > > + return NULL; > > + > > This one needs more work, fdb_create() is also used when user-space is adding new > entries, so it would be nice to return a proper error. The callers usually map this return code to -ENOMEM, which I deemed an appropriate return code for violating the new limit, as I understood it as a memory limit for the FDB table. Is there a different error return code you had in mind here, or would you rather only count dynamic entries towards the limit at all? > > fdb = kmem_cache_alloc(br_fdb_cache, GFP_ATOMIC); > > if (!fdb) > > return NULL; > > @@ -408,6 +413,7 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br, > > } > > > > hlist_add_head_rcu(&fdb->fdb_node, &br->fdb_list); > > + br->fdb_n_entries++; > > > > return fdb; > > } > > diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c > > index 05c5863d2e20..e5b8d36a3291 100644 > > --- a/net/bridge/br_netlink.c > > +++ b/net/bridge/br_netlink.c > > @@ -1527,6 +1527,12 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[], > > return err; > > } > > > > + if (data[IFLA_BR_FDB_MAX_ENTRIES]) { > > + u32 val = nla_get_u32(data[IFLA_BR_FDB_MAX_ENTRIES]); > > + > > + br->fdb_max_entries = val; > > + } > > + > > return 0; > > } > > > > @@ -1656,7 +1662,8 @@ static int br_fill_info(struct sk_buff *skb, const struct net_device *brdev) > > nla_put_u8(skb, IFLA_BR_TOPOLOGY_CHANGE_DETECTED, > > br->topology_change_detected) || > > nla_put(skb, IFLA_BR_GROUP_ADDR, ETH_ALEN, br->group_addr) || > > - nla_put(skb, IFLA_BR_MULTI_BOOLOPT, sizeof(bm), &bm)) > > + nla_put(skb, IFLA_BR_MULTI_BOOLOPT, sizeof(bm), &bm) || > > + nla_put_u32(skb, IFLA_BR_FDB_MAX_ENTRIES, br->fdb_max_entries)) > > You are not returning the current entry count, that is also needed. For a v2 this now also returns the current entry count under IFLA_BR_FDB_CUR_ENTRIES. > > return -EMSGSIZE; > > > > #ifdef CONFIG_BRIDGE_VLAN_FILTERING > > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > > index 2119729ded2b..64fb359c6e3e 100644 > > --- a/net/bridge/br_private.h > > +++ b/net/bridge/br_private.h > > @@ -494,6 +494,8 @@ struct net_bridge { > > #endif > > > > struct rhashtable fdb_hash_tbl; > > + u32 fdb_n_entries; > > + u32 fdb_max_entries; > > These are not critical, so I'd use 4 byte holes in net_bridge and pack it better > instead of making it larger. For a v2 I now moved it into (conditional) holes now in front of CONFIG_BRIDGE_VLAN_FILTERING (only a hole if it is enabled) and CONFIG_SWITCHDEV (only a hole if it is disabled). I could not find any other holes, but please tell me if you had any others in mind. > > struct list_head port_list; > > #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER) > > union { > Thanks for your detailed feedback.
Powered by blists - more mailing lists