sd-device: bound the tag filter BPF program size

The tag-match loop emits 6 instructions per tag into a fixed 512-entry
stack array without a bounds check (unlike the subsystem loop below),
so enough tags overflow the stack. Refuse with -E2BIG instead.

Follow-up for b1c097af8df58a94cba031a347061b7cb9b62d9b

diff --git a/src/libsystemd/sd-device/device-monitor.c b/src/libsystemd/sd-device/device-monitor.c
index 962329a865ebd90af7259f1adb1e344e3a935d79..7d9433920ec35126ae888973f9669648d365b275 100644 (file)
--- a/src/libsystemd/sd-device/device-monitor.c
+++ b/src/libsystemd/sd-device/device-monitor.c
@@ -781,34 +781,57 @@ int device_monitor_send(
         return count;
 }
 
-static void bpf_stmt(struct sock_filter *ins, unsigned *i,
-                     unsigned short code, unsigned data) {
+static int bpf_stmt_impl(struct sock_filter *ins, size_t *i, size_t n_ins,
+                         unsigned short code, unsigned data) {
+        assert(ins);
         assert(i);
 
+        if (*i >= n_ins)
+                return -E2BIG;
+
         ins[(*i)++] = (struct sock_filter) {
                 .code = code,
                 .k = data,
         };
+        return 0;
 }
 
-static void bpf_jmp(struct sock_filter *ins, unsigned *i,
-                    unsigned short code, unsigned data,
-                    unsigned short jt, unsigned short jf) {
+#define bpf_stmt(ins, i, code, data) \
+        bpf_stmt_impl((ins), (i), ELEMENTSOF(ins), (code), (data))
+
+static int bpf_jmp_impl(struct sock_filter *ins, size_t *i, size_t n_ins,
+                        unsigned short code, unsigned data,
+                        unsigned jt, unsigned jf) {
+        assert(ins);
         assert(i);
 
+        if (*i >= n_ins)
+                return -E2BIG;
+
+        /* The jump offsets are stored in single bytes (struct sock_filter.jt/.jf are __u8). A larger
+         * offset would be silently truncated and make the filter branch to the wrong instruction, i.e.
+         * drop events that should match. */
+        if (jt > UINT8_MAX || jf > UINT8_MAX)
+                return -E2BIG;
+
         ins[(*i)++] = (struct sock_filter) {
                 .code = code,
                 .jt = jt,
                 .jf = jf,
                 .k = data,
         };
+        return 0;
 }
 
+#define bpf_jmp(ins, i, code, data, jt, jf) \
+        bpf_jmp_impl((ins), (i), ELEMENTSOF(ins), (code), (data), (jt), (jf))
+
 _public_ int sd_device_monitor_filter_update(sd_device_monitor *m) {
         struct sock_filter ins[512] = {};
         struct sock_fprog filter;
         const char *subsystem, *devtype, *tag;
-        unsigned i = 0;
+        size_t i = 0;
+        int r;
 
         assert_return(m, -EINVAL);
 
@@ -823,11 +846,11 @@ _public_ int sd_device_monitor_filter_update(sd_device_monitor *m) {
         }
 
         /* load magic in A */
-        bpf_stmt(ins, &i, BPF_LD|BPF_W|BPF_ABS, offsetof(monitor_netlink_header, magic));
+        r = bpf_stmt(ins, &i, BPF_LD|BPF_W|BPF_ABS, offsetof(monitor_netlink_header, magic));
         /* jump if magic matches */
-        bpf_jmp(ins, &i, BPF_JMP|BPF_JEQ|BPF_K, UDEV_MONITOR_MAGIC, 1, 0);
+        RET_GATHER(r, bpf_jmp(ins, &i, BPF_JMP|BPF_JEQ|BPF_K, UDEV_MONITOR_MAGIC, 1, 0));
         /* wrong magic, pass packet */
-        bpf_stmt(ins, &i, BPF_RET|BPF_K, 0xffffffff);
+        RET_GATHER(r, bpf_stmt(ins, &i, BPF_RET|BPF_K, 0xffffffff));
 
         if (!set_isempty(m->tag_filter)) {
                 int tag_matches = set_size(m->tag_filter);
@@ -839,23 +862,23 @@ _public_ int sd_device_monitor_filter_update(sd_device_monitor *m) {
                         uint32_t tag_bloom_lo = tag_bloom_bits & 0xffffffff;
 
                         /* load device bloom bits in A */
-                        bpf_stmt(ins, &i, BPF_LD|BPF_W|BPF_ABS, offsetof(monitor_netlink_header, filter_tag_bloom_hi));
+                        RET_GATHER(r, bpf_stmt(ins, &i, BPF_LD|BPF_W|BPF_ABS, offsetof(monitor_netlink_header, filter_tag_bloom_hi)));
                         /* clear bits (tag bits & bloom bits) */
-                        bpf_stmt(ins, &i, BPF_ALU|BPF_AND|BPF_K, tag_bloom_hi);
+                        RET_GATHER(r, bpf_stmt(ins, &i, BPF_ALU|BPF_AND|BPF_K, tag_bloom_hi));
                         /* jump to next tag if it does not match */
-                        bpf_jmp(ins, &i, BPF_JMP|BPF_JEQ|BPF_K, tag_bloom_hi, 0, 3);
+                        RET_GATHER(r, bpf_jmp(ins, &i, BPF_JMP|BPF_JEQ|BPF_K, tag_bloom_hi, 0, 3));
 
                         /* load device bloom bits in A */
-                        bpf_stmt(ins, &i, BPF_LD|BPF_W|BPF_ABS, offsetof(monitor_netlink_header, filter_tag_bloom_lo));
+                        RET_GATHER(r, bpf_stmt(ins, &i, BPF_LD|BPF_W|BPF_ABS, offsetof(monitor_netlink_header, filter_tag_bloom_lo)));
                         /* clear bits (tag bits & bloom bits) */
-                        bpf_stmt(ins, &i, BPF_ALU|BPF_AND|BPF_K, tag_bloom_lo);
+                        RET_GATHER(r, bpf_stmt(ins, &i, BPF_ALU|BPF_AND|BPF_K, tag_bloom_lo));
                         /* jump behind end of tag match block if tag matches */
                         tag_matches--;
-                        bpf_jmp(ins, &i, BPF_JMP|BPF_JEQ|BPF_K, tag_bloom_lo, 1 + (tag_matches * 6), 0);
+                        RET_GATHER(r, bpf_jmp(ins, &i, BPF_JMP|BPF_JEQ|BPF_K, tag_bloom_lo, 1 + (tag_matches * 6), 0));
                 }
 
                 /* nothing matched, drop packet */
-                bpf_stmt(ins, &i, BPF_RET|BPF_K, 0);
+                RET_GATHER(r, bpf_stmt(ins, &i, BPF_RET|BPF_K, 0));
         }
 
         /* add all subsystem matches */
@@ -864,33 +887,33 @@ _public_ int sd_device_monitor_filter_update(sd_device_monitor *m) {
                         uint32_t hash = string_hash32(subsystem);
 
                         /* load device subsystem value in A */
-                        bpf_stmt(ins, &i, BPF_LD|BPF_W|BPF_ABS, offsetof(monitor_netlink_header, filter_subsystem_hash));
+                        RET_GATHER(r, bpf_stmt(ins, &i, BPF_LD|BPF_W|BPF_ABS, offsetof(monitor_netlink_header, filter_subsystem_hash)));
                         if (!devtype) {
                                 /* jump if subsystem does not match */
-                                bpf_jmp(ins, &i, BPF_JMP|BPF_JEQ|BPF_K, hash, 0, 1);
+                                RET_GATHER(r, bpf_jmp(ins, &i, BPF_JMP|BPF_JEQ|BPF_K, hash, 0, 1));
                         } else {
                                 /* jump if subsystem does not match */
-                                bpf_jmp(ins, &i, BPF_JMP|BPF_JEQ|BPF_K, hash, 0, 3);
+                                RET_GATHER(r, bpf_jmp(ins, &i, BPF_JMP|BPF_JEQ|BPF_K, hash, 0, 3));
                                 /* load device devtype value in A */
-                                bpf_stmt(ins, &i, BPF_LD|BPF_W|BPF_ABS, offsetof(monitor_netlink_header, filter_devtype_hash));
+                                RET_GATHER(r, bpf_stmt(ins, &i, BPF_LD|BPF_W|BPF_ABS, offsetof(monitor_netlink_header, filter_devtype_hash)));
                                 /* jump if value does not match */
                                 hash = string_hash32(devtype);
-                                bpf_jmp(ins, &i, BPF_JMP|BPF_JEQ|BPF_K, hash, 0, 1);
+                                RET_GATHER(r, bpf_jmp(ins, &i, BPF_JMP|BPF_JEQ|BPF_K, hash, 0, 1));
                         }
 
                         /* matched, pass packet */
-                        bpf_stmt(ins, &i, BPF_RET|BPF_K, 0xffffffff);
-
-                        if (i+1 >= ELEMENTSOF(ins))
-                                return -E2BIG;
+                        RET_GATHER(r, bpf_stmt(ins, &i, BPF_RET|BPF_K, 0xffffffff));
                 }
 
                 /* nothing matched, drop packet */
-                bpf_stmt(ins, &i, BPF_RET|BPF_K, 0);
+                RET_GATHER(r, bpf_stmt(ins, &i, BPF_RET|BPF_K, 0));
         }
 
         /* matched, pass packet */
-        bpf_stmt(ins, &i, BPF_RET|BPF_K, 0xffffffff);
+        RET_GATHER(r, bpf_stmt(ins, &i, BPF_RET|BPF_K, 0xffffffff));
+
+        if (r < 0)
+                return r;
 
         /* install filter */
         filter = (struct sock_fprog) {

diff --git a/src/libsystemd/sd-device/test-sd-device-monitor.c b/src/libsystemd/sd-device/test-sd-device-monitor.c
index 8d88f3eb6727a1bc499c50f0a9f80803727a7433..523cc7c6cd19228aed5b8ff0e78772fb8a574955 100644 (file)
--- a/src/libsystemd/sd-device/test-sd-device-monitor.c
+++ b/src/libsystemd/sd-device/test-sd-device-monitor.c
@@ -13,6 +13,7 @@
 #include "path-util.h"
 #include "socket-util.h"
 #include "stat-util.h"
+#include "stdio-util.h"
 #include "string-util.h"
 #include "tests.h"
 #include "time-util.h"
@@ -282,6 +283,21 @@ TEST(sd_device_monitor_filter_add_match_tag) {
         ASSERT_EQ(sd_event_loop(sd_device_monitor_get_event(monitor_client)), 100);
 }
 
+TEST(sd_device_monitor_filter_update_bounds) {
+        _cleanup_(sd_device_monitor_unrefp) sd_device_monitor *m = NULL;
+
+        ASSERT_OK(device_monitor_new_full(&m, MONITOR_GROUP_NONE, -EBADF));
+
+        /* Each tag emits 6 BPF instructions into a fixed 512-entry array, too many must be refused */
+        for (unsigned u = 0; u < 200; u++) {
+                char t[32];
+                xsprintf(t, "tag%u", u);
+                ASSERT_OK(sd_device_monitor_filter_add_match_tag(m, t));
+        }
+
+        ASSERT_ERROR(sd_device_monitor_filter_update(m), E2BIG);
+}
+
 TEST(sd_device_monitor_filter_add_match_sysattr) {
         _cleanup_(sd_device_monitor_unrefp) sd_device_monitor *monitor_server = NULL, *monitor_client = NULL;
         _cleanup_(sd_device_enumerator_unrefp) sd_device_enumerator *e = NULL;