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

[Micah Morton]

On Mon, Mar 2, 2026 at 6:40 PM yanwei.gao <gaoyanwei.tx at 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 at 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
>