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

[Micah Morton]

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

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
> >