From: Arran Cudbard-Bell <a.cudbardb@freeradius.org>
Date: Tue, 14 May 2024 14:57:47 +0000 (-0600)
Subject: Enable write protection on a per-module list basis
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=db7b3ff40696498eab03721948b3cd1303bcad89;p=thirdparty%2Ffreeradius-server.git

Enable write protection on a per-module list basis
---

diff --git a/src/lib/io/master.c b/src/lib/io/master.c
index e89506578ec..87729313347 100644
--- a/src/lib/io/master.c
+++ b/src/lib/io/master.c
@@ -2702,8 +2702,10 @@ static int mod_instantiate(module_inst_ctx_t const *mctx)
 	 *	Create a list of client modules.
 	 *
 	 *	FIXME - Probably only want to do this for connected sockets?
+	 *
+	 *	FIXME - We probably want write protect enabled?
 	 */
-	inst->clients = module_list_alloc(inst, &module_list_type_thread_local, "clients");
+	inst->clients = module_list_alloc(inst, &module_list_type_thread_local, "clients", false);
 	module_list_mask_set(inst->clients, MODULE_INSTANCE_BOOTSTRAPPED);
 
 	return 0;
diff --git a/src/lib/server/module.c b/src/lib/server/module.c
index c073e78b106..6e036ee0736 100644
--- a/src/lib/server/module.c
+++ b/src/lib/server/module.c
@@ -622,16 +622,14 @@ void module_list_debug(module_list_t const *ml)
 static inline CC_HINT(always_inline)
 int module_data_protect(module_instance_t *mi, module_data_pool_t *pool)
 {
-	if (pool->start == NULL) return 0; /* noop */
+	if ((pool->start == NULL) || !mi->ml->write_protect) return 0; /* noop */
 
 	DEBUG3("Protecting data %s %p-%p", mi->name, pool->start, (uint8_t *)pool->start + pool->len);
 
-#if 0
 	if (unlikely(mprotect(pool->start, pool->len, PROT_READ) < 0)) {
 		fr_strerror_printf("Protecting \"%s\" module data failed: %s", mi->name, fr_syserror(errno));
 		return -1;
 	}
-#endif
 
 	return 0;
 }
@@ -646,16 +644,14 @@ int module_data_protect(module_instance_t *mi, module_data_pool_t *pool)
 static inline CC_HINT(always_inline)
 int module_data_unprotect(module_instance_t const *mi, module_data_pool_t const *pool)
 {
-	if (pool->start == NULL) return 0; /* noop */
+	if ((pool->start == NULL) || !mi->ml->write_protect) return 0; /* noop */
 
 	DEBUG3("Unprotecting data %s %p-%p", mi->name, pool->start, (uint8_t *)pool->start + pool->len);
 
-#if 0
 	if (unlikely(mprotect(pool->start, pool->len, PROT_READ | PROT_WRITE) < 0)) {
 		fr_strerror_printf("Unprotecting \"%s\" data failed: %s", mi->name, fr_syserror(errno));
 		return -1;
 	}
-#endif
 
 	return 0;
 }
@@ -1747,14 +1743,17 @@ void module_list_mask_set(module_list_t *ml, module_instance_state_t mask)
  * If the list is freed all module instance data will be freed.
  * If no more instances of the module exist the module be unloaded.
  *
- * @param[in] ctx	To allocate the list in.
- * @param[in] type	of the list.  Controls whether this is a global
- *			module list, or a per-thread list containing
- *			variants of an existing module.
- * @param[in] name	of the list.  Used for debugging.
+ * @param[in] ctx		To allocate the list in.
+ * @param[in] type		of the list.  Controls whether this is a global
+ *				module list, or a per-thread list containing
+ *				variants of an existing module.
+ * @param[in] name		of the list.  Used for debugging.
+ * @param[in] write_protect	Whether to write protect the module data
+ *				after instantiation and bootstrapping.
  * @return A new module list.
  */
-module_list_t *module_list_alloc(TALLOC_CTX *ctx, module_list_type_t const *type, char const *name)
+module_list_t *module_list_alloc(TALLOC_CTX *ctx, module_list_type_t const *type,
+				 char const *name, bool write_protect)
 {
 	module_list_t *ml;
 
@@ -1778,6 +1777,7 @@ module_list_t *module_list_alloc(TALLOC_CTX *ctx, module_list_type_t const *type
 		talloc_free(ml);
 		return NULL;
 	}
+	ml->write_protect = write_protect;
 
 	return ml;
 }
diff --git a/src/lib/server/module.h b/src/lib/server/module.h
index 41d9ec42bb2..ed05320fdd6 100644
--- a/src/lib/server/module.h
+++ b/src/lib/server/module.h
@@ -331,8 +331,13 @@ typedef module_thread_instance_t *(*module_list_thread_data_get_t)(module_instan
 
 /** A list of modules
  *
- * This allows modules to be instantiated and freed in phases,
- * i.e. proto modules before rlm modules.
+ * This used to be a global structure, but was move to a struct.
+ *
+ * Module lists allow collections of modules to be created.  The module lists themselves can be configured
+ * to be thread-local or global, with optional runtime write protection.
+ *
+ * Thread-local module lists are used for dynamic modules, i.e. those created at runtime, where as the
+ * global module lists are used for backend modules, listeners, and process state machines.
  */
 struct module_list_s
 {
@@ -344,6 +349,11 @@ struct module_list_s
 	fr_rb_tree_t			*data_tree;		//!< Modules indexed by data.
 	fr_heap_t			*inst_heap;		//!< Heap of module instances.
 
+	bool				write_protect;		//!< If true, pages containing module boot or
+								///< instance data will be write protected after
+								///< bootstrapping and instantiation is complete,
+								///< to prevent accidental modification.
+
 	/** @name Callbacks to manage thread-specific data
 	 *
 	 * In "child" lists, which are only operating in a single thread, we don't need
@@ -491,7 +501,8 @@ bool			module_instance_skip_thread_instantiate(module_instance_t *mi);
 void			module_list_mask_set(module_list_t *ml, module_instance_state_t mask);
 /** @} */
 
-module_list_t 		*module_list_alloc(TALLOC_CTX *ctx, module_list_type_t const *type, char const *name)
+module_list_t 		*module_list_alloc(TALLOC_CTX *ctx, module_list_type_t const *type,
+					   char const *name, bool write_protect)
 					   CC_HINT(nonnull(2,3)) CC_HINT(warn_unused_result);
 
 void			modules_init(char const *lib_dir);
diff --git a/src/lib/server/module_rlm.c b/src/lib/server/module_rlm.c
index a5658c93569..ee13d73a148 100644
--- a/src/lib/server/module_rlm.c
+++ b/src/lib/server/module_rlm.c
@@ -1170,7 +1170,7 @@ static int _modules_rlm_free_atexit(UNUSED void *uctx)
  */
 int modules_rlm_init(void)
 {
-	MEM(rlm_modules = module_list_alloc(NULL, &module_list_type_global, "rlm"));
+	MEM(rlm_modules = module_list_alloc(NULL, &module_list_type_global, "rlm", true));
 	MEM(module_rlm_virtual_name_tree = fr_rb_inline_alloc(NULL, module_rlm_virtual_t, name_node,
 							      module_rlm_virtual_name_cmp, NULL));
 	fr_atexit_global(_modules_rlm_free_atexit, NULL);
diff --git a/src/lib/server/virtual_servers.c b/src/lib/server/virtual_servers.c
index c63cc65c6bd..f3f6e9c3a9a 100644
--- a/src/lib/server/virtual_servers.c
+++ b/src/lib/server/virtual_servers.c
@@ -1626,8 +1626,14 @@ int virtual_servers_init(void)
 		return -1;
 	}
 
-	MEM(process_modules = module_list_alloc(NULL, &module_list_type_global, "process"));
-	MEM(proto_modules = module_list_alloc(NULL, &module_list_type_global, "protocol"));
+	MEM(process_modules = module_list_alloc(NULL, &module_list_type_global, "process", true));
+
+	/*
+	 *	FIXME - We should be able to turn on write protection,
+	 *	but there are too many proto modules that hang things
+	 *	off of their instance data.
+	 */
+	MEM(proto_modules = module_list_alloc(NULL, &module_list_type_global, "protocol", false));
 	MEM(listen_addr_root = fr_rb_inline_alloc(NULL, fr_listen_t, virtual_server_node, listen_addr_cmp, NULL));
 	MEM(server_section_name_tree = fr_rb_alloc(NULL, server_section_name_cmp, NULL));