Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 5 Nov 2021 22:41:16 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@linux.intel.com>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        x86@kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
        David Hildenbrand <david@redhat.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        "H . Peter Anvin" <hpa@zytor.com>,
        Dave Hansen <dave.hansen@intel.com>,
        Tony Luck <tony.luck@intel.com>,
        Dan Williams <dan.j.williams@intel.com>,
        Andi Kleen <ak@linux.intel.com>,
        Kirill Shutemov <kirill.shutemov@linux.intel.com>,
        Kuppuswamy Sathyanarayanan <knsathya@kernel.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 09/10] x86/tdx: Handle in-kernel MMIO
Message-ID: <YYWzDKkH3ZDS1b2J@google.com>
References: <20211005204136.1812078-1-sathyanarayanan.kuppuswamy@linux.intel.com>
 <20211005204136.1812078-10-sathyanarayanan.kuppuswamy@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211005204136.1812078-10-sathyanarayanan.kuppuswamy@linux.intel.com>
Precedence: bulk

On Tue, Oct 05, 2021, Kuppuswamy Sathyanarayanan wrote:
> @@ -207,6 +210,103 @@ static void tdx_handle_io(struct pt_regs *regs, u32 exit_qual)
>  	}
>  }
>  
> +static unsigned long tdx_mmio(int size, bool write, unsigned long addr,
> +			      unsigned long *val)
> +{
> +	struct tdx_hypercall_output out = {0};
> +	u64 err;
> +
> +	err = _tdx_hypercall(EXIT_REASON_EPT_VIOLATION, size, write,
> +			     addr, *val, &out);
> +	*val = out.r11;

Val should not be written on error, and writing it for "write" is also weird.

> +	return err;
> +}
> +
> +static int tdx_handle_mmio(struct pt_regs *regs, struct ve_info *ve)
> +{
> +	char buffer[MAX_INSN_SIZE];
> +	unsigned long *reg, val;
> +	struct insn insn = {};
> +	enum mmio_type mmio;
> +	int size, ret;
> +	u8 sign_byte;
> +
> +	if (user_mode(regs)) {
> +		ret = insn_fetch_from_user(regs, buffer);
> +		if (!ret)
> +			return -EFAULT;
> +		if (!insn_decode_from_regs(&insn, regs, buffer, ret))
> +			return -EFAULT;
> +	} else {
> +		ret = copy_from_kernel_nofault(buffer, (void *)regs->ip,
> +					       MAX_INSN_SIZE);
> +		if (ret)
> +			return -EFAULT;
> +		insn_init(&insn, buffer, MAX_INSN_SIZE, 1);
> +		insn_get_length(&insn);
> +	}
> +
> +	mmio = insn_decode_mmio(&insn, &size);
> +	if (mmio == MMIO_DECODE_FAILED)
> +		return -EFAULT;
> +
> +	if (mmio != MMIO_WRITE_IMM && mmio != MMIO_MOVS) {
> +		reg = insn_get_modrm_reg_ptr(&insn, regs);
> +		if (!reg)
> +			return -EFAULT;
> +	}
> +
> +	switch (mmio) {
> +	case MMIO_WRITE:
> +		memcpy(&val, reg, size);
> +		ret = tdx_mmio(size, true, ve->gpa, &val);
> +		break;
> +	case MMIO_WRITE_IMM:
> +		val = insn.immediate.value;
> +		ret = tdx_mmio(size, true, ve->gpa, &val);
> +		break;
> +	case MMIO_READ:
> +		ret = tdx_mmio(size, false, ve->gpa, &val);

val is never set, i.e. this is leaking stack data to the untrusted VMM.

> +		if (ret)
> +			break;
> +		/* Zero-extend for 32-bit operation */
> +		if (size == 4)
> +			*reg = 0;
> +		memcpy(reg, &val, size);
> +		break;
> +	case MMIO_READ_ZERO_EXTEND:
> +		ret = tdx_mmio(size, false, ve->gpa, &val);

And here.

> +		if (ret)
> +			break;
> +
> +		/* Zero extend based on operand size */
> +		memset(reg, 0, insn.opnd_bytes);
> +		memcpy(reg, &val, size);
> +		break;
> +	case MMIO_READ_SIGN_EXTEND:
> +		ret = tdx_mmio(size, false, ve->gpa, &val);

And here.

> +		if (ret)
> +			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;
> +	case MMIO_MOVS:
> +	case MMIO_DECODE_FAILED:
> +		return -EFAULT;
> +	}
> +
> +	if (ret)
> +		return -EFAULT;
> +	return insn.length;
> +}
> +
>  unsigned long tdx_get_ve_info(struct ve_info *ve)
>  {
>  	struct tdx_module_output out = {0};
> @@ -256,6 +356,14 @@ int tdx_handle_virtualization_exception(struct pt_regs *regs,
>  	case EXIT_REASON_IO_INSTRUCTION:
>  		tdx_handle_io(regs, ve->exit_qual);
>  		break;
> +	case EXIT_REASON_EPT_VIOLATION:
> +		/* Currently only MMIO triggers EPT violation */
> +		ve->instr_len = tdx_handle_mmio(regs, ve);
> +		if (ve->instr_len < 0) {
> +			pr_warn_once("MMIO failed\n");

That's not remotely helpful.  Why not?

		if (WARN_ON_ONCE(ve->instr_len < 0))
			return -EFAULT;

> +			return -EFAULT;
> +		}
> +		break;
>  	default:
>  		pr_warn("Unexpected #VE: %lld\n", ve->exit_reason);
>  		return -EFAULT;
> -- 
> 2.25.1
>