Re: [PATCH v7 1/3] x86/speculation: apply IBPB more strictly to avoid cross-process data leak

[Thomas Gleixner]

On Thu, 27 Sep 2018, Stephen Smalley wrote:
> On 09/25/2018 08:38 AM, Jiri Kosina wrote:
> >   +static bool ibpb_needed(struct task_struct *tsk, u64 last_ctx_id)
> > +{
> > +	/*
> > +	 * Check if the current (previous) task has access to the memory
> > +	 * of the @tsk (next) task. If access is denied, make sure to
> > +	 * issue a IBPB to stop user->user Spectre-v2 attacks.
> > +	 *
> > +	 * Note: __ptrace_may_access() returns 0 or -ERRNO.
> > +	 */
> > +	return (tsk && tsk->mm && tsk->mm->context.ctx_id != last_ctx_id &&
> > +		ptrace_may_access_sched(tsk, PTRACE_MODE_SPEC_IBPB));
> 
> Would there be any safe way to perform the ptrace check earlier at a point
> where the locking constraints are less severe, and just pass down the result
> to this code?  Possibly just defaulting to enabling IBPB for safety if
> something changed in the interim that would invalidate the earlier ptrace
> check?  Probably not possible, but I thought I'd ask as it would avoid the
> need to skip both the ptrace_has_cap check and the LSM hook, and would reduce
> the critical section.

It's not possible unfortunately as this happens under the scheduler run
queue lock and this needs to be taken to figure out which is the next
task. We can't drop it before context switch and revisit the decision
afterwards to verify it, that would be a massive performance issue and open
an even more horrible can of worms.

Any check which needs to be done in that context should be as minimalistic
as possible. So having a special mode which then might invoke special hooks
makes a lot of sense.

> > + * Returns true on success, false on denial.
> > + *
> > + * Similar to ptrace_may_access(). Only to be called from context switch
> > + * code. Does not call into audit and the regular LSM hooks due to locking
> > + * constraints.
> 
> Pardon my ignorance, but can you clarify exactly what are the locking
> constraints for any code that might be called now or in the future from
> ptrace_may_access_sched().  What's permissible?  rcu_read_lock()?

rcu_read_lock() is fine. Locks might be fine, but the probability that you
run into a lock inversion is extremly high. Also please keep in mind that
this wants to be a raw_spinlock as otherwise preempt-RT will have issues
and the lock sections need to be really short. switch_to() is a hot path.

Thanks,

	tglx