Virtualization - Jun 2020 - Should SEV-ES #VC use IST? (Re: [PATCH] Allow RDTSC and RDTSCP from userspace)

On Tue, Jun 23, 2020 at 04:59:14PM +0200, Joerg Roedel
wrote:
> On Tue, Jun 23, 2020 at 04:53:44PM +0200, Peter Zijlstra wrote:
> > +noinstr void idtentry_validate_ist(struct pt_regs *regs)
> > +{
> > +	if ((regs->sp & ~(EXCEPTION_STKSZ-1)) => > +	   
(_RET_IP_ & ~(EXCEPTION_STKSZ-1)))
> > +		die("IST stack recursion", regs, 0);
> > +}
> 
> Yes, this is a start, it doesn't cover the case where the NMI stack is
> in-between, so I think you need to walk down regs->sp too.
That shouldn't be possible with the current code, I think.
> The dumpstack code already has some logic for this.
Reliability of that depends on the unwinder, I wouldn't want the guess
uwinder to OOPS me by accident.

On Tue, Jun 23, 2020 at 05:23:26PM +0200, Peter Zijlstra
wrote:
> On Tue, Jun 23, 2020 at 04:59:14PM +0200, Joerg Roedel wrote:
> > On Tue, Jun 23, 2020 at 04:53:44PM +0200, Peter Zijlstra wrote:
> > > +noinstr void idtentry_validate_ist(struct pt_regs *regs)
> > > +{
> > > +	if ((regs->sp & ~(EXCEPTION_STKSZ-1)) => > > +	
(_RET_IP_ & ~(EXCEPTION_STKSZ-1)))
> > > +		die("IST stack recursion", regs, 0);
> > > +}
> > 
> > Yes, this is a start, it doesn't cover the case where the NMI
stack is
> > in-between, so I think you need to walk down regs->sp too.
> 
> That shouldn't be possible with the current code, I think.
To clarify, we have: NMI, MCE, DB and DF.

DF (with the exception of ESPFIX) is fatal.
MCE from kernel is fatal (which is what makes the MCE in NMI 'work')
NMI and DB clear DR7, which avoids DB in NMI.

So that leaves: NMI in DB, and that works.

On Tue, Jun 23, 2020 at 05:23:26PM +0200, Peter Zijlstra
wrote:
> On Tue, Jun 23, 2020 at 04:59:14PM +0200, Joerg Roedel wrote:
> > On Tue, Jun 23, 2020 at 04:53:44PM +0200, Peter Zijlstra wrote:
> > > +noinstr void idtentry_validate_ist(struct pt_regs *regs)
> > > +{
> > > +	if ((regs->sp & ~(EXCEPTION_STKSZ-1)) => > > +	
(_RET_IP_ & ~(EXCEPTION_STKSZ-1)))
> > > +		die("IST stack recursion", regs, 0);
> > > +}
> > 
> > Yes, this is a start, it doesn't cover the case where the NMI
stack is
> > in-between, so I think you need to walk down regs->sp too.
> 
> That shouldn't be possible with the current code, I think.
Not with the current code, but possibly with SNP #VC exceptions:

      ->  First #VC
	  -> NMI before VC handler switched off its IST stack
	     (now on NMI IST stack)
	      -> Second SNP #VC exception before the NMI handler did the
		 #VC stack check (because HV messed around with some pages
		 touched there).

In the second #VC you use the same IST stack as in the first #VC, but
the the NMI-stack in-between.
> Reliability of that depends on the unwinder, I wouldn't want the guess
> uwinder to OOPS me by accident.
It doesn't use the full unwinder, it just assumes that there is a
pt_regs struct at the top of every kernel stack and walks through them
until SP points to a user-space stack.

As long as the assumption that there is a pt_regs struct on top of every
stack holds, this should be safe. The assumption might be wrong when an
exception happens during SYSCALL/SYSENTER entry, when the return frame
is not written by hardware.


	Joerg

On 23/06/2020 16:23, Peter Zijlstra wrote:
> On Tue, Jun 23, 2020 at 04:59:14PM +0200, Joerg Roedel wrote:
>> On Tue, Jun 23, 2020 at 04:53:44PM +0200, Peter Zijlstra wrote:
>>> +noinstr void idtentry_validate_ist(struct pt_regs *regs)
>>> +{
>>> +	if ((regs->sp & ~(EXCEPTION_STKSZ-1)) =>>> +	   
(_RET_IP_ & ~(EXCEPTION_STKSZ-1)))
>>> +		die("IST stack recursion", regs, 0);
>>> +}
>> Yes, this is a start, it doesn't cover the case where the NMI stack
is
>> in-between, so I think you need to walk down regs->sp too.
> That shouldn't be possible with the current code, I think.
NMI; #MC; Anything which IRET but isn't fatal - #DB, or #BP from
patching, #GP from *_safe(), etc; NMI

Sure its a corner case, but did you hear that IST is evil?

~Andrew

P.S. did you also hear that with Rowhammer, userspace has a nonzero
quantity of control over generating #MC, depending on how ECC is
configured on the platform.

On Tue, Jun 23, 2020 at 04:39:26PM +0100, Andrew Cooper
wrote:
> On 23/06/2020 16:23, Peter Zijlstra wrote:
> > On Tue, Jun 23, 2020 at 04:59:14PM +0200, Joerg Roedel wrote:
> >> Yes, this is a start, it doesn't cover the case where the NMI
stack is
> >> in-between, so I think you need to walk down regs->sp too.
> > That shouldn't be possible with the current code, I think.
> 
> NMI; #MC; Anything which IRET but isn't fatal - #DB, or #BP from
> patching, #GP from *_safe(), etc; NMI
> 
> Sure its a corner case, but did you hear that IST is evil?
Isn't current #MC unconditionally fatal from kernel? But yes, I was
sorta aware people want that changed.

And yes, NMI can recurse, mostly on #BP and #PF. Like I wrote, its
broken vs #MC.

But Joerg was talking about IST recursion with NMI in the middle,
something like: #DB, NMI, #DB, and not already being fatal. This one in
particular is ruled out by #DB itself clearing DR7 (but NMI would also
do that).
> P.S. did you also hear that with Rowhammer, userspace has a nonzero
> quantity of control over generating #MC, depending on how ECC is
> configured on the platform.
Yes, excellent stuff.

On Tue, Jun 23, 2020 at 05:38:55PM +0200, Joerg Roedel
wrote:
> On Tue, Jun 23, 2020 at 05:23:26PM +0200, Peter Zijlstra wrote:
> > Reliability of that depends on the unwinder, I wouldn't want the
guess
> > uwinder to OOPS me by accident.
> 
> It doesn't use the full unwinder, it just assumes that there is a
> pt_regs struct at the top of every kernel stack and walks through them
> until SP points to a user-space stack.
> 
> As long as the assumption that there is a pt_regs struct on top of every
> stack holds, this should be safe. The assumption might be wrong when an
> exception happens during SYSCALL/SYSENTER entry, when the return frame
> is not written by hardware.
The IRQ and SoftIRQ stacks don't have that I think. Only the task and
exception stacks.

On Tue, Jun 23, 2020 at 04:39:26PM +0100, Andrew Cooper
wrote:
> P.S. did you also hear that with Rowhammer, userspace has a nonzero
> quantity of control over generating #MC, depending on how ECC is
> configured on the platform.
Where does that #MC point to? Can it control for which address to flip
the bits for, i.e., make the #MC appear it has been generated for an
address in kernel space?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette