Re: [PATCH v2 2/5] x86/tdx: Add validation of userspace MMIO instructions

[Alexey Gladkov]

On Mon, Aug 05, 2024 at 10:40:55PM +0000, Edgecombe, Rick P wrote:
> On Mon, 2024-08-05 at 15:29 +0200, Alexey Gladkov (Intel) wrote:
> > +       vaddr = (unsigned long)insn_get_addr_ref(&insn, regs);
> > +
> > +       if (user_mode(regs)) {
> > +               if (mmap_read_lock_killable(current->mm))
> > +                       return -EINTR;
> > +
> > +               ret = valid_vaddr(ve, mmio, size, vaddr);
> > +               if (ret)
> > +                       goto unlock;
> > +       }
> > +
> 
> In the case of user MMIO, if the user instruction + MAX_INSN_SIZE straddles a
> page, then the "fetch" in the kernel could trigger a #VE. In this case the  
> kernel would handle this second #VE as a !user_mode() MMIO I guess.
> 
> Would something prevent the same munmap() checks needing to happen for that
> second kernel #VE? If not, I wonder if the munmap() protection logic should also
> trigger for any userspace range ve->gpa as well.

I've added two more patches that should fix the problem. We can try to
avoid crossing the page boundary by first reading the data to the end of
the page and trying to parse it and only if that fails read MAX_INSN_SIZE.

I fixed this locally for tdx because it is required to read and parse the
buffer at the same time.

It's generally worth fixing elsewhere as well. But this I propose to do by
a separate patchset.

-- 
Rgrds, legion