Re: [RFC] Have insn decoder functions return success/failure

[Masami Hiramatsu <mhiramat@kernel.org>]

On Tue, 27 Oct 2020 14:42:51 +0100
Borislav Petkov <bp@alien8.de> wrote:

> On Sat, Oct 24, 2020 at 09:10:25AM -0700, Andy Lutomirski wrote:
> > I can pretty much guarantee that a real modern CPU is able to decode a
> > <15 byte instruction that is followed by unmapped or non-executable
> > pages.  I don't know specifically how the CPU implements it, but it
> > works.
> 
> Yes, so reportedly and architecturally, a CPU tries to execute every
> last byte it has fetched. If it fails decoding an instruction because it
> is incomplete, then it raises a #PF. So you're correct.
> 
> > If I have a page that ends in 0x0F followed by an unmapped page, then
> > the correct response to an attempt to decode is SIGSEGV or -EFAULT.
> > If there's a page there that contains garbage, then the correct
> > response is SIGILL or -EINVAL or similar.  These are different
> > scenarios, and I don't think the current decoder API can be used to
> > distinguish them.
> 
> See above - the insn decoder should be taught to look only at the bytes
> it is *allowed* to look, i.e., the bytes which have been fetched and not
> peek forward. And I believe it does that to some extent but I need to
> look closer.

Yeah, it always does except for the prefix decoding. Anyway, it always
check the boundary (end address) when peek the byte.

> And it should detect the cases where the insn bytes come short. But that
> needs also looking but first things first.
> 
> Bottomline: it should do exactly what a CPU does, IMO.
> 
> Again, find me on IRC to hash out details but I believe we're in an
> agreement here.
> 
> > Take a look at fixup_umip_exception().  It currently has two bugs:
> > 
> > 1. If it tries to decode a short instruction followed by something
> > like a userfaultfd page, it will incorrectly trigger the userfaultfd.
> > This is because it tries to fetch MAX_INSN_SIZE even if the
> > instruction is shorter than that.

Hmm, did it pass the correct buf_size to insn_init()?
...
        nr_copied = insn_fetch_from_user(regs, buf);
...
Ah, I got it. It copies not until the page boundary but +MAX_INSN_SIZE...

> > 
> > 2. It will fail on execute-only memory, and it will succeed on NX
> > memory.  copy_from_user() is the wrong API to use here.  We don't have
> > the right API, and we should add it.  (Hi Dave - what's the best way
> > to do this?  New get_user_pages() mode?  Try to fault it in, hold an
> > appropriate lock, walk the page tables to check permissions, and then
> > access the user address directly?)

Good point! If we can not read the page we can not decode it by software.

Thank you,

> > 
> > I don't know how much anyone really cares about this for UMIP, but
> > with SEV-ES and such, I can see this becoming more important.
> 
> I'll have a look at those when I do the patchset.
> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette


-- 
Masami Hiramatsu <mhiramat@kernel.org>