Messages in this thread | | | Date | Fri, 5 Nov 2021 22:41:16 +0000 | From | Sean Christopherson <> | Subject | Re: [PATCH v7 09/10] x86/tdx: Handle in-kernel MMIO |
| |
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 >
| |