From nobody Thu Apr 3 19:31:59 2025 X-Original-To: dev-commits-src-branches@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4ZTBh05t7Dz5sM0n; Thu, 03 Apr 2025 19:32:00 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R10" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4ZTBgz4nGDz3S6G; Thu, 03 Apr 2025 19:31:59 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1743708719; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=HOpQ4JLs/8qRk6WUkCGZChXLEo9JyFbRJ/B/zCu9+uA=; b=dv4FTHgYxDF6ZTOX6F9oxbqZ7+5HgmBNJwqjCvqbgV412zWraaduN6PWjh0MjTaSjsKTBM QsgN7C9TFkZwHUGPlf2vVlN4tmnrw+SHOQ8ZjkW+k6BZ5mjjJ3HQNqDl2tZv+qqq81mnJs fiYOPc3jan+bzAaBfOBglaZ+36qPCjmlY0kWQiFgDz/7Orhbrea3bJ9N3Qq5GPGZxjwwYq g8m6CQRxgFjoJKpqHsgDTAg193YrwoM+j2Ll4e+dmlVKFFFUZA14v0SfhiUUSN6ugIuVGP xJZ8NtABkD7YvzxwV2o7eKZGU+2ZJfVZsWZW3jx+aF+t0vGJrSEX+pOuz0X9OQ== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1743708719; a=rsa-sha256; cv=none; b=ntRJTjdV+CbbnhpofKDKMwWBU9tRymwRQ176HLdClNtLUGWZ/V+g+5AtLvyp9k7tHQrbLn pHFPHo05EtNEENSLs3xKq1ivJy8a8wjKMfzVirzL2uoRrZJjX06L9x1ARZ+59eOKyqkye1 BV2DxKyIhsq6sY1fsI0BXIzr+G6Dwozp6LNuwAsAGVKFMYLnT1Mt+U1SFUO6ZFVYwB7cm+ mvDFqYwkenmy53/9ImtIetEtMHGH0dJ5Eu8qNglenbleyIddIKX2vPIaxeyvSF1oXXgLe6 bvfBcNkSee2pBP359ckcxl9qm3QbkwJMMrL76sd7EsewZ373AuPpDY0mbaWrLg== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1743708719; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=HOpQ4JLs/8qRk6WUkCGZChXLEo9JyFbRJ/B/zCu9+uA=; b=hEKPCa3WImHcdBflcrsesqCXC1oCziXfivZwKJ5mfhGu9c/Qx8yxJi3cJvCTUCezwUZ/H5 O3v85pH0/yH0xe0I3rS4qOQsIZuWBaNE2T+067rBkFo4qhi/P9/I1AdTqIlNQRX4JimL24 CtQJXo8ak1KiheojblpVPFC9tpGSpcEorG5HKwNNWZBrJsrQjnCLNqYoqQl9bsd2RVHlYN M4QU4l9q25SELTTw1Rl8yWiFwnTntQbVIbn2+aA1749EwNU766e7kgtLS3cm7CWwhheOce qPRezWpLkwYNr9JbXss6qi9OAe/b7WJsXnEg7uCqYmDxqT30upDS2qAYmH+9FQ== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 4ZTBgz4GvPzZl; Thu, 03 Apr 2025 19:31:59 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.18.1/8.18.1) with ESMTP id 533JVx4G035074; Thu, 3 Apr 2025 19:31:59 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.18.1/8.18.1/Submit) id 533JVxng035059; Thu, 3 Apr 2025 19:31:59 GMT (envelope-from git) Date: Thu, 3 Apr 2025 19:31:59 GMT Message-Id: <202504031931.533JVxng035059@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org From: Olivier Certner Subject: git: 862665e3805c - stable/14 - MAC/do: Factor out setting/destroying rule structures List-Id: Commits to the stable branches of the FreeBSD src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-branches List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-branches@freebsd.org Sender: owner-dev-commits-src-branches@FreeBSD.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: olce X-Git-Repository: src X-Git-Refname: refs/heads/stable/14 X-Git-Reftype: branch X-Git-Commit: 862665e3805ca092498c34eca356adb797eeaa6e Auto-Submitted: auto-generated The branch stable/14 has been updated by olce: URL: https://cgit.FreeBSD.org/src/commit/?id=862665e3805ca092498c34eca356adb797eeaa6e commit 862665e3805ca092498c34eca356adb797eeaa6e Author: Olivier Certner AuthorDate: 2024-07-02 17:07:25 +0000 Commit: Olivier Certner CommitDate: 2025-04-03 19:30:57 +0000 MAC/do: Factor out setting/destroying rule structures This generally removes duplicate code and clarifies higher-level operations, allowing to fix several important bugs. New (internal) functions: - ensure_rules(): Ensure that a jail has a populated 'mac_do_osd_jail_slot', and returns the corresponding 'struct rules'. - dealloc_rules(): Destroy the 'mac_do_osd_jail_slot' slot of a jail. - set_rules(): Assign already parsed rules to a jail. Leverages ensure_rules(). - parse_and_set_rules(): Combination of parse_rules() and set_rules(). Bugs fixed in mac_do_prison_set(): - A panic if "mdo" is explicitly passed to JAIL_SYS_NEW but "mdo.rules" is absent, in which case 'rules_string' wasn't set (setting 'rules' at this point would do nothing). - In the JAIL_SYS_NEW case, would release the prison lock and reacquire it, but still using the same 'rules' pointer that can have been freed and changed concurrently, as the prison lock is temporary unlocked. (This is generally a bug of the mac_do_alloc_prison()'s interface when 'lrp' is not NULL.) Suppress mac_do_alloc_prison(), as it has the following bugs: - The interface bug mentioned just above. - Wrong locking, leading to deadlocks in case of setting jail parameters multiple times (concurrently or not). It has been replaced by either parse_and_set_rules(), or by ensure_rules() directly coupled with prison_unlock(). Rename mac_do_dealloc_prison(), the OSD destructor, to dealloc_osd(), and make it free the 'struct rules' itself (which was leaking). While here, in parse_rules(): Clarify the contract by adding comments, and check (again) for the rules specification's length. Reviewed by: bapt Approved by: markj (mentor) Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D47597 (cherry picked from commit bbf8af664dc94804c219cd918788c0c127a5c310) --- sys/security/mac_do/mac_do.c | 235 ++++++++++++++++++++++++++++--------------- 1 file changed, 156 insertions(+), 79 deletions(-) diff --git a/sys/security/mac_do/mac_do.c b/sys/security/mac_do/mac_do.c index dca5a1809966..61c305547d39 100644 --- a/sys/security/mac_do/mac_do.c +++ b/sys/security/mac_do/mac_do.c @@ -69,6 +69,7 @@ toast_rules(struct rulehead *head) TAILQ_REMOVE(head, r, r_entries); free(r, M_DO); } + TAILQ_INIT(head); } static int @@ -129,15 +130,38 @@ out: return (error); } +/* + * Parse rules specification and produce rule structures out of it. + * + * 'head' must be an empty list head. Returns 0 on success, with 'head' filled + * with structures representing the rules. On error, 'head' is left empty and + * the returned value is non-zero. If 'string' has length greater or equal to + * MAC_RULE_STRING_LEN, ENAMETOOLONG is returned. If it is not in the expected + * format (comma-separated list of clauses of the form "=:", + * where is "uid" or "gid", an UID or GID (depending on ) and + * is "*", "any" or some UID), EINVAL is returned. + */ static int parse_rules(const char *const string, struct rulehead *const head) { - struct rule *new; - char *const copy = strdup(string, M_DO); - char *p = copy; + const size_t len = strlen(string); + char *copy; + char *p; char *element; + struct rule *new; int error = 0; + QMD_TAILQ_CHECK_TAIL(head, r_entries); + MPASS(TAILQ_EMPTY(head)); + + if (len >= MAC_RULE_STRING_LEN) + return (ENAMETOOLONG); + + copy = malloc(len + 1, M_DO, M_WAITOK); + bcopy(string, copy, len + 1); + MPASS(copy[len] == '\0'); /* Catch some races. */ + + p = copy; while ((element = strsep(&p, ",")) != NULL) { if (element[0] == '\0') continue; @@ -183,11 +207,125 @@ find_rules(struct prison *const pr, struct prison **const aprp) return (rules); } +/* + * Ensure the passed prison has its own 'struct rules'. + * + * On entry, the prison must be unlocked, but will be returned locked. Returns + * the newly allocated and initialized 'struct rules', or the existing one. + */ +static struct rules * +ensure_rules(struct prison *const pr) +{ + struct rules *rules, *new_rules; + void **rsv; + + if (pr == &prison0) { + prison_lock(pr); + return (&rules0); + } + + /* Optimistically try to avoid memory allocations. */ +restart: + prison_lock(pr); + rules = osd_jail_get(pr, mac_do_osd_jail_slot); + if (rules != NULL) + return (rules); + prison_unlock(pr); + + new_rules = malloc(sizeof(*new_rules), M_DO, M_WAITOK|M_ZERO); + TAILQ_INIT(&new_rules->head); + rsv = osd_reserve(mac_do_osd_jail_slot); + prison_lock(pr); + rules = osd_jail_get(pr, mac_do_osd_jail_slot); + if (rules != NULL) { + /* + * We could cleanup while holding the prison lock (given the + * current implementation of osd_free_reserved()), but be safe + * and a good citizen by not keeping it more than strictly + * necessary. The only consequence is that we have to relookup + * the rules. + */ + prison_unlock(pr); + osd_free_reserved(rsv); + free(new_rules, M_DO); + goto restart; + } + osd_jail_set_reserved(pr, mac_do_osd_jail_slot, rsv, new_rules); + return (new_rules); +} + +/* + * OSD destructor for slot 'mac_do_osd_jail_slot'. + * + * Called with 'value' not NULL. + */ +static void +dealloc_osd(void *const value) +{ + struct rules *const rules = value; + + toast_rules(&rules->head); + free(rules, M_DO); +} + +/* + * Deallocate the rules associated to a prison. + * + * Destroys the 'mac_do_osd_jail_slot' slot of the passed jail. + */ +static void +dealloc_rules(struct prison *const pr) +{ + prison_lock(pr); + /* This calls destructor dealloc_osd(). */ + osd_jail_del(pr, mac_do_osd_jail_slot); + prison_unlock(pr); +} + +/* + * Assign already parsed rules to a jail. + */ +static void +set_rules(struct prison *const pr, const char *const rules_string, + struct rulehead *const head) +{ + struct rules *rules; + struct rulehead old_head; + + MPASS(rules_string != NULL); + MPASS(strlen(rules_string) < MAC_RULE_STRING_LEN); + + TAILQ_INIT(&old_head); + rules = ensure_rules(pr); + strlcpy(rules->string, rules_string, MAC_RULE_STRING_LEN); + TAILQ_CONCAT(&old_head, &rules->head, r_entries); + TAILQ_CONCAT(&rules->head, head, r_entries); + prison_unlock(pr); + toast_rules(&old_head); +} + +/* + * Parse a rules specification and assign them to a jail. + * + * Returns the same error code as parse_rules() (which see). + */ +static int +parse_and_set_rules(struct prison *const pr, const char *rules_string) +{ + struct rulehead head; + int error; + + error = parse_rules(rules_string, &head); + if (error != 0) + return (error); + set_rules(pr, rules_string, &head); + return (0); +} + static int sysctl_rules(SYSCTL_HANDLER_ARGS) { char *new_string; - struct rulehead head, saved_head; struct prison *pr; struct rules *rules; int error; @@ -207,17 +345,7 @@ sysctl_rules(SYSCTL_HANDLER_ARGS) if (error) goto out; - TAILQ_INIT(&head); - error = parse_rules(new_string, &head); - if (error) - goto out; - TAILQ_INIT(&saved_head); - prison_lock(pr); - TAILQ_CONCAT(&saved_head, &rules->head, r_entries); - TAILQ_CONCAT(&rules->head, &head, r_entries); - strlcpy(rules->string, new_string, MAC_RULE_STRING_LEN); - prison_unlock(pr); - toast_rules(&saved_head); + error = parse_and_set_rules(pr, new_string); out: free(new_string, M_DO); @@ -236,51 +364,11 @@ destroy(struct mac_policy_conf *mpc) toast_rules(&rules0.head); } -static void -mac_do_alloc_prison(struct prison *pr, struct rules **lrp) -{ - struct prison *ppr; - struct rules *rules, *new_rules; - void **rsv; - - rules = find_rules(pr, &ppr); - if (ppr == pr) - goto done; - - prison_unlock(ppr); - new_rules = malloc(sizeof(*new_rules), M_PRISON, M_WAITOK|M_ZERO); - rsv = osd_reserve(mac_do_osd_jail_slot); - rules = find_rules(pr, &ppr); - if (ppr == pr) { - free(new_rules, M_PRISON); - osd_free_reserved(rsv); - goto done; - } - prison_lock(pr); - osd_jail_set_reserved(pr, mac_do_osd_jail_slot, rsv, new_rules); - TAILQ_INIT(&new_rules->head); -done: - if (lrp != NULL) - *lrp = rules; - prison_unlock(pr); - prison_unlock(ppr); -} - -static void -mac_do_dealloc_prison(void *data) -{ - struct rules *r = data; - - toast_rules(&r->head); -} - static int mac_do_prison_set(void *obj, void *data) { struct prison *pr = obj; struct vfsoptlist *opts = data; - struct rulehead head, saved_head; - struct rules *rules; char *rules_string; int error, jsys, len; @@ -289,33 +377,19 @@ mac_do_prison_set(void *obj, void *data) jsys = -1; error = vfs_getopt(opts, "mdo.rules", (void **)&rules_string, &len); if (error == ENOENT) - rules = NULL; + rules_string = ""; else jsys = JAIL_SYS_NEW; switch (jsys) { case JAIL_SYS_INHERIT: - prison_lock(pr); - osd_jail_del(pr, mac_do_osd_jail_slot); - prison_unlock(pr); + dealloc_rules(pr); + error = 0; break; case JAIL_SYS_NEW: - mac_do_alloc_prison(pr, &rules); - if (rules_string == NULL) - break; - TAILQ_INIT(&head); - error = parse_rules(rules_string, &head); - if (error) - return (1); - TAILQ_INIT(&saved_head); - prison_lock(pr); - TAILQ_CONCAT(&saved_head, &rules->head, r_entries); - TAILQ_CONCAT(&rules->head, &head, r_entries); - strlcpy(rules->string, rules_string, MAC_RULE_STRING_LEN); - prison_unlock(pr); - toast_rules(&saved_head); + error = parse_and_set_rules(pr, rules_string); break; } - return (0); + return (error); } SYSCTL_JAIL_PARAM_SYS_NODE(mdo, CTLFLAG_RW, "Jail MAC/do parameters"); @@ -346,9 +420,10 @@ done: static int mac_do_prison_create(void *obj, void *data __unused) { - struct prison *pr = obj; + struct prison *const pr = obj; - mac_do_alloc_prison(pr, NULL); + (void)ensure_rules(pr); + prison_unlock(pr); return (0); } @@ -405,11 +480,13 @@ init(struct mac_policy_conf *mpc) }; struct prison *pr; - mac_do_osd_jail_slot = osd_jail_register(mac_do_dealloc_prison, methods); + mac_do_osd_jail_slot = osd_jail_register(dealloc_osd, methods); TAILQ_INIT(&rules0.head); sx_slock(&allprison_lock); - TAILQ_FOREACH(pr, &allprison, pr_list) - mac_do_alloc_prison(pr, NULL); + TAILQ_FOREACH(pr, &allprison, pr_list) { + (void)ensure_rules(pr); + prison_unlock(pr); + } sx_sunlock(&allprison_lock); }