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

[Borislav Petkov]

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.

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.
> 
> 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?)
> 
> 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