From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: SEV-ES guest: In vctrap() only allow CPUID from userspace
To: Mike Larkin <mlarkin@nested.page>
Cc: tech@openbsd.org
Date: Thu, 3 Jul 2025 12:51:52 +0200

On Wed, Jul 02, 2025 at 11:46:38AM -0700, Mike Larkin wrote:
> On Tue, Jul 01, 2025 at 10:36:33AM +0200, Hans-J?rg H?xer wrote:
> > Hi,
> >
> > this diff restricts #VC trap handling.
> >
> > Take care,
> > HJ.
> >
> 
> see below
> 
> > -----------
> > commit bbf8ca5bfcd41771b0ecac3597bf033a603a54c3
> > Author: Hans-Joerg Hoexer <hshoexer@genua.de>
> > Date:   Thu Mar 20 17:17:31 2025 +0100
> >
> >     SEV-ES guest: In vctrap() only allow CPUID also from userspace
> >
> >     CPUID is the only instruction we allow to raise a #VC exception also
> >     from user space.  All other instructions are limited to raise #VC
> >     from kernel space only.
> >
> >     With respect to rdmsr/wrmsr this is an additional safe-guard, as
> >     these two instructions will raise a #GP anyway when the CPL is
> >     greater than 0.
> >
> >     With respect to in/out userland can be allowed to access IO ports.
> >     Howerver, for now we do not want to support this.
> >
> > diff --git a/sys/arch/amd64/amd64/trap.c b/sys/arch/amd64/amd64/trap.c
> > index f68cb2c90d5..9c532846c41 100644
> > --- a/sys/arch/amd64/amd64/trap.c
> > +++ b/sys/arch/amd64/amd64/trap.c
> > @@ -97,7 +97,7 @@
> >
> >  int	upageflttrap(struct trapframe *, uint64_t);
> >  int	kpageflttrap(struct trapframe *, uint64_t);
> > -int	vctrap(struct trapframe *);
> > +int	vctrap(struct trapframe *, int);
> >  void	kerntrap(struct trapframe *);
> >  void	usertrap(struct trapframe *);
> >  void	ast(struct trapframe *);
> > @@ -302,7 +302,7 @@ kpageflttrap(struct trapframe *frame, uint64_t cr2)
> >  }
> >
> >  int
> > -vctrap(struct trapframe *frame)
> > +vctrap(struct trapframe *frame, int user)
> >  {
> >  	uint64_t	 sw_exitcode, sw_exitinfo1, sw_exitinfo2;
> >  	uint8_t		*rip = (uint8_t *)(frame->tf_rip);
> > @@ -340,6 +340,8 @@ vctrap(struct trapframe *frame)
> >  		frame->tf_rip += 2;
> >  		break;
> >  	case SVM_VMEXIT_MSR: {
> > +		if (user)
> > +			return 0;	/* not allowed from userspace */
> >  		if (*rip == 0x0f && *(rip + 1) == 0x30) {
> >  			/* WRMSR */
> >  			ghcb_sync_val(GHCB_RAX, GHCB_SZ32, &syncout);
> > @@ -357,6 +359,8 @@ vctrap(struct trapframe *frame)
> >  		break;
> >  	    }
> >  	case SVM_VMEXIT_IOIO: {
> > +		if (user)
> > +			return 0;	/* not allowed from userspace */
> 
> Does this not break cases that use the IOPB for port access from usermode?

Yes, but it kind of intensional.

My first impression was that there might be a security problem.
After discussing with hshoexer@ we came to the conclusion that that
usually a general protections fault is generated by the processor
when accessing such a proviledged instruction.

If it is allowed, e.g. for CPUID or IN/OUT with special setup, we
may end here.  For IN/OUT we have to parse the instructions which
may be incomplete.  Better disable the feature at all.  Do you think
we need IO access from userland when running in SEV-ES virtual
environment?  We can still implement it later.

Note that currently the kernel panics if it does not know the
instruction.

> >  		switch (*rip) {
> >  		case 0x66: {
> >  			switch (*(rip + 1)) {
> > @@ -505,7 +509,7 @@ kerntrap(struct trapframe *frame)
> >  #endif /* NISA > 0 */
> >
> >  	case T_VC:
> > -		if (vctrap(frame))
> > +		if (vctrap(frame, 0))
> >  			return;
> >  		goto we_re_toast;
> >  	}
> > @@ -588,9 +592,11 @@ usertrap(struct trapframe *frame)
> >  		    : ILL_BADSTK;
> >  		break;
> >  	case T_VC:
> > -		vctrap(frame);
> > -		goto out;
> > -
> > +		if (vctrap(frame, 1))
> > +			goto out;
> > +		sig = SIGILL;
> > +		code = ILL_PRVOPC;
> > +		break;
> >  	case T_PAGEFLT:			/* page fault */
> >  		if (!uvm_map_inentry(p, &p->p_spinentry, PROC_STACK(p),
> >  		    "[%s]%d/%d sp=%lx inside %lx-%lx: not MAP_STACK\n",
> > diff --git a/sys/arch/amd64/amd64/vector.S b/sys/arch/amd64/amd64/vector.S
> > index 59823862c18..4181ab216d0 100644
> > --- a/sys/arch/amd64/amd64/vector.S
> > +++ b/sys/arch/amd64/amd64/vector.S
> > @@ -553,6 +553,7 @@ IDTVEC(vctrap_early)
> >  	TRAP_ENTRY_KERN		/* early #VC has to be in kernel mode */
> >  	cld
> >  	movq	%rsp, %rdi
> > +	movq	$0x0, %rsi
> >  	call	vctrap
> >  	movq	$0,-8(%rsp)
> >  	INTRFASTEXIT
>