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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201022093044.GA29222@zn.tnic>
Date:   Thu, 22 Oct 2020 11:30:44 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     x86-ml <x86@...nel.org>, Joerg Roedel <jroedel@...e.de>,
        lkml <linux-kernel@...r.kernel.org>
Subject: Re: [RFC] Have insn decoder functions return success/failure

On Thu, Oct 22, 2020 at 04:31:00PM +0900, Masami Hiramatsu wrote:
> No, insn_get_length() implies it decodes whole of the instruction.
> (yeah, we need an alias of that, something like insn_get_complete())

That's exactly what I'm trying to point out: the whole API is not
entirely wrong - it just needs a better naming and documentation. Now,
the implication that getting the length of the insn will give you a full
decode is a totally internal detail which users don't need and have to
know.

> I need insn.length too. Of course we can split it into 2 calls. But
> as I said, since the insn_get_length() implies it decodes all other
> parts, I just called it once.

Yes, I have noticed that and wrote about it further on. The intent was
to show that the API needs work.

> Hm, it is better to call insn_get_immediate() if it doesn't use length later.

Ok, so you see the problem. This thing wants to decode the whole insn -
that's what the function is called. But it reads like it does something
else.

> Would you mean we'd better have something like insn_get_until_immediate() ? 
> 
> Since the x86 instruction is CISC, we can not decode intermediate
> parts. The APIs follows that. If you are confused, I'm sorry about that.

No, I'm not confused - again, I'd like for the API to be properly
defined and callers should not have to care which parts of the insn they
need to decode in order to get something else they actually need.

So the main API should be: insn_decode_insn() or so and it should give
you everything you need.

If this succeeds, you can go poke at insn.<field> and you know you have
valid data there.

If there are specialized uses, you can call some of the insn_get_*
helpers if you're not interested in decoding the full insn.

But if simply calling insn_decode_insn() would give you everything and
that is not that expensive, we can do that - API simplicity.

What I don't want to have is calling insn_get_length() or so and then
inspecting the opcode bytes because that's totally non-transparent.

Thx.

-- 
Regards/Gruss,
    Boris.

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