Re: [PATCH 5.10 017/130] x86/insn: Add an insn_decode() API

On Tue, Jul 12, 2022 at 04:04:29PM -0700, Florian Fainelli wrote:

On 7/12/22 11:37, Greg Kroah-Hartman wrote:

...

From: Borislav Petkov bp@suse.de

commit 93281c4a96572a34504244969b938e035204778d upstream.

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. When there's an error fetching the next insn byte and the insn falls short, return -ENODATA to denote that.

While at it, make insn_get_opcode() more stricter as to whether what has seen so far is a valid insn and if not.

Copy linux/kconfig.h for the tools-version of the decoder so that it can use IS_ENABLED().

Also, cast the INSN_MODE_KERN dummy define value to (enum insn_mode) for tools use of the decoder because perf tool builds with -Werror and errors out with -Werror=sign-compare otherwise.

Signed-off-by: Borislav Petkov bp@suse.de Acked-by: Masami Hiramatsu mhiramat@kernel.org Link: https://lkml.kernel.org/r/20210304174237.31945-5-bp@alien8.de Signed-off-by: Ben Hutchings ben@decadent.org.uk Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org

---

arch/x86/include/asm/insn.h | 24 ++-- arch/x86/lib/insn.c | 216 +++++++++++++++++++++++++++++------- tools/arch/x86/include/asm/insn.h | 24 ++-- tools/arch/x86/lib/insn.c | 222 +++++++++++++++++++++++++++++--------- tools/include/linux/kconfig.h | 73 ++++++++++++ 5 files changed, 452 insertions(+), 107 deletions(-) create mode 100644 tools/include/linux/kconfig.h

--- 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) --- a/arch/x86/lib/insn.c +++ b/arch/x86/lib/insn.c @@ -13,6 +13,9 @@ #include <asm/inat.h> /*__ignore_sync_check__ */ #include <asm/insn.h> /* __ignore_sync_check__ */ +#include <linux/errno.h> +#include <linux/kconfig.h>

• #include <asm/emulate_prefix.h> /* __ignore_sync_check__ */ /* Verify next sizeof(t) bytes can be on the same instruction */

@@ -97,8 +100,12 @@ static void insn_get_emulate_prefix(stru

• 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
  */

-void insn_get_prefixes(struct insn *insn) +int insn_get_prefixes(struct insn *insn) { struct insn_field *prefixes = &insn->prefixes; insn_attr_t attr; @@ -106,7 +113,7 @@ void insn_get_prefixes(struct insn *insn int i, nb; if (prefixes->got)

• return;
  

• return 0;
  

  insn_get_emulate_prefix(insn);

@@ -217,8 +224,10 @@ vex_end: prefixes->got = 1;

• return 0;
• err_out:

• return;

• return -ENODATA; } /**

@@ -230,16 +239,25 @@ err_out:

• If necessary, first collects any preceding (prefix) bytes.
• Sets @insn->opcode.value = opcode1. No effect if @insn->opcode.got
• is already 1.

• • Returns:
• • 0: on success
• • < 0: on error
  */

-void insn_get_opcode(struct insn *insn) +int insn_get_opcode(struct insn *insn) { struct insn_field *opcode = &insn->opcode;

• int pfx_id, ret; insn_byte_t op;

• int pfx_id;

• if (opcode->got)

• return;
  

• if (!insn->prefixes.got)

• insn_get_prefixes(insn);
  

• return 0;
  

• if (!insn->prefixes.got) {

• ret = insn_get_prefixes(insn);
  

• if (ret)
  

• 	return ret;
  

• } /* Get first opcode */ op = get_next(insn_byte_t, insn);

@@ -254,9 +272,13 @@ void insn_get_opcode(struct insn *insn) insn->attr = inat_get_avx_attribute(op, m, p); if ((inat_must_evex(insn->attr) && !insn_is_evex(insn)) || (!inat_accept_vex(insn->attr) &&

•      !inat_is_group(insn->attr)))
  

• 	insn->attr = 0;	/* This instruction is bad */
  

• goto end;	/* VEX has only 1 byte for opcode */
  

•      !inat_is_group(insn->attr))) {
  

• 	/* This instruction is bad */
  

• 	insn->attr = 0;
  

• 	return -EINVAL;
  

• }
  

• /* VEX has only 1 byte for opcode */
  

• goto end;
  

  } insn->attr = inat_get_opcode_attribute(op);

@@ -267,13 +289,18 @@ void insn_get_opcode(struct insn *insn) pfx_id = insn_last_prefix_id(insn); insn->attr = inat_get_escape_attribute(op, pfx_id, insn->attr); }

• if (inat_must_vex(insn->attr))

• insn->attr = 0;	/* This instruction is bad */
  

• if (inat_must_vex(insn->attr)) {

• /* This instruction is bad */
  

• insn->attr = 0;
  

• return -EINVAL;
  

• } end: opcode->got = 1;
• return 0; err_out:

• return;

• return -ENODATA; } /**

@@ -283,15 +310,25 @@ err_out:

• Populates @insn->modrm and updates @insn->next_byte to point past the
• ModRM byte, if any. If necessary, first collects the preceding bytes
• (prefixes and opcode(s)). No effect if @insn->modrm.got is already 1.

• • Returns:
• • 0: on success
• • < 0: on error
  */

-void insn_get_modrm(struct insn *insn) +int insn_get_modrm(struct insn *insn) { struct insn_field *modrm = &insn->modrm; insn_byte_t pfx_id, mod;

• int ret;
• if (modrm->got)

• return;
  

• if (!insn->opcode.got)

• insn_get_opcode(insn);
  

• return 0;
  

• if (!insn->opcode.got) {

• ret = insn_get_opcode(insn);
  

• if (ret)
  

• 	return ret;
  

• } if (inat_has_modrm(insn->attr)) { mod = get_next(insn_byte_t, insn);

@@ -301,17 +338,22 @@ void insn_get_modrm(struct insn *insn) pfx_id = insn_last_prefix_id(insn); insn->attr = inat_get_group_attribute(mod, pfx_id, insn->attr);

• 	if (insn_is_avx(insn) && !inat_accept_vex(insn->attr))
  

• 		insn->attr = 0;	/* This is bad */
  

• 	if (insn_is_avx(insn) && !inat_accept_vex(insn->attr)) {
  

• 		/* Bad insn */
  

• 		insn->attr = 0;
  

• 		return -EINVAL;
  

• 	}
  

  } } if (insn->x86_64 && inat_is_force64(insn->attr)) insn->opnd_bytes = 8;
• modrm->got = 1;
• return 0; err_out:

• return;

• return -ENODATA; }

@@ -325,11 +367,16 @@ err_out: int insn_rip_relative(struct insn *insn) { struct insn_field *modrm = &insn->modrm;

• int ret; if (!insn->x86_64) return 0;

• if (!modrm->got)

• insn_get_modrm(insn);
  

• if (!modrm->got) {

• ret = insn_get_modrm(insn);
  

• if (ret)
  

• 	return 0;
  

• } /*
  • For rip-relative instructions, the mod field (top 2 bits)
  • is zero and the r/m field (bottom 3 bits) is 0x5.

@@ -343,15 +390,25 @@ int insn_rip_relative(struct insn *insn)

• If necessary, first collects the instruction up to and including the
• ModRM byte.

• • Returns:
• • 0: if decoding succeeded
• • < 0: otherwise.
  */

-void insn_get_sib(struct insn *insn) +int insn_get_sib(struct insn *insn) { insn_byte_t modrm;

• int ret; if (insn->sib.got)

• return;
  

• if (!insn->modrm.got)

• insn_get_modrm(insn);
  

• return 0;
  

• if (!insn->modrm.got) {

• ret = insn_get_modrm(insn);
  

• if (ret)
  

• 	return ret;
  

• }
• if (insn->modrm.nbytes) { modrm = (insn_byte_t)insn->modrm.value; if (insn->addr_bytes != 2 &&

@@ -362,8 +419,10 @@ void insn_get_sib(struct insn *insn) } insn->sib.got = 1;

• return 0;
• err_out:

• return;

• return -ENODATA; }

@@ -374,15 +433,25 @@ err_out:

• If necessary, first collects the instruction up to and including the
• SIB byte.
• Displacement value is sign-expanded.

• • • Returns:
• • 0: if decoding succeeded
• • < 0: otherwise.
  */

-void insn_get_displacement(struct insn *insn) +int insn_get_displacement(struct insn *insn) { insn_byte_t mod, rm, base;

• int ret; if (insn->displacement.got)

• return;
  

• if (!insn->sib.got)

• insn_get_sib(insn);
  

• return 0;
  

• if (!insn->sib.got) {

• ret = insn_get_sib(insn);
  

• if (ret)
  

• 	return ret;
  

• }
• if (insn->modrm.nbytes) { /*
  • Interpreting the modrm byte:

@@ -425,9 +494,10 @@ void insn_get_displacement(struct insn * } out: insn->displacement.got = 1;

• return 0; err_out:

• return;

• return -ENODATA; } /* Decode moffset16/32/64. Return 0 if failed */

@@ -538,20 +608,30 @@ err_out: } /**

• • insn_get_immediate() - Get the immediates of instruction

• • insn_get_immediate() - Get the immediate in an instruction
  • @insn: &struct insn containing instruction
  • If necessary, first collects the instruction up to and including the
  • displacement bytes.
  • Basically, most of immediates are sign-expanded. Unsigned-value can be

• • get by bit masking with ((1 << (nbytes * 8)) - 1)

• • computed by bit masking with ((1 << (nbytes * 8)) - 1)
• • Returns:
• • 0: on success
• • < 0: on error
  */

-void insn_get_immediate(struct insn *insn) +int insn_get_immediate(struct insn *insn) {

• int ret;
• if (insn->immediate.got)

• return;
  

• if (!insn->displacement.got)

• insn_get_displacement(insn);
  

• return 0;
  

• if (!insn->displacement.got) {

• ret = insn_get_displacement(insn);
  

• if (ret)
  

• 	return ret;
  

• } if (inat_has_moffset(insn->attr)) { if (!__get_moffset(insn))

@@ -604,9 +684,10 @@ void insn_get_immediate(struct insn *ins } done: insn->immediate.got = 1;

• return 0; err_out:

• return;

• return -ENODATA; } /**

@@ -615,13 +696,58 @@ err_out:

• If necessary, first collects the instruction up to and including the
• immediates bytes.

• */

-void insn_get_length(struct insn *insn)

• • Returns:
• • • 0 on success
• • • < 0 on error

+*/ +int insn_get_length(struct insn *insn) {

• int ret;
• if (insn->length)

• return;
  

• if (!insn->immediate.got)

• insn_get_immediate(insn);
  

• return 0;
  

• if (!insn->immediate.got) {

• ret = insn_get_immediate(insn);
  

• if (ret)
  

• 	return ret;
  

• }
• insn->length = (unsigned char)((unsigned long)insn->next_byte - (unsigned long)insn->kaddr);
• return 0;

+}

+/**

• • 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.
• */

+int insn_decode(struct insn *insn, const void *kaddr, int buf_len, enum insn_mode m) +{

• int ret;

+/* #define INSN_MODE_KERN -1 __ignore_sync_check__ mode is only valid in the kernel */

• if (m == INSN_MODE_KERN)

• insn_init(insn, kaddr, buf_len, IS_ENABLED(CONFIG_X86_64));
  

• else

• insn_init(insn, kaddr, buf_len, m == INSN_MODE_64);
  

• ret = insn_get_length(insn);
• if (ret)

• return ret;
  

• if (insn_complete(insn))

• return 0;
  

• return -EINVAL; }

--- a/tools/arch/x86/include/asm/insn.h +++ b/tools/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) --- a/tools/arch/x86/lib/insn.c +++ b/tools/arch/x86/lib/insn.c @@ -10,10 +10,13 @@ #else #include <string.h> #endif -#include "../include/asm/inat.h" /* __ignore_sync_check__ */ -#include "../include/asm/insn.h" /* __ignore_sync_check__ */ +#include <asm/inat.h> /* __ignore_sync_check__ */ +#include <asm/insn.h> /* __ignore_sync_check__ */

These includes breaks the build for me with:

CC /local/users/fainelli/buildroot/output/arm64/build/linux-custom/tools/perf/util/intel-pt-decoder/intel-pt-insn-decoder.o In file included from util/intel-pt-decoder/intel-pt-insn-decoder.c:15: util/intel-pt-decoder/../../../arch/x86/lib/insn.c:13:10: fatal error: asm/inat.h: No such file or directory #include <asm/inat.h> /* __ignore_sync_check__ */ ^~~~~~~~~~~~ compilation terminated. make[7]: *** [util/intel-pt-decoder/Build:14: /local/users/fainelli/buildroot/output/arm64/build/linux-custom/tools/perf/util/intel-pt-decoder/intel-pt-insn-decoder.o] Error 1 make[6]: *** [/local/users/fainelli/buildroot/output/arm64/build/linux-custom/tools/build/Makefile.build:139: intel-pt-decoder] Error 2 make[5]: *** [/local/users/fainelli/buildroot/output/arm64/build/linux-custom/tools/build/Makefile.build:139: util] Error 2 make[4]: *** [Makefile.perf:643: /local/users/fainelli/buildroot/output/arm64/build/linux-custom/tools/perf/perf-in.o] Error 2 make[3]: *** [Makefile.perf:229: sub-make] Error 2 make[2]: *** [Makefile:70: all] Error 2 make[1]: *** [package/pkg-generic.mk:294: /local/users/fainelli/buildroot/output/arm64/build/linux-tools/.stamp_built] Error 2 make: *** [Makefile:27: _all] Error 2

It looks like you would also need to back port this:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...

Thanks for this, now queued up.

greg k-h