[PATCH proxmox-ebpf v3 03/45] policy: add a check for non-first fragments

[Hannes Laimer <h.laimer@proxmox.com>]

A non-first fragment has no transport header, so no check matched it,
and a pair allowed only through narrowed rules dropped every later
fragment. Add a check that matches non-first fragments of a specific
(or any) protocol.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/policy/bpf/policy.bpf.c | 26 +++++++++++----
 src/policy/bpf/types.h      |  3 ++
 src/policy/mod.rs           | 63 ++++++++++++++++++++++---------------
 src/policy/types.rs         |  4 +++
 tests/policy.rs             | 62 ++++++++++++++++++++++++++++++++++++
 5 files changed, 127 insertions(+), 31 deletions(-)

diff --git a/src/policy/bpf/policy.bpf.c b/src/policy/bpf/policy.bpf.c
index 53acd35cc6df..fcf513bb13de 100644
--- a/src/policy/bpf/policy.bpf.c
+++ b/src/policy/bpf/policy.bpf.c
@@ -82,7 +82,10 @@ int tc_tap_ingress(struct __sk_buff *skb) {
     return TC_ACT_UNSPEC;
 }
 
-// What a check matches on. `matchable` is set once the parse reached the L4 header.
+// What a check matches on. `matchable` is set once the parse reached the L4 header. `fragment`
+// marks a non-first fragment, which has a protocol and no L4 header. `unreadable` marks a frame
+// the parse could not read to its L4 header, such as one cut short or behind a second tag in the
+// frame data.
 struct frame {
     __u16 eth_proto;
     __u16 sport;
@@ -95,6 +98,7 @@ struct frame {
     __u8 has_ports;
     __u8 has_icmp;
     __u8 unreadable;
+    __u8 fragment;
 };
 
 static __always_inline int is_ext_header(__u8 proto) {
@@ -114,7 +118,11 @@ static __always_inline int ipv6_l4(struct __sk_buff *skb, struct frame *f, __u32
                 return 0;
             }
             // the offset sits in the top 13 bits of the second halfword
-            if (hdr[2] || (hdr[3] & 0xf8)) return 0;
+            if (hdr[2] || (hdr[3] & 0xf8)) {
+                f->fragment = 1;
+                f->ip_proto = hdr[0];
+                return 0;
+            }
             *off += 8;
         } else {
             if (bpf_skb_load_bytes(skb, *off, hdr, 2) < 0) {
@@ -206,10 +214,12 @@ static __always_inline void parse_frame(struct __sk_buff *skb, struct frame *f)
             f->unreadable = 1;
             return;
         }
-        // only the first fragment carries the L4 header
-        if (bpf_ntohs(ip.frag_off) & IPV4_FRAG_OFFSET) return;
         f->family = FAMILY_V4;
         f->ip_proto = ip.protocol;
+        if (bpf_ntohs(ip.frag_off) & IPV4_FRAG_OFFSET) {
+            f->fragment = 1;
+            return;
+        }
         off += (__u32)ip.ihl * 4;
     } else if (f->eth_proto == ETHERTYPE_IPV6) {
         struct ipv6hdr ip6;
@@ -229,9 +239,13 @@ static __always_inline void parse_frame(struct __sk_buff *skb, struct frame *f)
 }
 
 static __always_inline int check_matches(const struct frame *f, const struct check_value *c) {
-    if (!f->matchable) return 0;
+    if (c->match_flags & CHECK_FRAGMENT) {
+        if (!f->fragment) return 0;
+    } else if (!f->matchable) {
+        return 0;
+    }
     if (c->family && c->family != f->family) return 0;
-    if (c->ip_proto != f->ip_proto) return 0;
+    if ((c->match_flags & CHECK_HAS_PROTO) && c->ip_proto != f->ip_proto) return 0;
 
     if (c->match_flags & CHECK_HAS_SPORT) {
         if (!f->has_ports || f->sport < c->sport_lo || f->sport > c->sport_hi) return 0;
diff --git a/src/policy/bpf/types.h b/src/policy/bpf/types.h
index a8b1df66fc41..0bfe1d3b9038 100644
--- a/src/policy/bpf/types.h
+++ b/src/policy/bpf/types.h
@@ -26,6 +26,9 @@
 #define CHECK_HAS_DPORT (1 << 1)
 #define CHECK_HAS_ICMP_TYPE (1 << 2)
 #define CHECK_HAS_ICMP_CODE (1 << 3)
+// the check is for non-first fragments, which no other check matches
+#define CHECK_FRAGMENT (1 << 4)
+#define CHECK_HAS_PROTO (1 << 5)
 
 struct nic_entry {
     __u16 id;
diff --git a/src/policy/mod.rs b/src/policy/mod.rs
index 62ee7576b6fc..e3d3e580f7f3 100644
--- a/src/policy/mod.rs
+++ b/src/policy/mod.rs
@@ -76,8 +76,11 @@ pub enum Family {
 /// left out is not matched on.
 #[derive(Clone, Debug, Default, PartialEq, Eq)]
 pub struct CheckEntry {
-    pub ip_proto: u8,
+    pub ip_proto: Option<u8>,
     pub family: Family,
+    /// Match non-first fragments instead of frames with an L4 header. A fragment has no L4 header,
+    /// so a check that names a port or an ICMP field matches nothing.
+    pub fragment: bool,
     pub sport: Option<(u16, u16)>,
     pub dport: Option<(u16, u16)>,
     pub icmp_type: Option<u8>,
@@ -88,6 +91,12 @@ pub struct CheckEntry {
 impl CheckEntry {
     fn value(&self) -> CheckValue {
         let mut match_flags = 0;
+        if self.fragment {
+            match_flags |= CHECK_FRAGMENT;
+        }
+        if self.ip_proto.is_some() {
+            match_flags |= CHECK_HAS_PROTO;
+        }
         if self.sport.is_some() {
             match_flags |= CHECK_HAS_SPORT;
         }
@@ -103,7 +112,7 @@ impl CheckEntry {
         let (sport_lo, sport_hi) = self.sport.unwrap_or_default();
         let (dport_lo, dport_hi) = self.dport.unwrap_or_default();
         CheckValue {
-            ip_proto: self.ip_proto,
+            ip_proto: self.ip_proto.unwrap_or_default(),
             family: self.family as u8,
             match_flags,
             allow: self.allow as u8,
@@ -681,13 +690,10 @@ mod tests {
 
     fn entry(port: u16) -> CheckEntry {
         CheckEntry {
-            ip_proto: 6,
-            family: Family::Any,
-            sport: None,
+            ip_proto: Some(6),
             dport: Some((port, port)),
-            icmp_type: None,
-            icmp_code: None,
             allow: true,
+            ..Default::default()
         }
     }
 
@@ -718,25 +724,19 @@ mod tests {
     #[test]
     fn check_entry_bytes() {
         let plain = CheckEntry {
-            ip_proto: 17,
-            family: Family::Any,
-            sport: None,
-            dport: None,
-            icmp_type: None,
-            icmp_code: None,
-            allow: false,
+            ip_proto: Some(17),
+            ..Default::default()
         };
-        assert_eq!(
-            bytes(plain.value()),
-            [17, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
-        );
+        let b = bytes(plain.value());
+        assert_eq!(b[..4], [17, 0, CHECK_HAS_PROTO, 0]);
+        assert_eq!(b[4..], [0; 12]);
 
         let v4 = CheckEntry {
             family: Family::V4,
             allow: true,
             ..plain.clone()
         };
-        assert_eq!(bytes(v4.value())[..4], [17, 4, 0, 1]);
+        assert_eq!(bytes(v4.value())[..4], [17, 4, CHECK_HAS_PROTO, 1]);
         let v6 = CheckEntry {
             family: Family::V6,
             ..plain.clone()
@@ -748,7 +748,7 @@ mod tests {
             ..plain.clone()
         };
         let b = bytes(sport.value());
-        assert_eq!(b[2], CHECK_HAS_SPORT);
+        assert_eq!(b[2], CHECK_HAS_PROTO | CHECK_HAS_SPORT);
         assert_eq!(b[4..6], 1000u16.to_ne_bytes());
         assert_eq!(b[6..8], 2000u16.to_ne_bytes());
         assert_eq!(b[8..], [0; 8]);
@@ -758,27 +758,40 @@ mod tests {
             ..plain.clone()
         };
         let b = bytes(dport.value());
-        assert_eq!(b[2], CHECK_HAS_DPORT);
+        assert_eq!(b[2], CHECK_HAS_PROTO | CHECK_HAS_DPORT);
         assert_eq!(b[4..8], [0; 4]);
         assert_eq!(b[8..10], 22u16.to_ne_bytes());
         assert_eq!(b[10..12], 23u16.to_ne_bytes());
 
         let icmp = CheckEntry {
-            ip_proto: 1,
+            ip_proto: Some(1),
             icmp_type: Some(3),
             icmp_code: Some(4),
             ..plain.clone()
         };
         let b = bytes(icmp.value());
-        assert_eq!(b[2], CHECK_HAS_ICMP_TYPE | CHECK_HAS_ICMP_CODE);
+        assert_eq!(
+            b[2],
+            CHECK_HAS_PROTO | CHECK_HAS_ICMP_TYPE | CHECK_HAS_ICMP_CODE
+        );
         assert_eq!(b[12..], [3, 4, 0, 0]);
 
         let code_only = CheckEntry {
-            ip_proto: 1,
+            ip_proto: Some(1),
             icmp_code: Some(0),
+            ..plain.clone()
+        };
+        assert_eq!(
+            bytes(code_only.value())[2],
+            CHECK_HAS_PROTO | CHECK_HAS_ICMP_CODE
+        );
+
+        let fragment = CheckEntry {
+            ip_proto: None,
+            fragment: true,
             ..plain
         };
-        assert_eq!(bytes(code_only.value())[2], CHECK_HAS_ICMP_CODE);
+        assert_eq!(bytes(fragment.value())[..3], [0, 0, CHECK_FRAGMENT]);
     }
 
     #[test]
diff --git a/src/policy/types.rs b/src/policy/types.rs
index e411ba4ae6f2..c8e97c177625 100644
--- a/src/policy/types.rs
+++ b/src/policy/types.rs
@@ -25,6 +25,8 @@ pub const CHECK_HAS_SPORT: u8 = 1 << 0;
 pub const CHECK_HAS_DPORT: u8 = 1 << 1;
 pub const CHECK_HAS_ICMP_TYPE: u8 = 1 << 2;
 pub const CHECK_HAS_ICMP_CODE: u8 = 1 << 3;
+pub const CHECK_FRAGMENT: u8 = 1 << 4;
+pub const CHECK_HAS_PROTO: u8 = 1 << 5;
 
 #[repr(C)]
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
@@ -151,5 +153,7 @@ mod layout {
             defined("CHECK_HAS_ICMP_CODE"),
             usize::from(CHECK_HAS_ICMP_CODE)
         );
+        assert_eq!(defined("CHECK_FRAGMENT"), usize::from(CHECK_FRAGMENT));
+        assert_eq!(defined("CHECK_HAS_PROTO"), usize::from(CHECK_HAS_PROTO));
     }
 }
diff --git a/tests/policy.rs b/tests/policy.rs
index c755c2cc5a60..5ca034da5f68 100644
--- a/tests/policy.rs
+++ b/tests/policy.rs
@@ -51,6 +51,8 @@ struct Check {
     dport: Option<(u16, u16)>,
     icmp_type: Option<u8>,
     icmp_code: Option<u8>,
+    fragment: bool,
+    any_proto: bool,
     allow: bool,
 }
 
@@ -80,6 +82,12 @@ impl Check {
             v.match_flags |= CHECK_HAS_ICMP_CODE;
             v.icmp_code = code;
         }
+        if self.fragment {
+            v.match_flags |= CHECK_FRAGMENT;
+        }
+        if !self.any_proto {
+            v.match_flags |= CHECK_HAS_PROTO;
+        }
         v
     }
 }
@@ -381,6 +389,60 @@ fn a_later_fragment_matches_nothing() {
     assert_eq!(run(&fragment), TC_ACT_UNSPEC);
 }
 
+#[test]
+fn a_fragment_check_matches_later_fragments_only() {
+    let fragments = Check {
+        fragment: true,
+        any_proto: true,
+        allow: true,
+        ..Default::default()
+    };
+    install(false, &[fragments]);
+
+    let later = ipv4(PROTO_TCP, 185, &tcp(40000, 22));
+    assert_eq!(run(&ethernet(ETHERTYPE_IPV4, 0, &later)), TC_ACT_UNSPEC);
+    let mut later = fragment_header(PROTO_UDP, 185);
+    later.extend_from_slice(&udp(40000, 53));
+    assert_eq!(
+        run(&ethernet(ETHERTYPE_IPV6, 0, &ipv6(PROTO_FRAGMENT, &later))),
+        TC_ACT_UNSPEC
+    );
+
+    // the first fragment carries the L4 header, more fragments following or not
+    let first = ipv4(PROTO_TCP, 0x2000, &tcp(40000, 22));
+    assert_eq!(run(&ethernet(ETHERTYPE_IPV4, 0, &first)), TC_ACT_SHOT);
+    assert_eq!(run(&tcp_frame(40000, 22)), TC_ACT_SHOT);
+    let mut first = fragment_header(PROTO_UDP, 0);
+    first.extend_from_slice(&udp(40000, 53));
+    assert_eq!(
+        run(&ethernet(ETHERTYPE_IPV6, 0, &ipv6(PROTO_FRAGMENT, &first))),
+        TC_ACT_SHOT
+    );
+}
+
+#[test]
+fn a_fragment_check_can_name_the_protocol() {
+    let udp_fragments = Check {
+        ip_proto: PROTO_UDP,
+        fragment: true,
+        allow: true,
+        ..Default::default()
+    };
+    install(false, &[udp_fragments]);
+
+    let later = |protocol| ethernet(ETHERTYPE_IPV4, 0, &ipv4(protocol, 185, &[0; 8]));
+    assert_eq!(run(&later(PROTO_UDP)), TC_ACT_UNSPEC);
+    assert_eq!(run(&later(PROTO_TCP)), TC_ACT_SHOT);
+
+    let later = |protocol| {
+        let mut payload = fragment_header(protocol, 185);
+        payload.extend_from_slice(&[0; 8]);
+        ethernet(ETHERTYPE_IPV6, 0, &ipv6(PROTO_FRAGMENT, &payload))
+    };
+    assert_eq!(run(&later(PROTO_UDP)), TC_ACT_UNSPEC);
+    assert_eq!(run(&later(PROTO_TCP)), TC_ACT_SHOT);
+}
+
 #[test]
 fn one_vlan_tag_is_parsed_through() {
     install(
-- 
2.47.3