From 564fce9a3d457ae7b7ee0e9b76a68d477396e57e Mon Sep 17 00:00:00 2001 From: Quentin Deslandes Date: Thu, 12 Mar 2026 16:03:43 +0100 Subject: [PATCH] lib: chain: disable rules with incompatible same-layer matchers A rule combining matchers on the same network layer but targeting different protocols (e.g. ip4.saddr + ip6.daddr, or tcp.sport + udp.dport) can never match a packet. Detect these during chain validation and disable the rule instead of generating dead BPF code. --- src/libbpfilter/chain.c | 50 +++++++++ tests/e2e/CMakeLists.txt | 1 + tests/e2e/rules/incompatible_matchers.sh | 26 +++++ tests/unit/libbpfilter/chain.c | 132 +++++++++++++++++++++++ 4 files changed, 209 insertions(+) create mode 100755 tests/e2e/rules/incompatible_matchers.sh diff --git a/src/libbpfilter/chain.c b/src/libbpfilter/chain.c index 9213febf..f210a969 100644 --- a/src/libbpfilter/chain.c +++ b/src/libbpfilter/chain.c @@ -58,6 +58,53 @@ static int _bf_rule_references_empty_set(const struct bf_chain *chain, return 0; } +/** + * @brief Check if a rule has matchers on the same layer but different + * protocols. + * + * A rule matching on e.g. both IPv4 source address and IPv6 destination + * address can never match a packet (a packet is either IPv4 or IPv6, not + * both). Same for layer 4 (e.g. TCP vs UDP). Such rules should be disabled. + * + * @param rule Rule to check. + * @return 0 if no conflict, 1 if the rule contains incompatible matchers. + */ +static int _bf_rule_has_incompatible_matchers(const struct bf_rule *rule) +{ + uint32_t layer_hdr_id[_BF_MATCHER_LAYER_MAX]; + + assert(rule); + + for (int i = 0; i < _BF_MATCHER_LAYER_MAX; ++i) + layer_hdr_id[i] = (uint32_t)-1; + + bf_list_foreach (&rule->matchers, matcher_node) { + struct bf_matcher *matcher = bf_list_node_get_data(matcher_node); + const struct bf_matcher_meta *meta; + + if (bf_matcher_get_type(matcher) == BF_MATCHER_SET) + continue; + + meta = bf_matcher_get_meta(bf_matcher_get_type(matcher)); + if (!meta || meta->layer <= BF_MATCHER_NO_LAYER) + continue; + + if (layer_hdr_id[meta->layer] == (uint32_t)-1) { + layer_hdr_id[meta->layer] = meta->hdr_id; + continue; + } + + if (layer_hdr_id[meta->layer] != meta->hdr_id) { + bf_warn( + "rule %u has incompatible matchers on the same layer, rule will be disabled", + rule->index); + return 1; + } + } + + return 0; +} + int _bf_chain_check_rule(struct bf_chain *chain, struct bf_rule *rule) { int r; @@ -70,6 +117,9 @@ int _bf_chain_check_rule(struct bf_chain *chain, struct bf_rule *rule) rule->disabled = r; + if (!rule->disabled) + rule->disabled = _bf_rule_has_incompatible_matchers(rule); + if (rule->log && !rule->disabled) chain->flags |= BF_FLAG(BF_CHAIN_LOG); diff --git a/tests/e2e/CMakeLists.txt b/tests/e2e/CMakeLists.txt index 12ac7909..524a8258 100644 --- a/tests/e2e/CMakeLists.txt +++ b/tests/e2e/CMakeLists.txt @@ -107,6 +107,7 @@ bf_add_e2e_test(e2e matchers/udp_sport.sh) bf_add_e2e_test(e2e rules/action_order.sh ROOT) bf_add_e2e_test(e2e rules/icmp_tc.sh ROOT) bf_add_e2e_test(e2e rules/icmp_xdp.sh ROOT) +bf_add_e2e_test(e2e rules/incompatible_matchers.sh ROOT) bf_add_e2e_test(e2e rules/log.sh ROOT) bf_add_e2e_test(e2e rules/mark.sh ROOT) bf_add_e2e_test(e2e rules/redirect.sh ROOT) diff --git a/tests/e2e/rules/incompatible_matchers.sh b/tests/e2e/rules/incompatible_matchers.sh new file mode 100755 index 00000000..3cbc649e --- /dev/null +++ b/tests/e2e/rules/incompatible_matchers.sh @@ -0,0 +1,26 @@ +#!/usr/bin/env bash + +# Rules with matchers on the same layer but different protocols (e.g. ip4 + ip6) +# should be disabled: the chain loads successfully but the rule has no effect. + +. "$(dirname "$0")"/../e2e_test_util.sh + +make_sandbox +start_bpfilter + +# Baseline: ping works before any chain is loaded. +ping -c 1 -W 0.1 ${NS_IP_ADDR} + +# L3 conflict: ip4.saddr + ip6.daddr in the same rule. The rule can never match +# a packet (IPv4 and IPv6 are mutually exclusive), so it should be disabled. +${FROM_NS} bfcli chain set --from-str "chain xdp BF_HOOK_XDP{ifindex=${NS_IFINDEX}} ACCEPT rule ip4.saddr eq ${HOST_IP_ADDR} ip6.daddr eq ::1 counter DROP" + +# Ping must still succeed: the DROP rule is disabled, default policy is ACCEPT. +ping -c 1 -W 0.1 ${NS_IP_ADDR} + +# L4 conflict: tcp.sport + udp.dport in the same rule. +${FROM_NS} bfcli chain set --from-str "chain xdp BF_HOOK_XDP{ifindex=${NS_IFINDEX}} ACCEPT rule tcp.sport eq 80 udp.dport eq 53 counter DROP" + +ping -c 1 -W 0.1 ${NS_IP_ADDR} + +stop_bpfilter diff --git a/tests/unit/libbpfilter/chain.c b/tests/unit/libbpfilter/chain.c index 5a05000e..83f130f8 100644 --- a/tests/unit/libbpfilter/chain.c +++ b/tests/unit/libbpfilter/chain.c @@ -141,6 +141,137 @@ static void mixed_enabled_disabled_log_flag(void **state) assert_int_equal(chain->flags & BF_FLAG(BF_CHAIN_LOG), 0); } +static void incompatible_matchers_disable_rule(void **state) +{ + (void)state; + + // L3 conflict: IPv4 + IPv6 matchers. + { + _free_bf_chain_ struct bf_chain *chain = NULL; + _clean_bf_list_ bf_list rules = + bf_list_default(bf_rule_free, bf_rule_pack); + struct bf_rule *rule = NULL; + uint32_t ip4_addr = 0; + uint8_t ip6_addr[16] = {}; + + assert_ok(bf_rule_new(&rule)); + assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_IP4_SADDR, BF_MATCHER_EQ, + &ip4_addr, sizeof(ip4_addr))); + assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_IP6_DADDR, BF_MATCHER_EQ, + ip6_addr, sizeof(ip6_addr))); + assert_ok(bf_list_add_tail(&rules, rule)); + + assert_ok(bf_chain_new(&chain, "test", BF_HOOK_TC_EGRESS, + BF_VERDICT_ACCEPT, NULL, &rules)); + assert_true(rule->disabled); + } + + // L4 conflict: TCP + UDP matchers. + { + _free_bf_chain_ struct bf_chain *chain = NULL; + _clean_bf_list_ bf_list rules = + bf_list_default(bf_rule_free, bf_rule_pack); + struct bf_rule *rule = NULL; + uint16_t port = 0; + + assert_ok(bf_rule_new(&rule)); + assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_TCP_SPORT, BF_MATCHER_EQ, + &port, sizeof(port))); + assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_UDP_DPORT, BF_MATCHER_EQ, + &port, sizeof(port))); + assert_ok(bf_list_add_tail(&rules, rule)); + + assert_ok(bf_chain_new(&chain, "test", BF_HOOK_TC_EGRESS, + BF_VERDICT_ACCEPT, NULL, &rules)); + assert_true(rule->disabled); + } + + // L4 conflict: ICMP + ICMPv6 matchers. + { + _free_bf_chain_ struct bf_chain *chain = NULL; + _clean_bf_list_ bf_list rules = + bf_list_default(bf_rule_free, bf_rule_pack); + struct bf_rule *rule = NULL; + uint8_t type_val = 0; + + assert_ok(bf_rule_new(&rule)); + assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_ICMP_TYPE, BF_MATCHER_EQ, + &type_val, sizeof(type_val))); + assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_ICMPV6_TYPE, + BF_MATCHER_EQ, &type_val, + sizeof(type_val))); + assert_ok(bf_list_add_tail(&rules, rule)); + + assert_ok(bf_chain_new(&chain, "test", BF_HOOK_TC_EGRESS, + BF_VERDICT_ACCEPT, NULL, &rules)); + assert_true(rule->disabled); + } + + // No conflict: same protocol at same layer (TCP sport + TCP dport). + { + _free_bf_chain_ struct bf_chain *chain = NULL; + _clean_bf_list_ bf_list rules = + bf_list_default(bf_rule_free, bf_rule_pack); + struct bf_rule *rule = NULL; + uint16_t port = 0; + + assert_ok(bf_rule_new(&rule)); + assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_TCP_SPORT, BF_MATCHER_EQ, + &port, sizeof(port))); + assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_TCP_DPORT, BF_MATCHER_EQ, + &port, sizeof(port))); + assert_ok(bf_list_add_tail(&rules, rule)); + + assert_ok(bf_chain_new(&chain, "test", BF_HOOK_TC_EGRESS, + BF_VERDICT_ACCEPT, NULL, &rules)); + assert_false(rule->disabled); + } + + // No conflict: different layers (IPv4 + TCP). + { + _free_bf_chain_ struct bf_chain *chain = NULL; + _clean_bf_list_ bf_list rules = + bf_list_default(bf_rule_free, bf_rule_pack); + struct bf_rule *rule = NULL; + uint32_t ip4_addr = 0; + uint16_t port = 0; + + assert_ok(bf_rule_new(&rule)); + assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_IP4_SADDR, BF_MATCHER_EQ, + &ip4_addr, sizeof(ip4_addr))); + assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_TCP_SPORT, BF_MATCHER_EQ, + &port, sizeof(port))); + assert_ok(bf_list_add_tail(&rules, rule)); + + assert_ok(bf_chain_new(&chain, "test", BF_HOOK_TC_EGRESS, + BF_VERDICT_ACCEPT, NULL, &rules)); + assert_false(rule->disabled); + } + + // Disabled rule does not set BF_CHAIN_LOG. + { + _free_bf_chain_ struct bf_chain *chain = NULL; + _clean_bf_list_ bf_list rules = + bf_list_default(bf_rule_free, bf_rule_pack); + struct bf_rule *rule = NULL; + uint32_t ip4_addr = 0; + uint8_t ip6_addr[16] = {}; + + assert_ok(bf_rule_new(&rule)); + rule->log = 1; + assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_IP4_SADDR, BF_MATCHER_EQ, + &ip4_addr, sizeof(ip4_addr))); + assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_IP6_DADDR, BF_MATCHER_EQ, + ip6_addr, sizeof(ip6_addr))); + assert_ok(bf_list_add_tail(&rules, rule)); + + assert_ok(bf_chain_new(&chain, "test", BF_HOOK_TC_EGRESS, + BF_VERDICT_ACCEPT, NULL, &rules)); + assert_true(rule->disabled); + assert_int_equal(chain->flags & BF_FLAG(BF_CHAIN_LOG), 0); + } +} + static void get_set_by_name(void **state) { _free_bf_chain_ struct bf_chain *chain = bft_chain_dummy(true); @@ -159,6 +290,7 @@ int main(void) cmocka_unit_test(dump), cmocka_unit_test(get_set_from_matcher), cmocka_unit_test(mixed_enabled_disabled_log_flag), + cmocka_unit_test(incompatible_matchers_disable_rule), cmocka_unit_test(get_set_by_name), };