RE: [PATCH v5 1/2] x86/speculation: apply IBPB more strictly to avoid cross-process data leak

[Schaufler, Casey]

> -----Original Message-----
> From: Jiri Kosina [mailto:jikos@xxxxxxxxxx]
> Sent: Monday, September 10, 2018 1:42 PM
> To: Schaufler, Casey <casey.schaufler@xxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>; Ingo Molnar <mingo@xxxxxxxxxx>;
> Peter Zijlstra <peterz@xxxxxxxxxxxxx>; Josh Poimboeuf
> <jpoimboe@xxxxxxxxxx>; Andrea Arcangeli <aarcange@xxxxxxxxxx>;
> Woodhouse, David <dwmw@xxxxxxxxxxxx>; Andi Kleen <ak@xxxxxxxxxxxxxxx>;
> Tim Chen <tim.c.chen@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> x86@xxxxxxxxxx
> Subject: RE: [PATCH v5 1/2] x86/speculation: apply IBPB more strictly to avoid
> cross-process data leak
> 
> On Mon, 10 Sep 2018, Schaufler, Casey wrote:
> 
> > It you're going to call __ptrace_access_check(),
> 
> I guess you mean __ptrace_may_access() here.

Sorry, yes. Too much code, too little brain.

> > which already includes an LSM hook, it makes a whole lot of sense to
> > make that the path for doing any module specific checks. It seems wrong
> > to disable the LSM hook there, then turn around and introduce a new one
> > that does the check you just disabled. The patches I had proposed
> > created a new LSM hook because there was not path to an existing hook.
> > With your addition of __ptrace_access_check() that is no longer an issue
> > once any locking problems are resolved. Rather than use a new hook, the
> > existing ptrace hooks ought to work just fine, and any new checks can be
> > added in a new module that has its own ptrace_access_check hook.
> 
> Sorry for being dense, but what exactly are you proposing here then?

Sorry for not being clear.

You added a call to __ptrace_may_access(). The security modules that get
called from __ptrace_may_access() via security_ptrace_access_check()
have known and/or suspected locking issues. So far so good. ...

> This patch (v4 and v5) explicitly avoids calling out to ptrace_has_cap()
> (which calls out to LSM through has_ns_capability_*() ->
> security_capable()) in PTRACE_MODE_IBPB case, exactly to avoid locking
> issues with security modules; there are known callchains that lead to
> deadlock.

So rather than avoiding calling these, why not avoid doing the things inside
the module specific code that cause the locking issues? Move the checks you
put in the ptrace code into the security module hooks.  I can't say 100%, but from
what I've seen so far, it's locking in the audit sub-system that causes the issues,
and that is pretty easy to work around.

> With the same reasoning, security_ptrace_access_check() call is avoided,
> only there is no know particular callchain that'd lead to a lock being
> taken, but noone has done such audit (yet), as it's all hidden behind LSM
> callbacks.

I've had a pretty good look, and there's nothing magic about the LSM callbacks.

> So please tell me what exactly you'd like to see changed in the IBPB patch
> and why exactly, I am not seeing it yet.

Short of a patch to show the changes (which I wish I could do today, but really can't)
what I want to see is:

	- Put ptrace back to using the security module interfaces.
	- Identify where this causes locking issues and work with the module
	  owners (a reasonable lot, all) to provide lock safe paths for the IBPB case.

Otherwise, I have to add a new LSM hook right after your ptrace call and duplicate
a whole lot of what you've just turned off, plus creating lock safe code that duplicates
what ptrace already does. While I would rather have the side-channel checks be
separate from the ptrace checks I can't justify doing both.

I'm sorry if that isn't clearer. I am trying.

> 
> Thanks,
> 
> --
> Jiri Kosina
> SUSE Labs