Re: [PATCH v6 0/3] Harden spectrev2 userspace-userspace protection

[Jiri Kosina]

On Wed, 19 Sep 2018, Peter Zijlstra wrote:

> > diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> > index 5c5e7cb597cd..202a4d9c2af7 100644
> > --- a/kernel/ptrace.c
> > +++ b/kernel/ptrace.c
> > @@ -330,9 +330,7 @@ int __ptrace_may_access(struct task_struct *task, unsigned int mode)
> >                !ptrace_has_cap(mm->user_ns, mode))))
> >             return -EPERM;
> > 
> > -       if (!(mode & PTRACE_MODE_NOACCESS_CHK))
> > -               return security_ptrace_access_check(task, mode);
> > -       return 0;
> > +       return security_ptrace_access_check(task, mode);
> >  }
> > 
> >  bool ptrace_may_access(struct task_struct *task, unsigned int mode)
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 161a4f29f860..30d21142e9fe 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2215,7 +2215,12 @@ static int selinux_ptrace_access_check(struct task_struct *child,
> >  {
> >         u32 sid = current_sid();
> >         u32 csid = task_sid(child);
> > +       struct av_decision avd;
> > 
> > +       if (mode == PTRACE_MODE_IBPB)
> > +               return avc_has_perm_noaudit(&selinux_state, sid, csid,
> > +                                           SECCLASS_PROCESS, PROCESS__PTRACE,
> > +                                           0, &avd);
> >         if (mode & PTRACE_MODE_READ)
> >                 return avc_has_perm(&selinux_state,
> >                                     sid, csid, SECCLASS_FILE, FILE__READ, NULL);
> > 
> 
> As far as I can tell, this still has:
> 
> 	avc_has_perm_noaudit()
> 	  security_compute_av()
> 	    read_lock(&state->ss->policy_rwlock);
> 	  avc_insert()
> 	    spin_lock_irqsave();
> 	  avc_denied()
> 	    avc_update_node()
> 	      spin_lock_irqsave();
> 
> under the scheduler's raw_spinlock_t, which are invalid lock nestings.

Agreed. Therefore, if the current form (v6) of the patches is merged, the 
check before security_ptrace_access_check() should stay.

Once all the LSM callbacks are potentially audited, it could then go in 
second phase.

Is there anything else blocking v6 being merged? (and then Tim's set on 
top I guess, once the details are sorted out there).

Thanks,

-- 
Jiri Kosina
SUSE Labs