On Mon, Jun 16, 2025 at 6:31 AM Simon Horman [off-list ref] wrote:

On Fri, Jun 13, 2025 at 02:18:54PM -0400, Stephen Smalley wrote:

quoted

On Fri, Jun 13, 2025 at 10:15 AM Simon Horman [off-list ref] wrote:

quoted

On Tue, Jun 10, 2025 at 01:21:58PM -0400, Stephen Smalley wrote:

quoted

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@gmail.com>

...

quoted

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

quoted

+     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()?

Yes, I believe you are correct - will fix. Since there is only one
caller of this function, possibly some simplification and optimization
is possible here.

quoted

quoted

Flagged by Smatch.

quoted

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

...