Date: Fri, 29 May 2026 16:01:57 +0000 From: Olivier Certner <olce@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: 4c98f7a0025e - main - MAC/do: Serialize installing/modifying some jail's configuration Message-ID: <6a19b875.36111.367fa35d@gitrepo.freebsd.org>
index | next in thread | raw e-mail
The branch main has been updated by olce: URL: https://cgit.FreeBSD.org/src/commit/?id=4c98f7a0025eea550ecf6f93f818cd03c6ac0fd5 commit 4c98f7a0025eea550ecf6f93f818cd03c6ac0fd5 Author: Olivier Certner <olce@FreeBSD.org> AuthorDate: 2026-04-29 18:32:56 +0000 Commit: Olivier Certner <olce@FreeBSD.org> CommitDate: 2026-05-29 15:36:29 +0000 MAC/do: Serialize installing/modifying some jail's configuration See the immediately preceding commit for explanations on what this is fixing. When setting 'mac.do' to 'inherit' on a jail with 'mac.do.rules' and 'mac.do.exec_paths' also specified in the same call, ensure that the check that these passed parameters are the same as those to be inherited is atomic with respect to enabling the inheritance (i.e., removing the jail's 'struct conf' object). (See previous commit "MAC/do: Fix the recent logic to set jail parameters, make it more tolerant" as for why this check exists.) Because we currently only modify a single configuration object per transaction, we introduce the parse_and_commit_conf() wrapper around parse_and_set_conf() to remove duplicated code that would ensue from calling the latter directly, namely, releasing the 'mac_do_rwl' lock and freeing the old configuration object (if any). Taking the 'mac_do_rwl' lock for writing as a way to freeze all accesses to mac_do(4) configurations was deemed too thin an operation to be worth wrapping. Reviewed by: bapt (older version) Fixes: 9818224174c4 ("MAC/do: Executable paths feature (GSoC 2025's final state)") MFC after: 1 month Sponsored by: The FreeBSD Foundation Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38 --- sys/security/mac_do/mac_do.c | 99 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 76 insertions(+), 23 deletions(-) diff --git a/sys/security/mac_do/mac_do.c b/sys/security/mac_do/mac_do.c index 8065ff4a0e47..c30ece0a0794 100644 --- a/sys/security/mac_do/mac_do.c +++ b/sys/security/mac_do/mac_do.c @@ -1404,10 +1404,10 @@ set_conf(struct prison *const pr, struct conf *const conf) * * Destroys the 'osd_jail_slot' slot of the passed jail. */ -static void -remove_conf(struct prison *const pr) +static struct conf * +remove_conf_locked(struct prison *const pr) { - set_conf(pr, NULL); + return (set_conf_locked(pr, NULL, NULL)); } static struct conf * @@ -1481,11 +1481,15 @@ clone_exec_paths(struct exec_paths *const dst, * parameters are copied from those of the passed model configuration (which is * expected to be the currently applicable configuration, i.e., that of the * closest ancestor jail that has one). + * + * 'mac_do_rml' needs to be write-locked (and stays so). 'old_conf' serves to + * return, on no error, the old configuration with a reference (which must be + * eventually freed). */ static int parse_and_set_conf(struct prison *const pr, const char *const rules_string, const char *const exec_paths_string, const struct conf *const model_conf, - struct parse_error **const parse_error) + struct conf **const old_conf, struct parse_error **const parse_error) { struct conf *const conf = new_conf(); int error = 0; @@ -1511,7 +1515,8 @@ parse_and_set_conf(struct prison *const pr, const char *const rules_string, clone_exec_paths(&conf->exec_paths, &model_conf->exec_paths); - set_conf(pr, conf); + MPASS(error == 0); + *old_conf = set_conf_locked(pr, conf, osd_reserve(osd_jail_slot)); MPASS(error == 0 && *parse_error == NULL); out: @@ -1522,6 +1527,30 @@ error: goto out; } +/* + * Calls parse_and_set_conf() and closes the current configuration transaction. + * + * Closes the transaction by unlocking 'mac_do_rml' and releasing the old + * configuration returned by parse_and_set_conf(). + */ +static int +parse_and_commit_conf(struct prison *const pr, const char *const rules_string, + const char *const exec_paths_string, const struct conf *const model_conf, + struct parse_error **const parse_error) +{ + struct conf *old_conf; + int error; + + error = parse_and_set_conf(pr, rules_string, exec_paths_string, + model_conf, &old_conf, parse_error); + rm_wunlock(&mac_do_rml); + + if (error == 0 && old_conf != NULL) + drop_conf(old_conf); + return (error); +} + + static int mac_do_sysctl_rules(SYSCTL_HANDLER_ARGS) { @@ -1531,14 +1560,23 @@ mac_do_sysctl_rules(SYSCTL_HANDLER_ARGS) struct parse_error *parse_error = NULL; int error; - conf = find_conf(pr, NULL); + if (req->newptr != NULL) { + rm_wlock(&mac_do_rml); + conf = find_conf_locked(pr, NULL); + } else + conf = find_conf(pr, NULL); strlcpy(buf, conf->rules.string, MAX_RULE_STRING_SIZE); error = sysctl_handle_string(oidp, buf, MAX_RULE_STRING_SIZE, req); - if (error != 0 || req->newptr == NULL) + if (req->newptr == NULL) + goto out; + if (error != 0) { + rm_wunlock(&mac_do_rml); goto out; + } - error = parse_and_set_conf(pr, buf, NULL, conf, &parse_error); + /* Unlocks 'mac_do_rml'. */ + error = parse_and_commit_conf(pr, buf, NULL, conf, &parse_error); if (error != 0) { if (print_parse_error) printf("MAC/do: Parse error at index %zu: %s\n", @@ -1571,14 +1609,23 @@ mac_do_sysctl_exec_paths(SYSCTL_HANDLER_ARGS) struct parse_error *parse_error = NULL; int error; - conf = find_conf(pr, NULL); + if (req->newptr != NULL) { + rm_wlock(&mac_do_rml); + conf = find_conf_locked(pr, NULL); + } else + conf = find_conf(pr, NULL); strlcpy(buf, conf->exec_paths.exec_paths_str, MAX_EXEC_PATHS_SIZE); error = sysctl_handle_string(oidp, buf, MAX_EXEC_PATHS_SIZE, req); - if (error != 0 || req->newptr == NULL) + if (req->newptr == NULL) goto out; + if (error != 0) { + rm_wunlock(&mac_do_rml); + goto out; + } - error = parse_and_set_conf(pr, NULL, buf, conf, &parse_error); + /* Unlocks 'mac_do_rml'. */ + error = parse_and_commit_conf(pr, NULL, buf, conf, &parse_error); if (error != 0) { if (print_parse_error) printf("MAC/do: Parse error at index %zu: %s\n", @@ -1807,14 +1854,17 @@ mac_do_jail_set(void *obj, void *data) } if (jsys == JAIL_SYS_INHERIT) { + struct conf *old_conf = NULL; + error = 0; + rm_wlock(&mac_do_rml); if (!absent_or_empty_rules || !absent_or_empty_exec_paths) { /* * Some values specified. Check that they match the * ones we are going to inherit. */ - model_conf = find_conf(pr->pr_parent, NULL); + model_conf = find_conf_locked(pr->pr_parent, NULL); if (strcmp(model_conf->rules.string, rules_string) != 0) { error = EINVAL; @@ -1838,23 +1888,25 @@ mac_do_jail_set(void *obj, void *data) } if (error == 0) - /* - * There's no TOCTOU problem here as the removal of the - * current jail's configuration commutes with changing - * the inherited configuration we checked against. - */ - remove_conf(pr); + old_conf = remove_conf_locked(pr); + + rm_wunlock(&mac_do_rml); + + if (old_conf != NULL) + drop_conf(old_conf); return (error); } model_conf = NULL; + /* Freeze configuration accesses. */ + rm_wlock(&mac_do_rml); switch (jsys) { case JAIL_SYS_DISABLE: /* * mac_do(4) is disabled iff one of the parameter's string is - * empty. The parse_and_set_conf() call below treats passing + * empty. The parse_and_commit_conf() call below treats passing * NULL for a parameter as a flag to copy its value from the * relevant ancestor jail's configuration, so we have to watch * for the final result having an empty parameter if no @@ -1876,7 +1928,7 @@ mac_do_jail_set(void *obj, void *data) * values). */ if (rules_string == NULL || exec_paths_string == NULL) - model_conf = find_conf(pr, NULL); + model_conf = find_conf_locked(pr, NULL); /* If both are absent, we have to examine if, in the * currently applicable configuration, one of the * parameters, which we are going to copy, is @@ -1894,16 +1946,17 @@ mac_do_jail_set(void *obj, void *data) break; case JAIL_SYS_NEW: - /* See the comment before the call to find_conf() above. */ + /* See the comment before the same test above. */ if (rules_string == NULL || exec_paths_string == NULL) - model_conf = find_conf(pr, NULL); + model_conf = find_conf_locked(pr, NULL); break; default: __assert_unreachable(); } - error = parse_and_set_conf(pr, rules_string, exec_paths_string, + /* Unlocks 'mac_do_rml'. */ + error = parse_and_commit_conf(pr, rules_string, exec_paths_string, model_conf, &parse_error); if (model_conf != NULL) drop_conf(model_conf);home | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?6a19b875.36111.367fa35d>