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

From: Borislav Petkov <bp@alien8.de>
To: Andy Lutomirski <luto@kernel.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>, x86-ml <x86@kernel.org>,
	Joerg Roedel <jroedel@suse.de>,
	lkml <linux-kernel@vger.kernel.org>
Subject: Re: [RFC] Have insn decoder functions return success/failure
Date: Sat, 24 Oct 2020 10:23:16 +0200	[thread overview]
Message-ID: <20201024082316.GA11562@zn.tnic> (raw)
In-Reply-To: <CALCETrVQDVLPwTTXgsRYSjxVmzeK5ekmrEiT2rWkQKO0inRLGQ@mail.gmail.com>

On Fri, Oct 23, 2020 at 05:12:49PM -0700, Andy Lutomirski wrote:
> I disagree.  A real CPU does exactly what I'm describing.  If I stick

A real modern CPU fetches up to 32 bytes insn window which it tries
to decode etc. I don't know, though, what it does when that fetch
encounters a fault - I will have to ask people. I'm not sure it would
even try to feed a shorter stream of bytes to the decoder but lemme
ask...

> 0xcc at the end of a page and a make the next page not-present, I get
> #BP, not #PF.  But if I stick 0x0F at the end of a page and mark the
> next page not-present, I get #PF.  If we're trying to decode an
> instruction in user memory, we can kludge it by trying to fetch 15
> bytes and handling -EFAULT by fetching fewer bytes, but that's gross
> and doesn't really have the right semantics.  What we actually want is
> to fetch up to the page boundary and try to decode it.  If it's a
> valid instruction or if it's definitely invalid, we're done.
> Otherwise we fetch across the page boundary.

We can do that but why would you put all that logic in the insn decoder?
Is that use case sooo important?

I mean, it would work that way anyway *even* *now* - the insn decoder
will tell you that the insn it decoded wasn't valid and you, as a
caller, know that you didn't fetch the whole 15 bytes so that means
that you still need to fetch some more. You've got all the relevant
information.

> Eventually we should wrap this whole mess up in an insn_decode_user()
> helper that does the right thing.

Oh sure, you can do that easily. Just put the logic which determines
that it copied a shorter buffer and that it attempts to decode the
shorter buffer first in it. Yah, that can work.

> And we can then make that helper
> extra fancy by getting PKRU and EPT-hacker-execute-only right, whereas
> we currently get these cases wrong.
> 
> Does this make sense?

Sure, but you could point me to those cases so that I can get a better
idea what they do exactly.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette