On Tue, 27 Oct 2020 14:42:51 +0100 Borislav Petkov <b...@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 <mhira...@kernel.org>