Messages in this thread | | | From | Andy Lutomirski <> | Date | Fri, 23 Oct 2020 17:12:49 -0700 | Subject | Re: [RFC] Have insn decoder functions return success/failure |
| |
On Fri, Oct 23, 2020 at 4:27 PM Borislav Petkov <bp@alien8.de> wrote: > > On Fri, Oct 23, 2020 at 07:47:04PM +0900, Masami Hiramatsu wrote: > > Thanks! I look forward to it. > > Ok, here's a first stab, it is a single big diff and totally untested > but it should show what I mean. I've made some notes while converting, > as I went along. > > Have a look at insn_decode() and its call sites: they are almost trivial > now because caller needs simply to do: > > if (insn_decode(insn, buffer, ...)) > > and not care about any helper functions. > > For some of the call sites it still makes sense to do a piecemeal insn > decoding and I've left them this way but they can be converted too, if > one wants. > > In any case, just have a look please and lemme know if that looks OKish. > I'll do the actual splitting and testing afterwards. > > And what Andy wants can't be done with the decoder because it already > gets a fixed size buffer and length - it doesn't do the fetching. The > caller does. > > What you wanna do: > > > len = min(15, remaining bytes in page); > > fetch len bytes; > > insn_init(); > > ret = insn_decode_fully(); > > <--- you can't always know here whether the insn is valid if you don't > have all the bytes. But you can always fetch *all* bytes and then give > it to the decoder for checking. > > Also, this doesn't make any sense: try insn decode on a subset of bytes > and then if it fails, try it on the whole set of bytes. Why even try the > subset - it will almost always fail.
I disagree. A real CPU does exactly what I'm describing. If I stick 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.
Eventually we should wrap this whole mess up in an insn_decode_user() helper that does the right thing. 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?
| |