Date:   Wed, 5 Jan 2022 18:43:11 +0300
From:   "Kirill A. Shutemov" <kirill@shutemov.name>
To:     Borislav Petkov <bp@alien8.de>
Cc:     "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        tglx@linutronix.de, mingo@redhat.com, dave.hansen@intel.com,
        luto@kernel.org, peterz@infradead.org,
        sathyanarayanan.kuppuswamy@linux.intel.com, aarcange@redhat.com,
        ak@linux.intel.com, dan.j.williams@intel.com, david@redhat.com,
        hpa@zytor.com, jgross@suse.com, jmattson@google.com,
        joro@8bytes.org, jpoimboe@redhat.com, knsathya@kernel.org,
        pbonzini@redhat.com, sdeep@vmware.com, seanjc@google.com,
        tony.luck@intel.com, vkuznets@redhat.com, wanpengli@tencent.com,
        x86@kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 08/26] x86/tdx: Handle in-kernel MMIO
Message-ID: <20220105154311.bocij4mwbga4t2hb@box.shutemov.name>
References: <20211214150304.62613-1-kirill.shutemov@linux.intel.com>
 <20211214150304.62613-9-kirill.shutemov@linux.intel.com>
 <YdV1BpMiAUGrwASv@zn.tnic>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YdV1BpMiAUGrwASv@zn.tnic>
Precedence: bulk

On Wed, Jan 05, 2022 at 11:37:58AM +0100, Borislav Petkov wrote:
> On Tue, Dec 14, 2021 at 06:02:46PM +0300, Kirill A. Shutemov wrote:
> > In non-TDX VMs, MMIO is implemented by providing the guest a mapping
> > which will cause a VMEXIT on access and then the VMM emulating the
> > instruction that caused the VMEXIT. That's not possible in TDX guests
> > because it requires exposing guest register and memory state to
> > potentially malicious VMM.
> 
> What does that mean exactly? Aren't TDX registers encrypted just like
> SEV-ES ones? If so, they can't really be exposed...

Not encrypted, saved/restored by TDX module. But yes, cannot be exposed
(without guest intend).

I talk here about *why* the traditional way to handle MMIO -- on VMM side
-- doesn't work for TDX. It's not safe with untrusted VMM.

> > In TDX the MMIO regions are instead configured to trigger a #VE
> > exception in the guest. The guest #VE handler then emulates the MMIO
> > instruction inside the guest and converts them into a controlled
> 
> s/them/it/
> 
> > hypercall to the host.
> > 
> > MMIO addresses can be used with any CPU instruction that accesses the
> 
> s/the //
> 
> > memory. This patch, however, covers only MMIO accesses done via io.h
> 
> "Here are covered only the MMIO accesses ... "
> 
> > helpers, such as 'readl()' or 'writeq()'.
> > 
> > MMIO access via other means (like structure overlays) may result in
> > MMIO_DECODE_FAILED and an oops.
> 
> Why? They won't cause a EXIT_REASON_EPT_VIOLATION #VE or?

readX()/writeX() helpers limit the range of instructions which can trigger
MMIO. It makes MMIO instruction emulation feasible. Raw access to MMIO
region allows compiler to generate whatever instruction it wants.
Supporting all possible instructions is a task of a different scope.

> 
> > AMD SEV has the same limitations to MMIO handling.
> 
> See, the other guy is no better here. :-P

... but it works fine :P

> > +static int tdx_handle_mmio(struct pt_regs *regs, struct ve_info *ve)
> > +{
> > +	char buffer[MAX_INSN_SIZE];
> > +	unsigned long *reg, val = 0;
> > +	struct insn insn = {};
> > +	enum mmio_type mmio;
> > +	int size;
> > +	u8 sign_byte;
> > +	bool err;
> > +
> > +	if (copy_from_kernel_nofault(buffer, (void *)regs->ip, MAX_INSN_SIZE))
> > +		return -EFAULT;
> > +
> > +	insn_init(&insn, buffer, MAX_INSN_SIZE, 1);
> > +	insn_get_length(&insn);
> 
> There is insn_decode() - see how it is used and use it here pls.

Right, missed that.

> > +	case MMIO_READ_SIGN_EXTEND:
> > +		err = tdx_mmio_read(size, ve->gpa, &val);
> > +		if (err)
> > +			break;
> > +
> > +		if (size == 1)
> > +			sign_byte = (val & 0x80) ? 0xff : 0x00;
> > +		else
> > +			sign_byte = (val & 0x8000) ? 0xff : 0x00;
> > +
> > +		/* Sign extend based on operand size */
> > +		memset(reg, sign_byte, insn.opnd_bytes);
> > +		memcpy(reg, &val, size);
> > +		break;
> 
> You can simplify this a bit:
> 
>         case MMIO_READ_SIGN_EXTEND: {
>                 u8 sign_byte = 0, msb = 7;
> 
>                 err = tdx_mmio_read(size, ve->gpa, &val);
>                 if (err)
>                         break;
> 
>                 if (size > 1)
>                         msb = 15;
> 
>                 if (val & BIT(msb))
>                         sign_byte = -1;
> 
>                 /* Sign extend based on operand size */
>                 memset(reg, sign_byte, insn.opnd_bytes);
>                 memcpy(reg, &val, size);
>                 break;
>         }

Okay, will do.

-- 
 Kirill A. Shutemov