Virtualization - Jun 2020 - [PATCH v3 25/75] x86/sev-es: Add support for handling IOIO exceptions

On Wed, Jun 03, 2020 at 04:23:25PM +0200, Joerg Roedel
wrote:
> > > +		 */
> > > +		io_bytes   = (exit_info_1 >> 4) & 0x7;
> > > +		ghcb_count = sizeof(ghcb->shared_buffer) / io_bytes;
> > > +
> > > +		op_count    = (exit_info_1 & IOIO_REP) ? regs->cx : 1;
> > > +		exit_info_2 = min(op_count, ghcb_count);
> > > +		exit_bytes  = exit_info_2 * io_bytes;
> > > +
> > > +		es_base = insn_get_seg_base(ctxt->regs, INAT_SEG_REG_ES);
> > > +
> > > +		if (!(exit_info_1 & IOIO_TYPE_IN)) {
> > > +			ret = vc_insn_string_read(ctxt,
> > > +					       (void *)(es_base + regs->si),
> > 
> > SEV(-ES) is 64-bit only, why bother with the es_base charade?
> 
> User-space can also cause IOIO #VC exceptions, and user-space can be
> 32-bit legacy code with segments, so es_base has to be taken into
> account.
Is there actually a use case for this?  Exposing port IO to userspace
doesn't exactly improve security.

Given that i386 ABI requires EFLAGS.DF=0 upon function entry/exit, i.e. is
the de facto default, the DF bug implies this hasn't been tested.  And I
don't see how this could possibly have worked for SEV given that the kernel
unrolls string I/O because the VMM can't emulate string I/O.  Presumably
someone would have complained if they "needed" to run legacy crud. 
The
host and guest obviously need major updates, so supporting e.g. DPDK with
legacy virtio seems rather silly.
> > > +					       ghcb->shared_buffer, io_bytes,
> > > +					       exit_info_2, df);
> > 
> > df handling is busted, it's aways non-zero.  Same goes for the
SI/DI
> > adjustments below.
> 
> Right, this is fixed now.
> 
> > Batching the memory accesses and I/O accesses separately is
technically
> > wrong, e.g. a #DB on a memory access will result in bogus data being
shown
> > in the debugger.  In practice it seems unlikely to matter, but I'm
curious
> > as to why string I/O is supported in the first place.  I didn't
think there
> > was that much string I/O in the kernel?
> 
> True, #DBs won't be correct anymore. Currently debugging is not
> supported in SEV-ES guests anyway, but if it is supported the #DB
> exception would happen in the #VC handler and not on the original
> instruction.
As in, the guest can't debug itself?  Or the host can't debug the guest?

On Wed, Jun 03, 2020 at 04:07:16PM -0700, Sean Christopherson
wrote:
> On Wed, Jun 03, 2020 at 04:23:25PM +0200, Joerg Roedel wrote:
> > User-space can also cause IOIO #VC exceptions, and user-space can be
> > 32-bit legacy code with segments, so es_base has to be taken into
> > account.
> 
> Is there actually a use case for this?  Exposing port IO to userspace
> doesn't exactly improve security.
Might be true, but Linux supports it and this patch-set is not the place
to challenge this feature.
> Given that i386 ABI requires EFLAGS.DF=0 upon function entry/exit, i.e. is
> the de facto default, the DF bug implies this hasn't been tested.  And
I
> don't see how this could possibly have worked for SEV given that the
kernel
> unrolls string I/O because the VMM can't emulate string I/O. 
Presumably
> someone would have complained if they "needed" to run legacy
crud.  The
> host and guest obviously need major updates, so supporting e.g. DPDK with
> legacy virtio seems rather silly.
With SEV-ES and this unrolling of string-io doesn't need to happen
anymore. It is on the list of things to improve, but this patch-set is
already pretty big.
> > True, #DBs won't be correct anymore. Currently debugging is not
> > supported in SEV-ES guests anyway, but if it is supported the #DB
> > exception would happen in the #VC handler and not on the original
> > instruction.
> 
> As in, the guest can't debug itself?  Or the host can't debug the
guest?
Both, the guest can't debug itself because writes to DR7 never make it
to the hardware DR7 register. And the host obviously can't debug the
guest because it has no access to its unencrypted memory and register
state.

Regards,

	Joerg

On Thu, Jun 04, 2020 at 12:15:02PM +0200, Joerg Roedel
wrote:
> On Wed, Jun 03, 2020 at 04:07:16PM -0700, Sean Christopherson wrote:
> > On Wed, Jun 03, 2020 at 04:23:25PM +0200, Joerg Roedel wrote:
> > > User-space can also cause IOIO #VC exceptions, and user-space can
be
> > > 32-bit legacy code with segments, so es_base has to be taken into
> > > account.
> > 
> > Is there actually a use case for this?  Exposing port IO to userspace
> > doesn't exactly improve security.
> 
> Might be true, but Linux supports it and this patch-set is not the place
> to challenge this feature.
But SEV already broke it, no?