git: 9f146a81d2b3 - main - dummymbuf: Validate syntax upon write to net.dummymbuf.rules sysctl

The branch main has been updated by igoro:

URL: https://cgit.FreeBSD.org/src/commit/?id=9f146a81d2b33cfed0d5a494e798100c4e4f2a72

commit 9f146a81d2b33cfed0d5a494e798100c4e4f2a72
Author:     Igor Ostapenko <igoro@FreeBSD.org>
AuthorDate: 2024-10-05 08:04:08 +0000
Commit:     Igor Ostapenko <igoro@FreeBSD.org>
CommitDate: 2024-10-05 08:04:08 +0000

    dummymbuf: Validate syntax upon write to net.dummymbuf.rules sysctl
    
    For now, opargs are not validated due to their runtime nature.
    
    Reviewed by:    kp
    Approved by:    kp (mentor)
    Differential Revision:  https://reviews.freebsd.org/D46496
---
 sys/net/dummymbuf.c | 61 ++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 49 insertions(+), 12 deletions(-)

diff --git a/sys/net/dummymbuf.c b/sys/net/dummymbuf.c
index d4ba00b13235..99b6e6944752 100644
--- a/sys/net/dummymbuf.c
+++ b/sys/net/dummymbuf.c
@@ -40,6 +40,8 @@
 #include <net/vnet.h>
 #include <net/pfil.h>
 
+static int validate_rules(const char *);
+
 /*
  * Separate sysctl sub-tree
  */
@@ -65,6 +67,7 @@ VNET_DEFINE_STATIC(struct sx,	dmb_rules_lock);
 #define DMB_RULES_SUNLOCK()	sx_sunlock(&V_dmb_rules_lock)
 #define DMB_RULES_XLOCK()	sx_xlock(&V_dmb_rules_lock)
 #define DMB_RULES_XUNLOCK()	sx_xunlock(&V_dmb_rules_lock)
+#define DMB_RULES_LOCK_ASSERT()	sx_assert(&V_dmb_rules_lock, SA_LOCKED)
 
 static int
 dmb_sysctl_handle_rules(SYSCTL_HANDLER_ARGS)
@@ -86,12 +89,14 @@ dmb_sysctl_handle_rules(SYSCTL_HANDLER_ARGS)
 	} else {
 		/* read and write */
 		DMB_RULES_XLOCK();
-		if (*rulesp == NULL) {
-			*rulesp = malloc(arg2, M_DUMMYMBUF_RULES,
-			    M_WAITOK | M_ZERO);
-		}
-		arg1 = *rulesp;
+		arg1 = malloc(arg2, M_DUMMYMBUF_RULES, M_WAITOK | M_ZERO);
 		error = sysctl_handle_string(oidp, arg1, arg2, req);
+		if (error == 0 && (error = validate_rules(arg1)) == 0) {
+			free(*rulesp, M_DUMMYMBUF_RULES);
+			*rulesp = arg1;
+			arg1 = NULL;
+		}
+		free(arg1, M_DUMMYMBUF_RULES);
 		DMB_RULES_XUNLOCK();
 	}
 
@@ -135,7 +140,13 @@ VNET_DEFINE_STATIC(pfil_hook_t,		dmb_pfil_ethernet_hook);
  * Logging
  */
 
-#define FEEDBACK(pfil_type, pfil_flags, ifp, rule, msg)			\
+#define FEEDBACK_RULE(rule, msg)					\
+	printf("dummymbuf: %s: %.*s\n",					\
+	    (msg),							\
+	    (rule).syntax_len, (rule).syntax_begin			\
+	)
+
+#define FEEDBACK_PFIL(pfil_type, pfil_flags, ifp, rule, msg)		\
 	printf("dummymbuf: %s %b %s: %s: %.*s\n",			\
 	    (pfil_type == PFIL_TYPE_IP4 ?	"PFIL_TYPE_IP4" :	\
 	     pfil_type == PFIL_TYPE_IP6 ?	"PFIL_TYPE_IP6" :	\
@@ -198,7 +209,7 @@ bad:
 }
 
 static bool
-read_rule(const char **cur, struct rule *rule)
+read_rule(const char **cur, struct rule *rule, bool *eof)
 {
 	// {inet | inet6 | ethernet} {in | out} <ifname> <opname>[ <opargs>];
 
@@ -276,11 +287,37 @@ read_rule(const char **cur, struct rule *rule)
 		return (false);
 	rule->opargs = *cur;
 
+	// the next rule & eof
 	*cur = delim + 1;
+	while (**cur == ' ')
+		(*cur)++;
+	*eof = strlen(*cur) == 0;
 
 	return (true);
 }
 
+static int
+validate_rules(const char *rules)
+{
+	const char *cursor = rules;
+	bool parsed;
+	struct rule rule;
+	bool eof = false;
+
+	DMB_RULES_LOCK_ASSERT();
+
+	while (!eof && (parsed = read_rule(&cursor, &rule, &eof))) {
+		/* noop */
+	}
+
+	if (!parsed) {
+		FEEDBACK_RULE(rule, "rule parsing failed");
+		return (EINVAL);
+	}
+
+	return (0);
+}
+
 static pfil_return_t
 dmb_pfil_mbuf_chk(int pfil_type, struct mbuf **mp, struct ifnet *ifp,
     int flags, void *ruleset, void *unused)
@@ -289,26 +326,26 @@ dmb_pfil_mbuf_chk(int pfil_type, struct mbuf **mp, struct ifnet *ifp,
 	const char *cursor;
 	bool parsed;
 	struct rule rule;
+	bool eof = false;
 
 	DMB_RULES_SLOCK();
 	cursor = V_dmb_rules;
-	while ((parsed = read_rule(&cursor, &rule))) {
+	while (!eof && (parsed = read_rule(&cursor, &rule, &eof))) {
 		if (rule.pfil_type == pfil_type &&
 		    rule.pfil_dir == (flags & rule.pfil_dir)  &&
 		    strcmp(rule.ifname, ifp->if_xname) == 0) {
 			m = rule.op(m, &rule);
 			if (m == NULL) {
-				FEEDBACK(pfil_type, flags, ifp, rule,
+				FEEDBACK_PFIL(pfil_type, flags, ifp, rule,
 				    "mbuf operation failed");
 				break;
 			}
 			counter_u64_add(V_dmb_hits, 1);
 		}
-		if (strlen(cursor) == 0)
-			break;
 	}
 	if (!parsed) {
-		FEEDBACK(pfil_type, flags, ifp, rule, "rule parsing failed");
+		FEEDBACK_PFIL(pfil_type, flags, ifp, rule,
+		    "rule parsing failed");
 		m_freem(m);
 		m = NULL;
 	}