Introduce a limit on the amount of learned FDB entries on a bridge,
configured by netlink with a build time default on bridge creation in
the kernel config.
For backwards compatibility the kernel config default is disabling the
limit (0).
Without any limit a malicious actor may OOM a kernel by spamming packets
with changing MAC addresses on their bridge port, so allow the bridge
creator to limit the number of entries.
Currently the manual entries are identified by the bridge flags
BR_FDB_LOCAL or BR_FDB_ADDED_BY_USER, atomically bundled under the new
flag BR_FDB_DYNAMIC_LEARNED. This means the limit also applies to
entries created with BR_FDB_ADDED_BY_EXT_LEARN but none of BR_FDB_LOCAL
or BR_FDB_ADDED_BY_USER, e.g. ones added by SWITCHDEV_FDB_ADD_TO_BRIDGE.
Link to the corresponding iproute2 changes:
https://lore.kernel.org/r/[email protected]
Signed-off-by: Johannes Nixdorf <[email protected]>
---
Changes in v5:
- Set IFLA_BR_FDB_N_LEARNED to NLA_REJECT (from review)
- Moved the strict_start_type-commit after the netlink change, used
the new attribute. (from review)
- Dropped the new build time config option. (from review)
- Link to v4: https://lore.kernel.org/r/[email protected]
Changes in v4:
- Added the new test to the Makefile. (from review)
- Removed _entries from the names. (from iproute2 review, in some places
only for consistency)
- Wrapped the lines at 80 chars, except when longer lines are consistent
with neighbouring code. (from review)
- Fixed a race in fdb_delete. (from review)
- Link to v3: https://lore.kernel.org/r/[email protected]
Changes in v3:
- Fixed the flags for fdb_create in fdb_add_entry to use
BIT(...). Previously we passed garbage. (from review)
- Set strict_start_type for br_policy. (from review)
- Split out the combined accounting and limit patch, and the netlink
patch from the combined patch in v2. (from review)
- Count atomically, remove the newly introduced lock. (from review)
- Added the new attributes to br_policy. (from review)
- Added a selftest for the new feature. (from review)
- Link to v2: https://lore.kernel.org/netdev/[email protected]/
Changes in v2:
- Added BR_FDB_ADDED_BY_USER earlier in fdb_add_entry to ensure the
limit is not applied.
- Do not initialize fdb_*_entries to 0. (from review)
- Do not skip decrementing on 0. (from review)
- Moved the counters to a conditional hole in struct net_bridge to
avoid growing the struct. (from review, it still grows the struct as
there are 2 32-bit values)
- Add IFLA_BR_FDB_CUR_LEARNED_ENTRIES (from review)
- Fix br_get_size() with the added attributes.
- Only limit learned entries, rename to
*_(CUR|MAX)_LEARNED_ENTRIES. (from review)
- Added a default limit in Kconfig. (deemed acceptable in review
comments, helps with embedded use-cases where a special purpose kernel
is built anyways)
- Added an iproute2 patch for easier testing.
- Link to v1: https://lore.kernel.org/netdev/[email protected]/
Obsolete v1 review comments:
- Return better errors to users: Due to limiting the limit to
automatically created entries, netlink fdb add requests and changing
bridge ports are never rejected, so they do not yet need a more
friendly error returned.
---
Johannes Nixdorf (5):
net: bridge: Set BR_FDB_ADDED_BY_USER early in fdb_add_entry
net: bridge: Track and limit dynamically learned FDB entries
net: bridge: Add netlink knobs for number / max learned FDB entries
net: bridge: Set strict_start_type for br_policy
selftests: forwarding: bridge_fdb_learning_limit: Add a new selftest
include/uapi/linux/if_link.h | 2 +
net/bridge/br_fdb.c | 42 ++-
net/bridge/br_netlink.c | 17 +-
net/bridge/br_private.h | 4 +
tools/testing/selftests/net/forwarding/Makefile | 3 +-
.../net/forwarding/bridge_fdb_learning_limit.sh | 283 +++++++++++++++++++++
6 files changed, 344 insertions(+), 7 deletions(-)
---
base-commit: 58720809f52779dc0f08e53e54b014209d13eebb
change-id: 20230904-fdb_limit-fae5bbf16c88
Best regards,
--
Johannes Nixdorf <[email protected]>
Add a suite covering the fdb_n_learned and fdb_max_learned bridge
features, touching all special cases in accounting at least once.
Acked-by: Nikolay Aleksandrov <[email protected]>
Signed-off-by: Johannes Nixdorf <[email protected]>
---
tools/testing/selftests/net/forwarding/Makefile | 3 +-
.../net/forwarding/bridge_fdb_learning_limit.sh | 283 +++++++++++++++++++++
2 files changed, 285 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile
index 74e754e266c3..df593b7b3e6b 100644
--- a/tools/testing/selftests/net/forwarding/Makefile
+++ b/tools/testing/selftests/net/forwarding/Makefile
@@ -1,6 +1,7 @@
# SPDX-License-Identifier: GPL-2.0+ OR MIT
-TEST_PROGS = bridge_igmp.sh \
+TEST_PROGS = bridge_fdb_learning_limit.sh \
+ bridge_igmp.sh \
bridge_locked_port.sh \
bridge_mdb.sh \
bridge_mdb_host.sh \
diff --git a/tools/testing/selftests/net/forwarding/bridge_fdb_learning_limit.sh b/tools/testing/selftests/net/forwarding/bridge_fdb_learning_limit.sh
new file mode 100755
index 000000000000..0760a34b7114
--- /dev/null
+++ b/tools/testing/selftests/net/forwarding/bridge_fdb_learning_limit.sh
@@ -0,0 +1,283 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+# ShellCheck incorrectly believes that most of the code here is unreachable
+# because it's invoked by variable name following ALL_TESTS.
+#
+# shellcheck disable=SC2317
+
+ALL_TESTS="check_accounting check_limit"
+NUM_NETIFS=6
+source lib.sh
+
+TEST_MAC_BASE=de:ad:be:ef:42:
+
+NUM_PKTS=16
+FDB_LIMIT=8
+
+FDB_TYPES=(
+ # name is counted? overrides learned?
+ 'learned 1 0'
+ 'static 0 1'
+ 'user 0 1'
+ 'extern_learn 0 1'
+ 'local 0 1'
+)
+
+mac()
+{
+ printf "${TEST_MAC_BASE}%02x" "$1"
+}
+
+H1_DEFAULT_MAC=$(mac 42)
+
+switch_create()
+{
+ ip link add dev br0 type bridge
+
+ ip link set dev "$swp1" master br0
+ ip link set dev "$swp2" master br0
+ # swp3 is used to add local MACs, so do not add it to the bridge yet.
+
+ # swp2 is only used for replying when learning on swp1, its MAC should not be learned.
+ ip link set dev "$swp2" type bridge_slave learning off
+
+ ip link set dev br0 up
+
+ ip link set dev "$swp1" up
+ ip link set dev "$swp2" up
+ ip link set dev "$swp3" up
+}
+
+switch_destroy()
+{
+ ip link set dev "$swp3" down
+ ip link set dev "$swp2" down
+ ip link set dev "$swp1" down
+
+ ip link del dev br0
+}
+
+h_create()
+{
+ ip link set "$h1" addr "$H1_DEFAULT_MAC"
+
+ simple_if_init "$h1" 192.0.2.1/24
+ simple_if_init "$h2" 192.0.2.2/24
+}
+
+h_destroy()
+{
+ simple_if_fini "$h1" 192.0.2.1/24
+ simple_if_fini "$h2" 192.0.2.2/24
+}
+
+setup_prepare()
+{
+ h1=${NETIFS[p1]}
+ swp1=${NETIFS[p2]}
+
+ h2=${NETIFS[p3]}
+ swp2=${NETIFS[p4]}
+
+ swp3=${NETIFS[p6]}
+
+ vrf_prepare
+
+ h_create
+
+ switch_create
+}
+
+cleanup()
+{
+ pre_cleanup
+
+ switch_destroy
+
+ h_destroy
+
+ vrf_cleanup
+}
+
+fdb_get_n_learned()
+{
+ ip -d -j link show dev br0 type bridge | \
+ jq '.[]["linkinfo"]["info_data"]["fdb_n_learned"]'
+}
+
+fdb_get_n_mac()
+{
+ local mac=${1}
+
+ bridge -j fdb show br br0 | \
+ jq "map(select(.mac == \"${mac}\" and (has(\"vlan\") | not))) | length"
+}
+
+fdb_fill_learned()
+{
+ local i
+
+ for i in $(seq 1 "$NUM_PKTS"); do
+ fdb_add learned "$(mac "$i")"
+ done
+}
+
+fdb_reset()
+{
+ bridge fdb flush dev br0
+
+ # Keep the default MAC address of h1 in the table. We set it to a different one when
+ # testing dynamic learning.
+ bridge fdb add "$H1_DEFAULT_MAC" dev "$swp1" master static use
+}
+
+fdb_add()
+{
+ local type=$1 mac=$2
+
+ case "$type" in
+ learned)
+ ip link set "$h1" addr "$mac"
+ # Wait for a reply so we implicitly wait until after the forwarding
+ # code finished and the FDB entry was created.
+ PING_COUNT=1 ping_do "$h1" 192.0.2.2
+ check_err $? "Failed to ping another bridge port"
+ ip link set "$h1" addr "$H1_DEFAULT_MAC"
+ ;;
+ local)
+ ip link set dev "$swp3" addr "$mac" && ip link set "$swp3" master br0
+ ;;
+ static)
+ bridge fdb replace "$mac" dev "$swp1" master static
+ ;;
+ user)
+ bridge fdb replace "$mac" dev "$swp1" master static use
+ ;;
+ extern_learn)
+ bridge fdb replace "$mac" dev "$swp1" master extern_learn
+ ;;
+ esac
+
+ check_err $? "Failed to add a FDB entry of type ${type}"
+}
+
+fdb_del()
+{
+ local type=$1 mac=$2
+
+ case "$type" in
+ local)
+ ip link set "$swp3" nomaster
+ ;;
+ *)
+ bridge fdb del "$mac" dev "$swp1" master
+ ;;
+ esac
+
+ check_err $? "Failed to remove a FDB entry of type ${type}"
+}
+
+check_accounting_one_type()
+{
+ local type=$1 is_counted=$2 overrides_learned=$3
+ shift 3
+ RET=0
+
+ fdb_reset
+ fdb_add "$type" "$(mac 0)"
+ learned=$(fdb_get_n_learned)
+ [ "$learned" -ne "$is_counted" ]
+ check_fail $? "Inserted FDB type ${type}: Expected the count ${is_counted}, but got ${learned}"
+
+ fdb_del "$type" "$(mac 0)"
+ learned=$(fdb_get_n_learned)
+ [ "$learned" -ne 0 ]
+ check_fail $? "Removed FDB type ${type}: Expected the count 0, but got ${learned}"
+
+ if [ "$overrides_learned" -eq 1 ]; then
+ fdb_reset
+ fdb_add learned "$(mac 0)"
+ fdb_add "$type" "$(mac 0)"
+ learned=$(fdb_get_n_learned)
+ [ "$learned" -ne "$is_counted" ]
+ check_fail $? "Set a learned entry to FDB type ${type}: Expected the count ${is_counted}, but got ${learned}"
+ fdb_del "$type" "$(mac 0)"
+ fi
+
+ log_test "FDB accounting interacting with FDB type ${type}"
+}
+
+check_accounting()
+{
+ local type_args learned
+ RET=0
+
+ fdb_reset
+ learned=$(fdb_get_n_learned)
+ [ "$learned" -ne 0 ]
+ check_fail $? "Flushed the FDB table: Expected the count 0, but got ${learned}"
+
+ fdb_fill_learned
+ sleep 1
+
+ learned=$(fdb_get_n_learned)
+ [ "$learned" -ne "$NUM_PKTS" ]
+ check_fail $? "Filled the FDB table: Expected the count ${NUM_PKTS}, but got ${learned}"
+
+ log_test "FDB accounting"
+
+ for type_args in "${FDB_TYPES[@]}"; do
+ # This is intentional use of word splitting.
+ # shellcheck disable=SC2086
+ check_accounting_one_type $type_args
+ done
+}
+
+check_limit_one_type()
+{
+ local type=$1 is_counted=$2
+ local n_mac expected=$((1 - is_counted))
+ RET=0
+
+ fdb_reset
+ fdb_fill_learned
+
+ fdb_add "$type" "$(mac 0)"
+ n_mac=$(fdb_get_n_mac "$(mac 0)")
+ [ "$n_mac" -ne "$expected" ]
+ check_fail $? "Inserted FDB type ${type} at limit: Expected the count ${expected}, but got ${n_mac}"
+
+ log_test "FDB limits interacting with FDB type ${type}"
+}
+
+check_limit()
+{
+ local learned
+ RET=0
+
+ ip link set br0 type bridge fdb_max_learned "$FDB_LIMIT"
+
+ fdb_reset
+ fdb_fill_learned
+
+ learned=$(fdb_get_n_learned)
+ [ "$learned" -ne "$FDB_LIMIT" ]
+ check_fail $? "Filled the limited FDB table: Expected the count ${FDB_LIMIT}, but got ${learned}"
+
+ log_test "FDB limits"
+
+ for type_args in "${FDB_TYPES[@]}"; do
+ # This is intentional use of word splitting.
+ # shellcheck disable=SC2086
+ check_limit_one_type $type_args
+ done
+}
+
+trap cleanup EXIT
+
+setup_prepare
+
+tests_run
+
+exit $EXIT_STATUS
--
2.42.0
Set any new attributes added to br_policy to be parsed strictly, to
prevent userspace from passing garbage.
Signed-off-by: Johannes Nixdorf <[email protected]>
---
net/bridge/br_netlink.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 0c3cf6e6dea2..5ad4abfcb7ba 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1229,6 +1229,8 @@ static size_t br_port_get_slave_size(const struct net_device *brdev,
}
static const struct nla_policy br_policy[IFLA_BR_MAX + 1] = {
+ [IFLA_BR_UNSPEC] = { .strict_start_type =
+ IFLA_BR_FDB_N_LEARNED },
[IFLA_BR_FORWARD_DELAY] = { .type = NLA_U32 },
[IFLA_BR_HELLO_TIME] = { .type = NLA_U32 },
[IFLA_BR_MAX_AGE] = { .type = NLA_U32 },
--
2.42.0
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 maintaining a per bridge count of those automatically
generated entries in fdb_n_learned, and a limit in fdb_max_learned. If
the limit is hit new entries are not learned anymore.
For backwards compatibility the default setting of 0 disables the limit.
User-added entries by netlink or from bridge or bridge port addresses
are never blocked and do not count towards that limit.
Introduce a new fdb entry flag BR_FDB_DYNAMIC_LEARNED to keep track of
whether an FDB entry is included in the count. The flag is enabled for
dynamically learned entries, and disabled for all other entries. This
should be equivalent to BR_FDB_ADDED_BY_USER and BR_FDB_LOCAL being unset,
but contrary to the two flags it can be toggled atomically.
Atomicity is required here, as there are multiple callers that modify the
flags, but are not under a common lock (br_fdb_update is the exception
for br->hash_lock, br_fdb_external_learn_add for RTNL).
Reviewed-by: Ido Schimmel <[email protected]>
Acked-by: Nikolay Aleksandrov <[email protected]>
Signed-off-by: Johannes Nixdorf <[email protected]>
---
net/bridge/br_fdb.c | 35 +++++++++++++++++++++++++++++++++--
net/bridge/br_private.h | 4 ++++
2 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index f517ea92132c..cf77e71e026f 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -329,11 +329,18 @@ 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 (test_and_clear_bit(BR_FDB_DYNAMIC_LEARNED, &f->flags))
+ atomic_dec(&br->fdb_n_learned);
fdb_notify(br, f, RTM_DELNEIGH, swdev_notify);
call_rcu(&f->rcu, fdb_rcu_free);
}
-/* Delete a local entry if no other port had the same address. */
+/* Delete a local entry if no other port had the same address.
+ *
+ * This function should only be called on entries with BR_FDB_LOCAL set,
+ * so even with BR_FDB_ADDED_BY_USER cleared we never need to increase
+ * the accounting for dynamically learned entries again.
+ */
static void fdb_delete_local(struct net_bridge *br,
const struct net_bridge_port *p,
struct net_bridge_fdb_entry *f)
@@ -388,9 +395,20 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
__u16 vid,
unsigned long flags)
{
+ bool learned = !test_bit(BR_FDB_ADDED_BY_USER, &flags) &&
+ !test_bit(BR_FDB_LOCAL, &flags);
+ u32 max_learned = READ_ONCE(br->fdb_max_learned);
struct net_bridge_fdb_entry *fdb;
int err;
+ if (likely(learned)) {
+ int n_learned = atomic_read(&br->fdb_n_learned);
+
+ if (unlikely(max_learned && n_learned >= max_learned))
+ return NULL;
+ __set_bit(BR_FDB_DYNAMIC_LEARNED, &flags);
+ }
+
fdb = kmem_cache_alloc(br_fdb_cache, GFP_ATOMIC);
if (!fdb)
return NULL;
@@ -407,6 +425,9 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
return NULL;
}
+ if (likely(learned))
+ atomic_inc(&br->fdb_n_learned);
+
hlist_add_head_rcu(&fdb->fdb_node, &br->fdb_list);
return fdb;
@@ -893,8 +914,12 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
clear_bit(BR_FDB_LOCKED, &fdb->flags);
}
- if (unlikely(test_bit(BR_FDB_ADDED_BY_USER, &flags)))
+ if (unlikely(test_bit(BR_FDB_ADDED_BY_USER, &flags))) {
set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
+ if (test_and_clear_bit(BR_FDB_DYNAMIC_LEARNED,
+ &fdb->flags))
+ atomic_dec(&br->fdb_n_learned);
+ }
if (unlikely(fdb_modified)) {
trace_br_fdb_update(br, source, addr, vid, flags);
fdb_notify(br, fdb, RTM_NEWNEIGH, true);
@@ -1072,6 +1097,8 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
}
set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
+ if (test_and_clear_bit(BR_FDB_DYNAMIC_LEARNED, &fdb->flags))
+ atomic_dec(&br->fdb_n_learned);
}
if (fdb_to_nud(br, fdb) != state) {
@@ -1446,6 +1473,10 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
if (!p)
set_bit(BR_FDB_LOCAL, &fdb->flags);
+ if ((swdev_notify || !p) &&
+ test_and_clear_bit(BR_FDB_DYNAMIC_LEARNED, &fdb->flags))
+ atomic_dec(&br->fdb_n_learned);
+
if (modified)
fdb_notify(br, fdb, RTM_NEWNEIGH, swdev_notify);
}
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index a1f4acfa6994..8d2f9a3a3ecd 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -274,6 +274,7 @@ enum {
BR_FDB_NOTIFY,
BR_FDB_NOTIFY_INACTIVE,
BR_FDB_LOCKED,
+ BR_FDB_DYNAMIC_LEARNED,
};
struct net_bridge_fdb_key {
@@ -555,6 +556,9 @@ struct net_bridge {
struct kobject *ifobj;
u32 auto_cnt;
+ atomic_t fdb_n_learned;
+ u32 fdb_max_learned;
+
#ifdef CONFIG_NET_SWITCHDEV
/* Counter used to make sure that hardware domains get unique
* identifiers in case a bridge spans multiple switchdev instances.
--
2.42.0
On 10/16/23 16:27, Johannes Nixdorf wrote:
> Set any new attributes added to br_policy to be parsed strictly, to
> prevent userspace from passing garbage.
>
> Signed-off-by: Johannes Nixdorf <[email protected]>
> ---
> net/bridge/br_netlink.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index 0c3cf6e6dea2..5ad4abfcb7ba 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -1229,6 +1229,8 @@ static size_t br_port_get_slave_size(const struct net_device *brdev,
> }
>
> static const struct nla_policy br_policy[IFLA_BR_MAX + 1] = {
> + [IFLA_BR_UNSPEC] = { .strict_start_type =
> + IFLA_BR_FDB_N_LEARNED },
> [IFLA_BR_FORWARD_DELAY] = { .type = NLA_U32 },
> [IFLA_BR_HELLO_TIME] = { .type = NLA_U32 },
> [IFLA_BR_MAX_AGE] = { .type = NLA_U32 },
>
Acked-by: Nikolay Aleksandrov <[email protected]>
On Mon, Oct 16, 2023 at 03:27:23PM +0200, Johannes Nixdorf wrote:
> Set any new attributes added to br_policy to be parsed strictly, to
> prevent userspace from passing garbage.
>
> Signed-off-by: Johannes Nixdorf <[email protected]>
Reviewed-by: Ido Schimmel <[email protected]>
Hello:
This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <[email protected]>:
On Mon, 16 Oct 2023 15:27:19 +0200 you wrote:
> Introduce a limit on the amount of learned FDB entries on a bridge,
> configured by netlink with a build time default on bridge creation in
> the kernel config.
>
> For backwards compatibility the kernel config default is disabling the
> limit (0).
>
> [...]
Here is the summary with links:
- [net-next,v5,1/5] net: bridge: Set BR_FDB_ADDED_BY_USER early in fdb_add_entry
https://git.kernel.org/netdev/net-next/c/cbf51acbc5d5
- [net-next,v5,2/5] net: bridge: Track and limit dynamically learned FDB entries
https://git.kernel.org/netdev/net-next/c/bdb4dfda3b41
- [net-next,v5,3/5] net: bridge: Add netlink knobs for number / max learned FDB entries
https://git.kernel.org/netdev/net-next/c/ddd1ad68826d
- [net-next,v5,4/5] net: bridge: Set strict_start_type for br_policy
https://git.kernel.org/netdev/net-next/c/19297c3ab23c
- [net-next,v5,5/5] selftests: forwarding: bridge_fdb_learning_limit: Add a new selftest
https://git.kernel.org/netdev/net-next/c/6f84090333bb
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html