Re: [PATCH v7 09/10] x86/tdx: Handle in-kernel MMIO

Sean Christopherson
2021-11-05T22:41:24Z

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
>