Re: [PATCH v1] security/safesetid: fix comment and error handling

Micah Morton
2026-03-18T20:50:13Z

On Mon, Mar 2, 2026 at 6:40 PM yanwei.gao <gaoyanwei.tx@gmail.com> wrote:
>
> - Fix comment in lsm.c: use CAP_SETGID instead of CAP_SETUID in the
> GID capability check comment to match the actual logic.
> - In handle_policy_update(), set err = -EINVAL and goto out_free_buf
> when policy type is neither UID nor GID, so the error is returned
> to the caller instead of only logging.
> - In safesetid_init_securityfs(), return ret directly when
> policy_dir creation fails instead of goto error (no cleanup needed
> at that point).
>
> Signed-off-by: yanwei.gao <gaoyanwei.tx@gmail.com>
> ---
> security/safesetid/lsm.c | 2 +-
> security/safesetid/securityfs.c | 4 +++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> index d5fb949050dd..a7b68e65996c 100644
> --- a/security/safesetid/lsm.c
> +++ b/security/safesetid/lsm.c
> @@ -128,7 +128,7 @@ static int safesetid_security_capable(const struct cred *cred,
> if (setid_policy_lookup((kid_t){.gid = cred->gid}, INVALID_ID, GID) == SIDPOL_DEFAULT)
> return 0;
> /*
> - * Reject use of CAP_SETUID for functionality other than calling
> + * Reject use of CAP_SETGID for functionality other than calling
Looks good.
> * set*gid() (e.g. setting up userns gid mappings).
> */
> pr_warn("Operation requires CAP_SETGID, which is not available to GID %u for operations besides approved set*gid transitions\n",
> diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> index a71e548065a9..50682abd342b 100644
> --- a/security/safesetid/securityfs.c
> +++ b/security/safesetid/securityfs.c
> @@ -224,6 +224,8 @@ static ssize_t handle_policy_update(struct file *file,
> } else {
> /* Error, policy type is neither UID or GID */
> pr_warn("error: bad policy type");
> + err = -EINVAL;
> + goto out_free_buf;

This makes sense to me and matches how things are done in the
functions above in verify_ruleset() and parse_policy_line().

Is this code actually reachable? I guess if verify_ruleset returns
-EINVAL due to a bad policy type value the code still hits this spot?

> }
> err = len;
>
> @@ -321,7 +323,7 @@ int __init safesetid_init_securityfs(void)
> policy_dir = securityfs_create_dir("safesetid", NULL);
> if (IS_ERR(policy_dir)) {
> ret = PTR_ERR(policy_dir);
> - goto error;
> + return ret;

We still need the `securityfs_remove(policy_dir)` call to clean up the
dir created with `policy_dir = securityfs_create_dir("safesetid",
NULL)` don't we?

> }
>
> uid_policy_file = securityfs_create_file("uid_allowlist_policy", 0600,
> --
> 2.43.5
>