lkml.org 
[lkml]   [2020]   [Oct]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC] Have insn decoder functions return success/failure
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>

\
 
 \ /
  Last update: 2020-10-29 01:58    [W:0.091 / U:0.348 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site