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

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

[yanwei.gao]

- 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
 		 * 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;
 	}
 	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;
 	}
 
 	uid_policy_file = securityfs_create_file("uid_allowlist_policy", 0600,
-- 
2.43.5

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

[Micah Morton]

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
>

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

[Micah Morton]

On Wed, Mar 18, 2026 at 1:49 PM Micah Morton <mortonm@chromium.org> wrote:
>
> 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?

I guess securityfs_remove() just returns right away if the
'policy_dir' pointer IS_ERR, so either way is fine. I don't really see
a reason to change this though.. seems better to err on the side of
calling the securityfs_remove() function even if its currently a
no-op.

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