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

Micah Morton
2026-03-19T03:27:59Z

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