netdev - Re: [PATCH v4 27/42] selinux: introduce task_obj

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250616103112.GB23708@horms.kernel.org>
Date: Mon, 16 Jun 2025 11:31:12 +0100
From: Simon Horman <horms@...nel.org>
To: Stephen Smalley <stephen.smalley.work@...il.com>
Cc: selinux@...r.kernel.org, paul@...l-moore.com, omosnace@...hat.com,
	netdev@...r.kernel.org
Subject: Re: [PATCH v4 27/42] selinux: introduce task_obj_perm()

On Fri, Jun 13, 2025 at 02:18:54PM -0400, Stephen Smalley wrote:
> On Fri, Jun 13, 2025 at 10:15 AM Simon Horman <horms@...nel.org> wrote:
> >
> > On Tue, Jun 10, 2025 at 01:21:58PM -0400, Stephen Smalley wrote:
> > > Introduce task_obj_perm() for namespace-aware permission checking
> > > between two tasks using the objective SID for both tasks and
> > > without assuming that either task is current.
> > >
> > > Convert the permission checks of this form in the hook functions
> > > to use this new helper.
> > >
> > > Signed-off-by: Stephen Smalley <stephen.smalley.work@...il.com>
> >
> > ...
> >
> > > +int task_obj_has_perm(const struct task_struct *s,
> > > +                   const struct task_struct *t,
> > > +                   u16 tclass, u32 requested,
> > > +                   struct common_audit_data *ad)
> > > +{
> > > +     const struct cred *cred;
> > > +     const struct task_security_struct *tsec;
> > > +     struct selinux_state *state;
> > > +     u32 ssid;
> > > +     u32 tsid;
> > > +     int rc;
> > > +
> > > +     state = current_selinux_state;
> > > +     rcu_read_lock();
> > > +     tsec = task_security(s);
> > > +     if (tsec)
> > > +             ssid = tsec->sid;
> > > +     else
> > > +             ssid = SECINITSID_UNLABELED;
> >
> > Hi Stephen,
> >
> > Above it is assumed that tsec may be NULL...
> >
> > > +     rcu_read_unlock();
> > > +
> > > +     do {
> > > +             tsid = task_sid_obj_for_state(t, state);
> > > +
> > > +             rc = avc_has_perm(state, ssid, tsid, tclass, requested, ad);
> > > +             if (rc)
> > > +                     return rc;
> > > +
> > > +             cred = tsec->parent_cred;
> >
> > ... but here tsec is dereferenced without explicitly checking if it is not
> > NULL. I'm wondering if this is safe, e.g. due to the call to avc_has_perm().
> 
> No, you are correct - this is a bug. Thank you, fixed via
> https://github.com/stephensmalley/selinux-kernel/commit/85e72ed549d01a2da407feef6493cbdeca324f82
> and will likely squash into this patch on next submission.

Thanks.

One more thing now that I look at this again.
Does the access to tsec above need to be guarded by rcu_read_lock()?

> 
> >
> > Flagged by Smatch.
> >
> > > +             if (!cred)
> > > +                     break;
> > > +
> > > +             rcu_read_lock();
> > > +             tsec = selinux_cred(cred);
> > > +             ssid = tsec->sid;
> > > +             state = tsec->state;
> > > +             rcu_read_unlock();
> > > +     } while (cred);
> > > +
> > > +     return 0;
> > > +}
> >
> > ...
>