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
