Re: [RFC PATCH v0 03/19] x86/insn: Add an insn_decode() API

Masami Hiramatsu
2020-11-25T16:53:54Z

Hi Borislav,

On Tue, 24 Nov 2020 11:19:36 +0100
Borislav Petkov <bp@alien8.de> wrote:

> From: Borislav Petkov <bp@suse.de>
>
> Users of the instruction decoder should use this to decode instruction
> bytes. For that, have insn*() helpers return an int value to denote
> success/failure.
>
> While at it, make insn_get_opcode() more stricter as to whether what has
> seen so far is a valid insn and if not.

(only from the viewpoint of VEX coding, a bit stricter, but not perfect.)

> Copy linux/kconfig.h for the tools-version of the decoder so that it can
> use IS_ENABLED().
>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
> arch/x86/include/asm/insn.h | 24 ++--
> arch/x86/lib/insn.c | 182 +++++++++++++++++++++++-------
> tools/arch/x86/include/asm/insn.h | 24 ++--
> tools/arch/x86/lib/insn.c | 182 +++++++++++++++++++++++-------
> tools/include/linux/kconfig.h | 73 ++++++++++++
> 5 files changed, 391 insertions(+), 94 deletions(-)
> create mode 100644 tools/include/linux/kconfig.h
>
> diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
> index 5c1ae3eff9d4..e80ddfe86255 100644
> --- a/arch/x86/include/asm/insn.h
> +++ b/arch/x86/include/asm/insn.h
> @@ -87,13 +87,23 @@ struct insn {
> #define X86_VEX_M_MAX 0x1f /* VEX3.M Maximum value */
>
> 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);
> +extern int insn_get_prefixes(struct insn *insn);
> +extern int insn_get_opcode(struct insn *insn);
> +extern int insn_get_modrm(struct insn *insn);
> +extern int insn_get_sib(struct insn *insn);
> +extern int insn_get_displacement(struct insn *insn);
> +extern int insn_get_immediate(struct insn *insn);
> +extern int insn_get_length(struct insn *insn);
> +
> +enum insn_mode {
> + INSN_MODE_32,
> + INSN_MODE_64,
> + /* Mode is determined by the current kernel build. */
> + INSN_MODE_KERN,
> + INSN_NUM_MODES,
> +};
> +
> +extern int insn_decode(struct insn *insn, const void *kaddr, int buf_len, enum insn_mode m);
>
> /* Attribute will be determined after getting ModRM (for opcode groups) */
> static inline void insn_get_attribute(struct insn *insn)
> diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
> index 1ba994862b56..1f749e3b4a25 100644
> --- a/arch/x86/lib/insn.c
> +++ b/arch/x86/lib/insn.c
> @@ -13,6 +13,9 @@
> #include <asm/inat.h>
> #include <asm/insn.h>
>
> +#include <linux/errno.h>
> +#include <linux/kconfig.h>
> +
> #include <asm/emulate_prefix.h>
>
> /* Verify next sizeof(t) bytes can be on the same instruction */
> @@ -98,8 +101,12 @@ static void insn_get_emulate_prefix(struct insn *insn)
> * Populates the @insn->prefixes bitmap, and updates @insn->next_byte
> * to point to the (first) opcode. No effect if @insn->prefixes.got
> * is already set.
> + *
> + * * Returns:
> + * 0: on success
> + * !0: on error
> */

So this is different from...

[..]
> +
> +/**
> + * insn_decode() - Decode an x86 instruction
> + * @insn: &struct insn to be initialized
> + * @kaddr: address (in kernel memory) of instruction (or copy thereof)
> + * @buf_len: length of the insn buffer at @kaddr
> + * @m: insn mode, see enum insn_mode
> + *
> + * Returns:
> + * 0: if decoding succeeded
> + * < 0: otherwise.

this return value.

Even for the insn_get_*(), I would like to see them returning -EINVAL
as same as insn_decode(). Same API group has different return value is
confusing.

Thank you,

--
Masami Hiramatsu <mhiramat@kernel.org>