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

[Masami Hiramatsu <mhiramat@kernel.org>]

On Thu, 22 Oct 2020 11:30:44 +0200
Borislav Petkov <bp@alien8.de> wrote:

> 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.

Ok, what names would you like to suggest? insn_get_complete()?

> > 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.

Sorry, I can not get what you point. We already have those APIs, 

extern void insn_init(struct insn *insn, const void *kaddr, int buf_len, int x86_64);
extern void insn_get_prefixes(struct insn *insn);
extern void insn_get_opcode(struct insn *insn);
extern void insn_get_modrm(struct insn *insn);
extern void insn_get_sib(struct insn *insn);
extern void insn_get_displacement(struct insn *insn);
extern void insn_get_immediate(struct insn *insn);
extern void insn_get_length(struct insn *insn);

As I agreed, that we may need an alias of insn_get_length(). But it seems
clear to me, if you need insn.immediate, you must call insn_get_immediate().

> 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.

Ah, so you meant that we don't need such a different insn_get_* APIs,
but a single insn_decode() API, which will decode all fields.
(IOW, alias of insn_init() and insn_get_length(), right?)

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

OK, agreed.

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

I rather like simple "insn_decode()" function, no need to repeat
insn again.

int insn_decode(struct insn *insn, const void *kaddr, int buf_len, bool x86_64);

> 
> 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.

OK, I agreed.

Thank you,

> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette


-- 
Masami Hiramatsu <mhiramat@kernel.org>