From: Victor Julien <victor@inliniac.net>
Date: Tue, 14 Sep 2021 08:35:18 +0000 (+0200)
Subject: detect: track prefilter by progress, not engine
X-Git-Tag: suricata-6.0.4~48
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d5e80ca45a4470f3554c908dfccbd35d2f44a3a8;p=thirdparty%2Fsuricata.git

detect: track prefilter by progress, not engine

Fix FNs in case of too many prefilter engines. A transaction was tracking
which engines have run using a u64 bit array. The engines 'local_id' was
used to set and check this bit. However the bit checking code didn't
handle int types correctly, leading to an incorrect left shift result of
a u32 to a u64 bit value.

This commit addresses that by fixing the int handling, but also by
changing how the engines are tracked.

To avoid wasting prefilter engine tracking bit space, track what
ran by the progress they are registered at, instead of the individual
engine id's. While we can have many engines, the protocols use far
fewer unique progress values. So instead of tracking for dozens of
prefilter id's, we track for the handful of progress values.

To allow for this the engine array is sorted by tx_min_progress, then
app_proto and finally local_id. A new field is added to "know" when
the last relevant engine for a progress value is reached, so that we
can set the prefilter bit then.

A consquence is that the progress values have a ceiling now that
needs to fit in a 64 bit bitarray. The values used by parsers currently
does not exceed 5, so that seems to be ok.

Bug: #4685.
(cherry picked from commit 932cf0b6a6ad1d34fffe8dd92c14b5bc32c9f6fe)
---

diff --git a/src/detect-engine-prefilter.c b/src/detect-engine-prefilter.c
index f0f3e68b7b..11cb3c878f 100644
--- a/src/detect-engine-prefilter.c
+++ b/src/detect-engine-prefilter.c
@@ -54,6 +54,7 @@
 #include "app-layer-htp.h"
 
 #include "util-profiling.h"
+#include "util-validate.h"
 
 static int PrefilterStoreGetId(DetectEngineCtx *de_ctx,
         const char *name, void (*FreeFunc)(void *));
@@ -99,7 +100,8 @@ void DetectRunPrefilterTx(DetectEngineThreadCtx *det_ctx,
     /* reset rule store */
     det_ctx->pmq.rule_id_array_cnt = 0;
 
-    SCLogDebug("tx %p progress %d", tx->tx_ptr, tx->tx_progress);
+    SCLogDebug("packet %" PRIu64 " tx %p progress %d tx->prefilter_flags %" PRIx64, p->pcap_cnt,
+            tx->tx_ptr, tx->tx_progress, tx->prefilter_flags);
 
     PrefilterEngine *engine = sgh->tx_engines;
     do {
@@ -108,7 +110,7 @@ void DetectRunPrefilterTx(DetectEngineThreadCtx *det_ctx,
         if (engine->tx_min_progress > tx->tx_progress)
             goto next;
         if (tx->tx_progress > engine->tx_min_progress) {
-            if (tx->prefilter_flags & (1<<(engine->local_id))) {
+            if (tx->prefilter_flags & BIT_U64(engine->tx_min_progress)) {
                 goto next;
             }
         }
@@ -118,8 +120,8 @@ void DetectRunPrefilterTx(DetectEngineThreadCtx *det_ctx,
                 p, p->flow, tx->tx_ptr, tx->tx_id, flow_flags);
         PREFILTER_PROFILING_END(det_ctx, engine->gid);
 
-        if (tx->tx_progress > engine->tx_min_progress) {
-            tx->prefilter_flags |= (1<<(engine->local_id));
+        if (tx->tx_progress > engine->tx_min_progress && engine->is_last_for_progress) {
+            tx->prefilter_flags |= BIT_U64(engine->tx_min_progress);
         }
     next:
         if (engine->is_last)
@@ -345,6 +347,21 @@ void PrefilterCleanupRuleGroup(const DetectEngineCtx *de_ctx, SigGroupHead *sgh)
     }
 }
 
+static int PrefilterSetupRuleGroupSortHelper(const void *a, const void *b)
+{
+    const PrefilterEngine *s0 = a;
+    const PrefilterEngine *s1 = b;
+    if (s1->tx_min_progress == s0->tx_min_progress) {
+        if (s1->alproto == s0->alproto) {
+            return s0->local_id > s1->local_id ? 1 : -1;
+        } else {
+            return s0->alproto > s1->alproto ? 1 : -1;
+        }
+    } else {
+        return s0->tx_min_progress > s1->tx_min_progress ? 1 : -1;
+    }
+}
+
 void PrefilterSetupRuleGroup(DetectEngineCtx *de_ctx, SigGroupHead *sgh)
 {
     int r = PatternMatchPrepareGroup(de_ctx, sgh);
@@ -441,12 +458,60 @@ void PrefilterSetupRuleGroup(DetectEngineCtx *de_ctx, SigGroupHead *sgh)
             e->pectx = el->pectx;
             el->pectx = NULL; // e now owns the ctx
             e->gid = el->gid;
-            if (el->next == NULL) {
-                e->is_last = TRUE;
-            }
             e++;
         }
-        SCLogDebug("sgh %p max local_id %u", sgh, local_id);
+
+        /* sort by tx_min_progress, then alproto, then local_id */
+        qsort(sgh->tx_engines, local_id, sizeof(PrefilterEngine),
+                PrefilterSetupRuleGroupSortHelper);
+        sgh->tx_engines[local_id - 1].is_last = true;
+        sgh->tx_engines[local_id - 1].is_last_for_progress = true;
+
+        PrefilterEngine *engine;
+
+        /* per alproto to set is_last_for_progress per alproto because the inspect
+         * loop skips over engines that are not the correct alproto */
+        for (AppProto a = 1; a < ALPROTO_FAILED; a++) {
+            int last_tx_progress = 0;
+            bool last_tx_progress_set = false;
+            PrefilterEngine *prev_engine = NULL;
+            engine = sgh->tx_engines;
+            do {
+                BUG_ON(engine->tx_min_progress < last_tx_progress);
+                if (engine->alproto == a) {
+                    if (last_tx_progress_set && engine->tx_min_progress > last_tx_progress) {
+                        if (prev_engine) {
+                            prev_engine->is_last_for_progress = true;
+                        }
+                    }
+
+                    last_tx_progress_set = true;
+                    prev_engine = engine;
+                } else {
+                    if (prev_engine) {
+                        prev_engine->is_last_for_progress = true;
+                    }
+                }
+                last_tx_progress = engine->tx_min_progress;
+                if (engine->is_last)
+                    break;
+                engine++;
+            } while (1);
+        }
+#ifdef DEBUG
+        SCLogDebug("sgh %p", sgh);
+        engine = sgh->tx_engines;
+        do {
+            SCLogDebug("engine: gid %u alproto %s tx_min_progress %d is_last %s "
+                       "is_last_for_progress %s",
+                    engine->gid, AppProtoToString(engine->alproto), engine->tx_min_progress,
+                    engine->is_last ? "true" : "false",
+                    engine->is_last_for_progress ? "true" : "false");
+            if (engine->is_last)
+                break;
+            engine++;
+        } while (1);
+#endif
     }
 }
 
diff --git a/src/detect.c b/src/detect.c
index b28437e69e..3a5c7c6a63 100644
--- a/src/detect.c
+++ b/src/detect.c
@@ -1106,8 +1106,16 @@ static bool DetectRunTxInspectRule(ThreadVars *tv,
             }
             if (engine->mpm) {
                 if (tx->tx_progress > engine->progress) {
+                    TRACE_SID_TXS(s->id, tx,
+                            "engine->mpm: t->tx_progress %u > engine->progress %u, so set "
+                            "mpm_before_progress",
+                            tx->tx_progress, engine->progress);
                     mpm_before_progress = true;
                 } else if (tx->tx_progress == engine->progress) {
+                    TRACE_SID_TXS(s->id, tx,
+                            "engine->mpm: t->tx_progress %u == engine->progress %u, so set "
+                            "mpm_in_progress",
+                            tx->tx_progress, engine->progress);
                     mpm_in_progress = true;
                 }
             }
diff --git a/src/detect.h b/src/detect.h
index 3da11220ba..ed5daa3861 100644
--- a/src/detect.h
+++ b/src/detect.h
@@ -1327,7 +1327,8 @@ typedef struct PrefilterEngine_ {
 
     /* global id for this prefilter */
     uint32_t gid;
-    int is_last;
+    bool is_last;
+    bool is_last_for_progress;
 } PrefilterEngine;
 
 typedef struct SigGroupHeadInitData_ {