* [PATCH v1 01/19] x86/insn: Rename insn_decode() to insn_decode_regs()
2020-12-23 17:42 [PATCH v1 00/19] x86/insn: Add an insn_decode() API Borislav Petkov
@ 2020-12-23 17:42 ` Borislav Petkov
2020-12-28 17:16 ` Sean Christopherson
2020-12-23 17:42 ` [PATCH v1 02/19] x86/insn: Add @buf_len param to insn_init() kernel-doc comment Borislav Petkov
` (18 subsequent siblings)
19 siblings, 1 reply; 42+ messages in thread
From: Borislav Petkov @ 2020-12-23 17:42 UTC (permalink / raw)
To: Andy Lutomirski, Masami Hiramatsu; +Cc: X86 ML, LKML
From: Borislav Petkov <bp@suse.de>
Rename insn_decode() to insn_decode_regs() to denote that it receives
regs as param and free the name for a more generic version of the
function.
No functional changes.
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/include/asm/insn-eval.h | 4 ++--
arch/x86/kernel/sev-es.c | 2 +-
arch/x86/kernel/umip.c | 2 +-
arch/x86/lib/insn-eval.c | 6 +++---
4 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/arch/x86/include/asm/insn-eval.h b/arch/x86/include/asm/insn-eval.h
index a0f839aa144d..3797497a9270 100644
--- a/arch/x86/include/asm/insn-eval.h
+++ b/arch/x86/include/asm/insn-eval.h
@@ -23,7 +23,7 @@ unsigned long insn_get_seg_base(struct pt_regs *regs, int seg_reg_idx);
int insn_get_code_seg_params(struct pt_regs *regs);
int insn_fetch_from_user(struct pt_regs *regs,
unsigned char buf[MAX_INSN_SIZE]);
-bool insn_decode(struct insn *insn, struct pt_regs *regs,
- unsigned char buf[MAX_INSN_SIZE], int buf_size);
+bool insn_decode_regs(struct insn *insn, struct pt_regs *regs,
+ unsigned char buf[MAX_INSN_SIZE], int buf_size);
#endif /* _ASM_X86_INSN_EVAL_H */
diff --git a/arch/x86/kernel/sev-es.c b/arch/x86/kernel/sev-es.c
index 0bd1a0fc587e..37736486603e 100644
--- a/arch/x86/kernel/sev-es.c
+++ b/arch/x86/kernel/sev-es.c
@@ -256,7 +256,7 @@ static enum es_result vc_decode_insn(struct es_em_ctxt *ctxt)
return ES_EXCEPTION;
}
- if (!insn_decode(&ctxt->insn, ctxt->regs, buffer, res))
+ if (!insn_decode_regs(&ctxt->insn, ctxt->regs, buffer, res))
return ES_DECODE_FAILED;
} else {
res = vc_fetch_insn_kernel(ctxt, buffer);
diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
index f6225bf22c02..e3584894b074 100644
--- a/arch/x86/kernel/umip.c
+++ b/arch/x86/kernel/umip.c
@@ -356,7 +356,7 @@ bool fixup_umip_exception(struct pt_regs *regs)
if (!nr_copied)
return false;
- if (!insn_decode(&insn, regs, buf, nr_copied))
+ if (!insn_decode_regs(&insn, regs, buf, nr_copied))
return false;
umip_inst = identify_insn(&insn);
diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
index 4229950a5d78..265d23a0c334 100644
--- a/arch/x86/lib/insn-eval.c
+++ b/arch/x86/lib/insn-eval.c
@@ -1454,7 +1454,7 @@ int insn_fetch_from_user(struct pt_regs *regs, unsigned char buf[MAX_INSN_SIZE])
}
/**
- * insn_decode() - Decode an instruction
+ * insn_decode_regs() - Decode an instruction
* @insn: Structure to store decoded instruction
* @regs: Structure with register values as seen when entering kernel mode
* @buf: Buffer containing the instruction bytes
@@ -1467,8 +1467,8 @@ int insn_fetch_from_user(struct pt_regs *regs, unsigned char buf[MAX_INSN_SIZE])
*
* True if instruction was decoded, False otherwise.
*/
-bool insn_decode(struct insn *insn, struct pt_regs *regs,
- unsigned char buf[MAX_INSN_SIZE], int buf_size)
+bool insn_decode_regs(struct insn *insn, struct pt_regs *regs,
+ unsigned char buf[MAX_INSN_SIZE], int buf_size)
{
int seg_defs;
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v1 01/19] x86/insn: Rename insn_decode() to insn_decode_regs()
2020-12-23 17:42 ` [PATCH v1 01/19] x86/insn: Rename insn_decode() to insn_decode_regs() Borislav Petkov
@ 2020-12-28 17:16 ` Sean Christopherson
2020-12-29 19:36 ` Borislav Petkov
0 siblings, 1 reply; 42+ messages in thread
From: Sean Christopherson @ 2020-12-28 17:16 UTC (permalink / raw)
To: Borislav Petkov; +Cc: Andy Lutomirski, Masami Hiramatsu, X86 ML, LKML
On Wed, Dec 23, 2020, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
>
> Rename insn_decode() to insn_decode_regs() to denote that it receives
> regs as param and free the name for a more generic version of the
> function.
Can we add a preposition in there, e.g. insn_decode_from_regs() or
insn_decode_with_regs()? For me, "decode_regs" means "decode the register
operands of the instruction".
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v1 01/19] x86/insn: Rename insn_decode() to insn_decode_regs()
2020-12-28 17:16 ` Sean Christopherson
@ 2020-12-29 19:36 ` Borislav Petkov
0 siblings, 0 replies; 42+ messages in thread
From: Borislav Petkov @ 2020-12-29 19:36 UTC (permalink / raw)
To: Sean Christopherson; +Cc: Andy Lutomirski, Masami Hiramatsu, X86 ML, LKML
On Mon, Dec 28, 2020 at 09:16:50AM -0800, Sean Christopherson wrote:
> Can we add a preposition in there, e.g. insn_decode_from_regs() or
> insn_decode_with_regs()? For me, "decode_regs" means "decode the register
> operands of the instruction".
"...from_regs" it is.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v1 02/19] x86/insn: Add @buf_len param to insn_init() kernel-doc comment
2020-12-23 17:42 [PATCH v1 00/19] x86/insn: Add an insn_decode() API Borislav Petkov
2020-12-23 17:42 ` [PATCH v1 01/19] x86/insn: Rename insn_decode() to insn_decode_regs() Borislav Petkov
@ 2020-12-23 17:42 ` Borislav Petkov
2020-12-28 1:44 ` Masami Hiramatsu
2020-12-23 17:42 ` [PATCH v1 03/19] x86/insn: Add an insn_decode() API Borislav Petkov
` (17 subsequent siblings)
19 siblings, 1 reply; 42+ messages in thread
From: Borislav Petkov @ 2020-12-23 17:42 UTC (permalink / raw)
To: Andy Lutomirski, Masami Hiramatsu; +Cc: X86 ML, LKML
From: Borislav Petkov <bp@suse.de>
It wasn't documented so add it. No functional changes.
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/lib/insn.c | 1 +
tools/arch/x86/lib/insn.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
index 404279563891..1ba994862b56 100644
--- a/arch/x86/lib/insn.c
+++ b/arch/x86/lib/insn.c
@@ -37,6 +37,7 @@
* insn_init() - initialize struct insn
* @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
* @x86_64: !0 for 64-bit kernel or 64-bit app
*/
void insn_init(struct insn *insn, const void *kaddr, int buf_len, int x86_64)
diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c
index 0151dfc6da61..f3277d6e4ef2 100644
--- a/tools/arch/x86/lib/insn.c
+++ b/tools/arch/x86/lib/insn.c
@@ -37,6 +37,7 @@
* insn_init() - initialize struct insn
* @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
* @x86_64: !0 for 64-bit kernel or 64-bit app
*/
void insn_init(struct insn *insn, const void *kaddr, int buf_len, int x86_64)
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v1 02/19] x86/insn: Add @buf_len param to insn_init() kernel-doc comment
2020-12-23 17:42 ` [PATCH v1 02/19] x86/insn: Add @buf_len param to insn_init() kernel-doc comment Borislav Petkov
@ 2020-12-28 1:44 ` Masami Hiramatsu
0 siblings, 0 replies; 42+ messages in thread
From: Masami Hiramatsu @ 2020-12-28 1:44 UTC (permalink / raw)
To: Borislav Petkov; +Cc: Andy Lutomirski, X86 ML, LKML
On Wed, 23 Dec 2020 18:42:16 +0100
Borislav Petkov <bp@alien8.de> wrote:
> From: Borislav Petkov <bp@suse.de>
>
> It wasn't documented so add it. No functional changes.
>
Thank you for fixing!
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
> arch/x86/lib/insn.c | 1 +
> tools/arch/x86/lib/insn.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
> index 404279563891..1ba994862b56 100644
> --- a/arch/x86/lib/insn.c
> +++ b/arch/x86/lib/insn.c
> @@ -37,6 +37,7 @@
> * insn_init() - initialize struct insn
> * @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
> * @x86_64: !0 for 64-bit kernel or 64-bit app
> */
> void insn_init(struct insn *insn, const void *kaddr, int buf_len, int x86_64)
> diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c
> index 0151dfc6da61..f3277d6e4ef2 100644
> --- a/tools/arch/x86/lib/insn.c
> +++ b/tools/arch/x86/lib/insn.c
> @@ -37,6 +37,7 @@
> * insn_init() - initialize struct insn
> * @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
> * @x86_64: !0 for 64-bit kernel or 64-bit app
> */
> void insn_init(struct insn *insn, const void *kaddr, int buf_len, int x86_64)
> --
> 2.29.2
>
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v1 03/19] x86/insn: Add an insn_decode() API
2020-12-23 17:42 [PATCH v1 00/19] x86/insn: Add an insn_decode() API Borislav Petkov
2020-12-23 17:42 ` [PATCH v1 01/19] x86/insn: Rename insn_decode() to insn_decode_regs() Borislav Petkov
2020-12-23 17:42 ` [PATCH v1 02/19] x86/insn: Add @buf_len param to insn_init() kernel-doc comment Borislav Petkov
@ 2020-12-23 17:42 ` Borislav Petkov
2020-12-28 1:15 ` Masami Hiramatsu
2020-12-23 17:42 ` [PATCH v1 04/19] x86/insn-eval: Handle return values from the decoder Borislav Petkov
` (16 subsequent siblings)
19 siblings, 1 reply; 42+ messages in thread
From: Borislav Petkov @ 2020-12-23 17:42 UTC (permalink / raw)
To: Andy Lutomirski, Masami Hiramatsu; +Cc: X86 ML, LKML
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. 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().
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/include/asm/insn.h | 24 ++-
arch/x86/lib/insn.c | 239 +++++++++++++++++++++++-------
tools/arch/x86/include/asm/insn.h | 24 ++-
tools/arch/x86/lib/insn.c | 239 +++++++++++++++++++++++-------
tools/include/linux/kconfig.h | 73 +++++++++
5 files changed, 475 insertions(+), 124 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 a8c3d284fa46..088aa90e9158 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..b373aadcdd02 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 */
@@ -80,12 +83,15 @@ static int __insn_get_emulate_prefix(struct insn *insn,
return 1;
err_out:
- return 0;
+ return -ENODATA;
}
static void insn_get_emulate_prefix(struct insn *insn)
{
- if (__insn_get_emulate_prefix(insn, xen_prefix, sizeof(xen_prefix)))
+ int ret;
+
+ ret = __insn_get_emulate_prefix(insn, xen_prefix, sizeof(xen_prefix));
+ if (ret > 0)
return;
__insn_get_emulate_prefix(insn, kvm_prefix, sizeof(kvm_prefix));
@@ -98,8 +104,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
*/
-void insn_get_prefixes(struct insn *insn)
+int insn_get_prefixes(struct insn *insn)
{
struct insn_field *prefixes = &insn->prefixes;
insn_attr_t attr;
@@ -107,7 +117,7 @@ void insn_get_prefixes(struct insn *insn)
int i, nb;
if (prefixes->got)
- return;
+ return 0;
insn_get_emulate_prefix(insn);
@@ -218,8 +228,10 @@ void insn_get_prefixes(struct insn *insn)
prefixes->got = 1;
+ return 0;
+
err_out:
- return;
+ return -ENODATA;
}
/**
@@ -231,16 +243,25 @@ void insn_get_prefixes(struct insn *insn)
* 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);
@@ -255,9 +276,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 1;
+ }
+ /* VEX has only 1 byte for opcode */
+ goto end;
}
insn->attr = inat_get_opcode_attribute(op);
@@ -268,13 +293,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 1;
+ }
end:
opcode->got = 1;
+ return 0;
err_out:
- return;
+ return -ENODATA;
}
/**
@@ -284,15 +314,25 @@ void insn_get_opcode(struct insn *insn)
* 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);
@@ -302,17 +342,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 1;
+ }
}
}
if (insn->x86_64 && inat_is_force64(insn->attr))
insn->opnd_bytes = 8;
+
modrm->got = 1;
+ return 0;
err_out:
- return;
+ return -ENODATA;
}
@@ -326,11 +371,16 @@ void insn_get_modrm(struct insn *insn)
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 ret;
+ }
/*
* For rip-relative instructions, the mod field (top 2 bits)
* is zero and the r/m field (bottom 3 bits) is 0x5.
@@ -344,15 +394,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 &&
@@ -363,8 +423,10 @@ void insn_get_sib(struct insn *insn)
}
insn->sib.got = 1;
+ return 0;
+
err_out:
- return;
+ return -ENODATA;
}
@@ -375,15 +437,25 @@ void insn_get_sib(struct insn *insn)
* 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:
@@ -426,12 +498,13 @@ void insn_get_displacement(struct insn *insn)
}
out:
insn->displacement.got = 1;
+ return 0;
err_out:
- return;
+ return -ENODATA;
}
-/* Decode moffset16/32/64. Return 0 if failed */
+/* Decode moffset16/32/64. Return a negative value if failed. */
static int __get_moffset(struct insn *insn)
{
switch (insn->addr_bytes) {
@@ -457,10 +530,10 @@ static int __get_moffset(struct insn *insn)
return 1;
err_out:
- return 0;
+ return -ENODATA;
}
-/* Decode imm v32(Iz). Return 0 if failed */
+/* Decode imm v32(Iz). Return a negative value if failed. */
static int __get_immv32(struct insn *insn)
{
switch (insn->opnd_bytes) {
@@ -480,10 +553,10 @@ static int __get_immv32(struct insn *insn)
return 1;
err_out:
- return 0;
+ return -ENODATA;
}
-/* Decode imm v64(Iv/Ov), Return 0 if failed */
+/* Decode imm v64(Iv/Ov). Return a negative value if failed. */
static int __get_immv(struct insn *insn)
{
switch (insn->opnd_bytes) {
@@ -507,11 +580,12 @@ static int __get_immv(struct insn *insn)
insn->immediate1.got = insn->immediate2.got = 1;
return 1;
+
err_out:
- return 0;
+ return -ENODATA;
}
-/* Decode ptr16:16/32(Ap) */
+/* Decode ptr16:16/32(Ap). Return a negative value if failed. */
static int __get_immptr(struct insn *insn)
{
switch (insn->opnd_bytes) {
@@ -534,25 +608,36 @@ static int __get_immptr(struct insn *insn)
insn->immediate1.got = insn->immediate2.got = 1;
return 1;
+
err_out:
- return 0;
+ return -ENODATA;
}
/**
- * 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))
@@ -605,9 +690,10 @@ void insn_get_immediate(struct insn *insn)
}
done:
insn->immediate.got = 1;
+ return 0;
err_out:
- return;
+ return -ENODATA;
}
/**
@@ -616,13 +702,56 @@ void insn_get_immediate(struct insn *insn)
*
* 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;
+
+ 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;
}
diff --git a/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
index 52c6262e6bfd..ed43bf01ebcc 100644
--- 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)
diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c
index f3277d6e4ef2..a2f9d9e177f2 100644
--- a/tools/arch/x86/lib/insn.c
+++ b/tools/arch/x86/lib/insn.c
@@ -13,6 +13,9 @@
#include "../include/asm/inat.h"
#include "../include/asm/insn.h"
+#include <linux/errno.h>
+#include <linux/kconfig.h>
+
#include "../include/asm/emulate_prefix.h"
/* Verify next sizeof(t) bytes can be on the same instruction */
@@ -80,12 +83,15 @@ static int __insn_get_emulate_prefix(struct insn *insn,
return 1;
err_out:
- return 0;
+ return -ENODATA;
}
static void insn_get_emulate_prefix(struct insn *insn)
{
- if (__insn_get_emulate_prefix(insn, xen_prefix, sizeof(xen_prefix)))
+ int ret;
+
+ ret = __insn_get_emulate_prefix(insn, xen_prefix, sizeof(xen_prefix));
+ if (ret > 0)
return;
__insn_get_emulate_prefix(insn, kvm_prefix, sizeof(kvm_prefix));
@@ -98,8 +104,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
*/
-void insn_get_prefixes(struct insn *insn)
+int insn_get_prefixes(struct insn *insn)
{
struct insn_field *prefixes = &insn->prefixes;
insn_attr_t attr;
@@ -107,7 +117,7 @@ void insn_get_prefixes(struct insn *insn)
int i, nb;
if (prefixes->got)
- return;
+ return 0;
insn_get_emulate_prefix(insn);
@@ -218,8 +228,10 @@ void insn_get_prefixes(struct insn *insn)
prefixes->got = 1;
+ return 0;
+
err_out:
- return;
+ return -ENODATA;
}
/**
@@ -231,16 +243,25 @@ void insn_get_prefixes(struct insn *insn)
* 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);
@@ -255,9 +276,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 1;
+ }
+ /* VEX has only 1 byte for opcode */
+ goto end;
}
insn->attr = inat_get_opcode_attribute(op);
@@ -268,13 +293,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 1;
+ }
end:
opcode->got = 1;
+ return 0;
err_out:
- return;
+ return -ENODATA;
}
/**
@@ -284,15 +314,25 @@ void insn_get_opcode(struct insn *insn)
* 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);
@@ -302,17 +342,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 1;
+ }
}
}
if (insn->x86_64 && inat_is_force64(insn->attr))
insn->opnd_bytes = 8;
+
modrm->got = 1;
+ return 0;
err_out:
- return;
+ return -ENODATA;
}
@@ -326,11 +371,16 @@ void insn_get_modrm(struct insn *insn)
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 ret;
+ }
/*
* For rip-relative instructions, the mod field (top 2 bits)
* is zero and the r/m field (bottom 3 bits) is 0x5.
@@ -344,15 +394,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 &&
@@ -363,8 +423,10 @@ void insn_get_sib(struct insn *insn)
}
insn->sib.got = 1;
+ return 0;
+
err_out:
- return;
+ return -ENODATA;
}
@@ -375,15 +437,25 @@ void insn_get_sib(struct insn *insn)
* 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:
@@ -426,12 +498,13 @@ void insn_get_displacement(struct insn *insn)
}
out:
insn->displacement.got = 1;
+ return 0;
err_out:
- return;
+ return -ENODATA;
}
-/* Decode moffset16/32/64. Return 0 if failed */
+/* Decode moffset16/32/64. Return a negative value if failed. */
static int __get_moffset(struct insn *insn)
{
switch (insn->addr_bytes) {
@@ -457,10 +530,10 @@ static int __get_moffset(struct insn *insn)
return 1;
err_out:
- return 0;
+ return -ENODATA;
}
-/* Decode imm v32(Iz). Return 0 if failed */
+/* Decode imm v32(Iz). Return a negative value if failed. */
static int __get_immv32(struct insn *insn)
{
switch (insn->opnd_bytes) {
@@ -480,10 +553,10 @@ static int __get_immv32(struct insn *insn)
return 1;
err_out:
- return 0;
+ return -ENODATA;
}
-/* Decode imm v64(Iv/Ov), Return 0 if failed */
+/* Decode imm v64(Iv/Ov). Return a negative value if failed. */
static int __get_immv(struct insn *insn)
{
switch (insn->opnd_bytes) {
@@ -507,11 +580,12 @@ static int __get_immv(struct insn *insn)
insn->immediate1.got = insn->immediate2.got = 1;
return 1;
+
err_out:
- return 0;
+ return -ENODATA;
}
-/* Decode ptr16:16/32(Ap) */
+/* Decode ptr16:16/32(Ap). Return a negative value if failed. */
static int __get_immptr(struct insn *insn)
{
switch (insn->opnd_bytes) {
@@ -534,25 +608,36 @@ static int __get_immptr(struct insn *insn)
insn->immediate1.got = insn->immediate2.got = 1;
return 1;
+
err_out:
- return 0;
+ return -ENODATA;
}
/**
- * 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))
@@ -605,9 +690,10 @@ void insn_get_immediate(struct insn *insn)
}
done:
insn->immediate.got = 1;
+ return 0;
err_out:
- return;
+ return -ENODATA;
}
/**
@@ -616,13 +702,56 @@ void insn_get_immediate(struct insn *insn)
*
* 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;
+
+ 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;
}
diff --git a/tools/include/linux/kconfig.h b/tools/include/linux/kconfig.h
new file mode 100644
index 000000000000..1555a0c4f345
--- /dev/null
+++ b/tools/include/linux/kconfig.h
@@ -0,0 +1,73 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _TOOLS_LINUX_KCONFIG_H
+#define _TOOLS_LINUX_KCONFIG_H
+
+/* CONFIG_CC_VERSION_TEXT (Do not delete this comment. See help in Kconfig) */
+
+#ifdef CONFIG_CPU_BIG_ENDIAN
+#define __BIG_ENDIAN 4321
+#else
+#define __LITTLE_ENDIAN 1234
+#endif
+
+#define __ARG_PLACEHOLDER_1 0,
+#define __take_second_arg(__ignored, val, ...) val
+
+/*
+ * The use of "&&" / "||" is limited in certain expressions.
+ * The following enable to calculate "and" / "or" with macro expansion only.
+ */
+#define __and(x, y) ___and(x, y)
+#define ___and(x, y) ____and(__ARG_PLACEHOLDER_##x, y)
+#define ____and(arg1_or_junk, y) __take_second_arg(arg1_or_junk y, 0)
+
+#define __or(x, y) ___or(x, y)
+#define ___or(x, y) ____or(__ARG_PLACEHOLDER_##x, y)
+#define ____or(arg1_or_junk, y) __take_second_arg(arg1_or_junk 1, y)
+
+/*
+ * Helper macros to use CONFIG_ options in C/CPP expressions. Note that
+ * these only work with boolean and tristate options.
+ */
+
+/*
+ * Getting something that works in C and CPP for an arg that may or may
+ * not be defined is tricky. Here, if we have "#define CONFIG_BOOGER 1"
+ * we match on the placeholder define, insert the "0," for arg1 and generate
+ * the triplet (0, 1, 0). Then the last step cherry picks the 2nd arg (a one).
+ * When CONFIG_BOOGER is not defined, we generate a (... 1, 0) pair, and when
+ * the last step cherry picks the 2nd arg, we get a zero.
+ */
+#define __is_defined(x) ___is_defined(x)
+#define ___is_defined(val) ____is_defined(__ARG_PLACEHOLDER_##val)
+#define ____is_defined(arg1_or_junk) __take_second_arg(arg1_or_junk 1, 0)
+
+/*
+ * IS_BUILTIN(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y', 0
+ * otherwise. For boolean options, this is equivalent to
+ * IS_ENABLED(CONFIG_FOO).
+ */
+#define IS_BUILTIN(option) __is_defined(option)
+
+/*
+ * IS_MODULE(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'm', 0
+ * otherwise.
+ */
+#define IS_MODULE(option) __is_defined(option##_MODULE)
+
+/*
+ * IS_REACHABLE(CONFIG_FOO) evaluates to 1 if the currently compiled
+ * code can call a function defined in code compiled based on CONFIG_FOO.
+ * This is similar to IS_ENABLED(), but returns false when invoked from
+ * built-in code when CONFIG_FOO is set to 'm'.
+ */
+#define IS_REACHABLE(option) __or(IS_BUILTIN(option), \
+ __and(IS_MODULE(option), __is_defined(MODULE)))
+
+/*
+ * IS_ENABLED(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y' or 'm',
+ * 0 otherwise.
+ */
+#define IS_ENABLED(option) __or(IS_BUILTIN(option), IS_MODULE(option))
+
+#endif /* _TOOLS_LINUX_KCONFIG_H */
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v1 03/19] x86/insn: Add an insn_decode() API
2020-12-23 17:42 ` [PATCH v1 03/19] x86/insn: Add an insn_decode() API Borislav Petkov
@ 2020-12-28 1:15 ` Masami Hiramatsu
2020-12-29 20:06 ` Borislav Petkov
0 siblings, 1 reply; 42+ messages in thread
From: Masami Hiramatsu @ 2020-12-28 1:15 UTC (permalink / raw)
To: Borislav Petkov; +Cc: Andy Lutomirski, X86 ML, LKML
Hi,
On Wed, 23 Dec 2020 18:42:17 +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. When there's an error fetching the next insn byte and
> the insn falls short, return -ENODATA to denote that.
-ENODATA sounds good to me :)
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
BTW, insn_decode() can return -EINVAL if !insn_complete(), is that OK?
>
> 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().
I think tools clone code must not use INSN_MODE_KERN because the tools may
not use kernel Kconfig.
Hmm, this may be better to make a different patch to introduce a NOSYNC tag
for sync checker in the tools. Something like;
> +int insn_decode(struct insn *insn, const void *kaddr, int buf_len, enum insn_mode m)
> +{
> + int ret;
> +
> + if (m == INSN_MODE_KERN)
return -EINVAL; /* NOSYNC */
> + else
> + insn_init(insn, kaddr, buf_len, m == INSN_MODE_64);
> +
Then we can simple file list for sync-check.sh.
Thank you,
>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
> arch/x86/include/asm/insn.h | 24 ++-
> arch/x86/lib/insn.c | 239 +++++++++++++++++++++++-------
> tools/arch/x86/include/asm/insn.h | 24 ++-
> tools/arch/x86/lib/insn.c | 239 +++++++++++++++++++++++-------
> tools/include/linux/kconfig.h | 73 +++++++++
> 5 files changed, 475 insertions(+), 124 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 a8c3d284fa46..088aa90e9158 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..b373aadcdd02 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 */
> @@ -80,12 +83,15 @@ static int __insn_get_emulate_prefix(struct insn *insn,
> return 1;
>
> err_out:
> - return 0;
> + return -ENODATA;
> }
>
> static void insn_get_emulate_prefix(struct insn *insn)
> {
> - if (__insn_get_emulate_prefix(insn, xen_prefix, sizeof(xen_prefix)))
> + int ret;
> +
> + ret = __insn_get_emulate_prefix(insn, xen_prefix, sizeof(xen_prefix));
> + if (ret > 0)
> return;
>
> __insn_get_emulate_prefix(insn, kvm_prefix, sizeof(kvm_prefix));
> @@ -98,8 +104,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
> */
> -void insn_get_prefixes(struct insn *insn)
> +int insn_get_prefixes(struct insn *insn)
> {
> struct insn_field *prefixes = &insn->prefixes;
> insn_attr_t attr;
> @@ -107,7 +117,7 @@ void insn_get_prefixes(struct insn *insn)
> int i, nb;
>
> if (prefixes->got)
> - return;
> + return 0;
>
> insn_get_emulate_prefix(insn);
>
> @@ -218,8 +228,10 @@ void insn_get_prefixes(struct insn *insn)
>
> prefixes->got = 1;
>
> + return 0;
> +
> err_out:
> - return;
> + return -ENODATA;
> }
>
> /**
> @@ -231,16 +243,25 @@ void insn_get_prefixes(struct insn *insn)
> * 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);
> @@ -255,9 +276,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 1;
> + }
> + /* VEX has only 1 byte for opcode */
> + goto end;
> }
>
> insn->attr = inat_get_opcode_attribute(op);
> @@ -268,13 +293,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 1;
> + }
> end:
> opcode->got = 1;
> + return 0;
>
> err_out:
> - return;
> + return -ENODATA;
> }
>
> /**
> @@ -284,15 +314,25 @@ void insn_get_opcode(struct insn *insn)
> * 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);
> @@ -302,17 +342,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 1;
> + }
> }
> }
>
> if (insn->x86_64 && inat_is_force64(insn->attr))
> insn->opnd_bytes = 8;
> +
> modrm->got = 1;
> + return 0;
>
> err_out:
> - return;
> + return -ENODATA;
> }
>
>
> @@ -326,11 +371,16 @@ void insn_get_modrm(struct insn *insn)
> 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 ret;
> + }
> /*
> * For rip-relative instructions, the mod field (top 2 bits)
> * is zero and the r/m field (bottom 3 bits) is 0x5.
> @@ -344,15 +394,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 &&
> @@ -363,8 +423,10 @@ void insn_get_sib(struct insn *insn)
> }
> insn->sib.got = 1;
>
> + return 0;
> +
> err_out:
> - return;
> + return -ENODATA;
> }
>
>
> @@ -375,15 +437,25 @@ void insn_get_sib(struct insn *insn)
> * 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:
> @@ -426,12 +498,13 @@ void insn_get_displacement(struct insn *insn)
> }
> out:
> insn->displacement.got = 1;
> + return 0;
>
> err_out:
> - return;
> + return -ENODATA;
> }
>
> -/* Decode moffset16/32/64. Return 0 if failed */
> +/* Decode moffset16/32/64. Return a negative value if failed. */
> static int __get_moffset(struct insn *insn)
> {
> switch (insn->addr_bytes) {
> @@ -457,10 +530,10 @@ static int __get_moffset(struct insn *insn)
> return 1;
>
> err_out:
> - return 0;
> + return -ENODATA;
> }
>
> -/* Decode imm v32(Iz). Return 0 if failed */
> +/* Decode imm v32(Iz). Return a negative value if failed. */
> static int __get_immv32(struct insn *insn)
> {
> switch (insn->opnd_bytes) {
> @@ -480,10 +553,10 @@ static int __get_immv32(struct insn *insn)
> return 1;
>
> err_out:
> - return 0;
> + return -ENODATA;
> }
>
> -/* Decode imm v64(Iv/Ov), Return 0 if failed */
> +/* Decode imm v64(Iv/Ov). Return a negative value if failed. */
> static int __get_immv(struct insn *insn)
> {
> switch (insn->opnd_bytes) {
> @@ -507,11 +580,12 @@ static int __get_immv(struct insn *insn)
> insn->immediate1.got = insn->immediate2.got = 1;
>
> return 1;
> +
> err_out:
> - return 0;
> + return -ENODATA;
> }
>
> -/* Decode ptr16:16/32(Ap) */
> +/* Decode ptr16:16/32(Ap). Return a negative value if failed. */
> static int __get_immptr(struct insn *insn)
> {
> switch (insn->opnd_bytes) {
> @@ -534,25 +608,36 @@ static int __get_immptr(struct insn *insn)
> insn->immediate1.got = insn->immediate2.got = 1;
>
> return 1;
> +
> err_out:
> - return 0;
> + return -ENODATA;
> }
>
> /**
> - * 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))
> @@ -605,9 +690,10 @@ void insn_get_immediate(struct insn *insn)
> }
> done:
> insn->immediate.got = 1;
> + return 0;
>
> err_out:
> - return;
> + return -ENODATA;
> }
>
> /**
> @@ -616,13 +702,56 @@ void insn_get_immediate(struct insn *insn)
> *
> * 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;
> +
> + 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;
> }
> diff --git a/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
> index 52c6262e6bfd..ed43bf01ebcc 100644
> --- 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)
> diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c
> index f3277d6e4ef2..a2f9d9e177f2 100644
> --- a/tools/arch/x86/lib/insn.c
> +++ b/tools/arch/x86/lib/insn.c
> @@ -13,6 +13,9 @@
> #include "../include/asm/inat.h"
> #include "../include/asm/insn.h"
>
> +#include <linux/errno.h>
> +#include <linux/kconfig.h>
> +
> #include "../include/asm/emulate_prefix.h"
>
> /* Verify next sizeof(t) bytes can be on the same instruction */
> @@ -80,12 +83,15 @@ static int __insn_get_emulate_prefix(struct insn *insn,
> return 1;
>
> err_out:
> - return 0;
> + return -ENODATA;
> }
>
> static void insn_get_emulate_prefix(struct insn *insn)
> {
> - if (__insn_get_emulate_prefix(insn, xen_prefix, sizeof(xen_prefix)))
> + int ret;
> +
> + ret = __insn_get_emulate_prefix(insn, xen_prefix, sizeof(xen_prefix));
> + if (ret > 0)
> return;
>
> __insn_get_emulate_prefix(insn, kvm_prefix, sizeof(kvm_prefix));
> @@ -98,8 +104,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
> */
> -void insn_get_prefixes(struct insn *insn)
> +int insn_get_prefixes(struct insn *insn)
> {
> struct insn_field *prefixes = &insn->prefixes;
> insn_attr_t attr;
> @@ -107,7 +117,7 @@ void insn_get_prefixes(struct insn *insn)
> int i, nb;
>
> if (prefixes->got)
> - return;
> + return 0;
>
> insn_get_emulate_prefix(insn);
>
> @@ -218,8 +228,10 @@ void insn_get_prefixes(struct insn *insn)
>
> prefixes->got = 1;
>
> + return 0;
> +
> err_out:
> - return;
> + return -ENODATA;
> }
>
> /**
> @@ -231,16 +243,25 @@ void insn_get_prefixes(struct insn *insn)
> * 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);
> @@ -255,9 +276,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 1;
> + }
> + /* VEX has only 1 byte for opcode */
> + goto end;
> }
>
> insn->attr = inat_get_opcode_attribute(op);
> @@ -268,13 +293,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 1;
> + }
> end:
> opcode->got = 1;
> + return 0;
>
> err_out:
> - return;
> + return -ENODATA;
> }
>
> /**
> @@ -284,15 +314,25 @@ void insn_get_opcode(struct insn *insn)
> * 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);
> @@ -302,17 +342,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 1;
> + }
> }
> }
>
> if (insn->x86_64 && inat_is_force64(insn->attr))
> insn->opnd_bytes = 8;
> +
> modrm->got = 1;
> + return 0;
>
> err_out:
> - return;
> + return -ENODATA;
> }
>
>
> @@ -326,11 +371,16 @@ void insn_get_modrm(struct insn *insn)
> 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 ret;
> + }
> /*
> * For rip-relative instructions, the mod field (top 2 bits)
> * is zero and the r/m field (bottom 3 bits) is 0x5.
> @@ -344,15 +394,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 &&
> @@ -363,8 +423,10 @@ void insn_get_sib(struct insn *insn)
> }
> insn->sib.got = 1;
>
> + return 0;
> +
> err_out:
> - return;
> + return -ENODATA;
> }
>
>
> @@ -375,15 +437,25 @@ void insn_get_sib(struct insn *insn)
> * 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:
> @@ -426,12 +498,13 @@ void insn_get_displacement(struct insn *insn)
> }
> out:
> insn->displacement.got = 1;
> + return 0;
>
> err_out:
> - return;
> + return -ENODATA;
> }
>
> -/* Decode moffset16/32/64. Return 0 if failed */
> +/* Decode moffset16/32/64. Return a negative value if failed. */
> static int __get_moffset(struct insn *insn)
> {
> switch (insn->addr_bytes) {
> @@ -457,10 +530,10 @@ static int __get_moffset(struct insn *insn)
> return 1;
>
> err_out:
> - return 0;
> + return -ENODATA;
> }
>
> -/* Decode imm v32(Iz). Return 0 if failed */
> +/* Decode imm v32(Iz). Return a negative value if failed. */
> static int __get_immv32(struct insn *insn)
> {
> switch (insn->opnd_bytes) {
> @@ -480,10 +553,10 @@ static int __get_immv32(struct insn *insn)
> return 1;
>
> err_out:
> - return 0;
> + return -ENODATA;
> }
>
> -/* Decode imm v64(Iv/Ov), Return 0 if failed */
> +/* Decode imm v64(Iv/Ov). Return a negative value if failed. */
> static int __get_immv(struct insn *insn)
> {
> switch (insn->opnd_bytes) {
> @@ -507,11 +580,12 @@ static int __get_immv(struct insn *insn)
> insn->immediate1.got = insn->immediate2.got = 1;
>
> return 1;
> +
> err_out:
> - return 0;
> + return -ENODATA;
> }
>
> -/* Decode ptr16:16/32(Ap) */
> +/* Decode ptr16:16/32(Ap). Return a negative value if failed. */
> static int __get_immptr(struct insn *insn)
> {
> switch (insn->opnd_bytes) {
> @@ -534,25 +608,36 @@ static int __get_immptr(struct insn *insn)
> insn->immediate1.got = insn->immediate2.got = 1;
>
> return 1;
> +
> err_out:
> - return 0;
> + return -ENODATA;
> }
>
> /**
> - * 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))
> @@ -605,9 +690,10 @@ void insn_get_immediate(struct insn *insn)
> }
> done:
> insn->immediate.got = 1;
> + return 0;
>
> err_out:
> - return;
> + return -ENODATA;
> }
>
> /**
> @@ -616,13 +702,56 @@ void insn_get_immediate(struct insn *insn)
> *
> * 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;
> +
> + 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;
> }
> diff --git a/tools/include/linux/kconfig.h b/tools/include/linux/kconfig.h
> new file mode 100644
> index 000000000000..1555a0c4f345
> --- /dev/null
> +++ b/tools/include/linux/kconfig.h
> @@ -0,0 +1,73 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _TOOLS_LINUX_KCONFIG_H
> +#define _TOOLS_LINUX_KCONFIG_H
> +
> +/* CONFIG_CC_VERSION_TEXT (Do not delete this comment. See help in Kconfig) */
> +
> +#ifdef CONFIG_CPU_BIG_ENDIAN
> +#define __BIG_ENDIAN 4321
> +#else
> +#define __LITTLE_ENDIAN 1234
> +#endif
> +
> +#define __ARG_PLACEHOLDER_1 0,
> +#define __take_second_arg(__ignored, val, ...) val
> +
> +/*
> + * The use of "&&" / "||" is limited in certain expressions.
> + * The following enable to calculate "and" / "or" with macro expansion only.
> + */
> +#define __and(x, y) ___and(x, y)
> +#define ___and(x, y) ____and(__ARG_PLACEHOLDER_##x, y)
> +#define ____and(arg1_or_junk, y) __take_second_arg(arg1_or_junk y, 0)
> +
> +#define __or(x, y) ___or(x, y)
> +#define ___or(x, y) ____or(__ARG_PLACEHOLDER_##x, y)
> +#define ____or(arg1_or_junk, y) __take_second_arg(arg1_or_junk 1, y)
> +
> +/*
> + * Helper macros to use CONFIG_ options in C/CPP expressions. Note that
> + * these only work with boolean and tristate options.
> + */
> +
> +/*
> + * Getting something that works in C and CPP for an arg that may or may
> + * not be defined is tricky. Here, if we have "#define CONFIG_BOOGER 1"
> + * we match on the placeholder define, insert the "0," for arg1 and generate
> + * the triplet (0, 1, 0). Then the last step cherry picks the 2nd arg (a one).
> + * When CONFIG_BOOGER is not defined, we generate a (... 1, 0) pair, and when
> + * the last step cherry picks the 2nd arg, we get a zero.
> + */
> +#define __is_defined(x) ___is_defined(x)
> +#define ___is_defined(val) ____is_defined(__ARG_PLACEHOLDER_##val)
> +#define ____is_defined(arg1_or_junk) __take_second_arg(arg1_or_junk 1, 0)
> +
> +/*
> + * IS_BUILTIN(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y', 0
> + * otherwise. For boolean options, this is equivalent to
> + * IS_ENABLED(CONFIG_FOO).
> + */
> +#define IS_BUILTIN(option) __is_defined(option)
> +
> +/*
> + * IS_MODULE(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'm', 0
> + * otherwise.
> + */
> +#define IS_MODULE(option) __is_defined(option##_MODULE)
> +
> +/*
> + * IS_REACHABLE(CONFIG_FOO) evaluates to 1 if the currently compiled
> + * code can call a function defined in code compiled based on CONFIG_FOO.
> + * This is similar to IS_ENABLED(), but returns false when invoked from
> + * built-in code when CONFIG_FOO is set to 'm'.
> + */
> +#define IS_REACHABLE(option) __or(IS_BUILTIN(option), \
> + __and(IS_MODULE(option), __is_defined(MODULE)))
> +
> +/*
> + * IS_ENABLED(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y' or 'm',
> + * 0 otherwise.
> + */
> +#define IS_ENABLED(option) __or(IS_BUILTIN(option), IS_MODULE(option))
> +
> +#endif /* _TOOLS_LINUX_KCONFIG_H */
> --
> 2.29.2
>
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v1 03/19] x86/insn: Add an insn_decode() API
2020-12-28 1:15 ` Masami Hiramatsu
@ 2020-12-29 20:06 ` Borislav Petkov
2020-12-30 9:00 ` Masami Hiramatsu
0 siblings, 1 reply; 42+ messages in thread
From: Borislav Petkov @ 2020-12-29 20:06 UTC (permalink / raw)
To: Masami Hiramatsu; +Cc: Andy Lutomirski, X86 ML, LKML
On Mon, Dec 28, 2020 at 10:15:10AM +0900, Masami Hiramatsu wrote:
> BTW, insn_decode() can return -EINVAL if !insn_complete(), is that OK?
It does with this change. Or are you asking whether it returning -EINVAL
in that case is ok?
I don't see why not - this way callers can differentiate where it failed
- at fetching bytes with -ENODATA or it wasn't decoded completely -
-EINVAL.
> I think tools clone code must not use INSN_MODE_KERN because the tools may
> not use kernel Kconfig.
>
> Hmm, this may be better to make a different patch to introduce a NOSYNC tag
> for sync checker in the tools. Something like;
I'd actually prefer this:
diff --git a/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
index f8772b371452..545320c67855 100644
--- a/tools/arch/x86/include/asm/insn.h
+++ b/tools/arch/x86/include/asm/insn.h
@@ -98,8 +98,6 @@ 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,
};
so that when a tool does use INSN_MODE_KERN, it would fail building:
In file included from util/intel-pt-decoder/intel-pt-insn-decoder.c:15:
util/intel-pt-decoder/../../../arch/x86/lib/insn.c: In function ‘insn_decode’:
util/intel-pt-decoder/../../../arch/x86/lib/insn.c:751:11: error: ‘INSN_MODE_KERN’ undeclared (first use in this function); did you mean ‘INSN_MODE_64’?
751 | if (m == INSN_MODE_KERN)
| ^~~~~~~~~~~~~~
| INSN_MODE_64
util/intel-pt-decoder/../../../arch/x86/lib/insn.c:751:11: note: each undeclared identifier is reported only once for each function it appears in
LD arch/perf-in.o
util/intel-pt-decoder/intel-pt-insn-decoder.c: In function ‘intel_pt_get_insn’:
util/intel-pt-decoder/intel-pt-insn-decoder.c:163:37: error: ‘INSN_MODE_KERN’ undeclared (first use in this function); did you mean ‘INSN_MODE_64’?
163 | ret = insn_decode(&insn, buf, len, INSN_MODE_KERN);
| ^~~~~~~~~~~~~~
| INSN_MODE_64
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v1 03/19] x86/insn: Add an insn_decode() API
2020-12-29 20:06 ` Borislav Petkov
@ 2020-12-30 9:00 ` Masami Hiramatsu
2020-12-30 9:28 ` Borislav Petkov
0 siblings, 1 reply; 42+ messages in thread
From: Masami Hiramatsu @ 2020-12-30 9:00 UTC (permalink / raw)
To: Borislav Petkov; +Cc: Andy Lutomirski, X86 ML, LKML
On Tue, 29 Dec 2020 21:06:54 +0100
Borislav Petkov <bp@alien8.de> wrote:
> On Mon, Dec 28, 2020 at 10:15:10AM +0900, Masami Hiramatsu wrote:
> > BTW, insn_decode() can return -EINVAL if !insn_complete(), is that OK?
>
> It does with this change. Or are you asking whether it returning -EINVAL
> in that case is ok?
>
> I don't see why not - this way callers can differentiate where it failed
> - at fetching bytes with -ENODATA or it wasn't decoded completely -
> -EINVAL.
Ah, I got it.
>
> > I think tools clone code must not use INSN_MODE_KERN because the tools may
> > not use kernel Kconfig.
> >
> > Hmm, this may be better to make a different patch to introduce a NOSYNC tag
> > for sync checker in the tools. Something like;
>
> I'd actually prefer this:
>
> diff --git a/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
> index f8772b371452..545320c67855 100644
> --- a/tools/arch/x86/include/asm/insn.h
> +++ b/tools/arch/x86/include/asm/insn.h
> @@ -98,8 +98,6 @@ 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,
> };
Agreed. This is much simpler.
Maybe I need to replace it with dummy lines but it is possible.
>
>
> so that when a tool does use INSN_MODE_KERN, it would fail building:
>
> In file included from util/intel-pt-decoder/intel-pt-insn-decoder.c:15:
> util/intel-pt-decoder/../../../arch/x86/lib/insn.c: In function ‘insn_decode’:
> util/intel-pt-decoder/../../../arch/x86/lib/insn.c:751:11: error: ‘INSN_MODE_KERN’ undeclared (first use in this function); did you mean ‘INSN_MODE_64’?
> 751 | if (m == INSN_MODE_KERN)
> | ^~~~~~~~~~~~~~
> | INSN_MODE_64
This part is OK. I can replace it with dummy lines.
> util/intel-pt-decoder/../../../arch/x86/lib/insn.c:751:11: note: each undeclared identifier is reported only once for each function it appears in
> LD arch/perf-in.o
> util/intel-pt-decoder/intel-pt-insn-decoder.c: In function ‘intel_pt_get_insn’:
> util/intel-pt-decoder/intel-pt-insn-decoder.c:163:37: error: ‘INSN_MODE_KERN’ undeclared (first use in this function); did you mean ‘INSN_MODE_64’?
> 163 | ret = insn_decode(&insn, buf, len, INSN_MODE_KERN);
> | ^~~~~~~~~~~~~~
> | INSN_MODE_64
But in [17/19], your patch seems not using INSN_MODE_KERN there.
--- a/tools/perf/util/intel-pt-decoder/intel-pt-insn-decoder.c
+++ b/tools/perf/util/intel-pt-decoder/intel-pt-insn-decoder.c
@@ -158,11 +158,13 @@ int intel_pt_get_insn(const unsigned char *buf, size_t len, int x86_64,
struct intel_pt_insn *intel_pt_insn)
{
struct insn insn;
+ int ret;
- insn_init(&insn, buf, len, x86_64);
- insn_get_length(&insn);
- if (!insn_complete(&insn) || insn.length > len)
+ ret = insn_decode(&insn, buf, len,
+ x86_64 ? INSN_MODE_64 : INSN_MODE_32);
+ if (ret < 0 || insn.length > len)
return -1;
+
intel_pt_insn_decoder(&insn, intel_pt_insn);
if (insn.length < INTEL_PT_INSN_BUF_SZ)
memcpy(intel_pt_insn->buf, buf, insn.length);
Thank you,
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v1 03/19] x86/insn: Add an insn_decode() API
2020-12-30 9:00 ` Masami Hiramatsu
@ 2020-12-30 9:28 ` Borislav Petkov
2021-01-06 5:21 ` Masami Hiramatsu
0 siblings, 1 reply; 42+ messages in thread
From: Borislav Petkov @ 2020-12-30 9:28 UTC (permalink / raw)
To: Masami Hiramatsu; +Cc: Andy Lutomirski, X86 ML, LKML
On Wed, Dec 30, 2020 at 06:00:52PM +0900, Masami Hiramatsu wrote:
> Maybe I need to replace it with dummy lines but it is possible.
Dummy lines like "IGNORE DURING CHECK" or something like that?
> But in [17/19], your patch seems not using INSN_MODE_KERN there.
I replaced it only locally just so that I can cause the build error and
show you what mean.
I still haven't thought about what do to exactly there wrt making
sync-check.sh happy but the aspect of failing the build is a nice one.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v1 03/19] x86/insn: Add an insn_decode() API
2020-12-30 9:28 ` Borislav Petkov
@ 2021-01-06 5:21 ` Masami Hiramatsu
2021-01-08 18:59 ` Borislav Petkov
0 siblings, 1 reply; 42+ messages in thread
From: Masami Hiramatsu @ 2021-01-06 5:21 UTC (permalink / raw)
To: Borislav Petkov; +Cc: Andy Lutomirski, X86 ML, LKML, Arnaldo Carvalho de Melo
On Wed, 30 Dec 2020 10:28:33 +0100
Borislav Petkov <bp@alien8.de> wrote:
> I still haven't thought about what do to exactly there wrt making
> sync-check.sh happy but the aspect of failing the build is a nice one.
So I think it is possible to introduce a keyword in a comment
for ignoring sync check something like below. This will allow us
a generic pattern matching.
The keyword is just an example, "no-sync-check" etc. is OK.
What would you think about it?
Thank you,
diff --git a/arch/x86/include/asm/inat.h b/arch/x86/include/asm/inat.h
index 4cf2ad521f65..ff3d119610f5 100644
--- a/arch/x86/include/asm/inat.h
+++ b/arch/x86/include/asm/inat.h
@@ -6,7 +6,7 @@
*
* Written by Masami Hiramatsu <mhiramat@redhat.com>
*/
-#include <asm/inat_types.h>
+#include <asm/inat_types.h> /* Different from tools */
/*
* Internal bits. Don't use bitmasks directly, because these bits are
diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
index a8c3d284fa46..dda4fe208659 100644
--- a/arch/x86/include/asm/insn.h
+++ b/arch/x86/include/asm/insn.h
@@ -8,7 +8,7 @@
*/
/* insn_attr_t is defined in inat.h */
-#include <asm/inat.h>
+#include <asm/inat.h> /* Different from tools */
struct insn_field {
union {
diff --git a/tools/arch/x86/include/asm/inat.h b/tools/arch/x86/include/asm/inat.h
index 877827b7c2c3..1fcfb4efb5f4 100644
--- a/tools/arch/x86/include/asm/inat.h
+++ b/tools/arch/x86/include/asm/inat.h
@@ -6,7 +6,7 @@
*
* Written by Masami Hiramatsu <mhiramat@redhat.com>
*/
-#include "inat_types.h"
+#include "inat_types.h" /* Different from kernel */
/*
* Internal bits. Don't use bitmasks directly, because these bits are
diff --git a/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
index 52c6262e6bfd..702135ddb4fd 100644
--- a/tools/arch/x86/include/asm/insn.h
+++ b/tools/arch/x86/include/asm/insn.h
@@ -8,7 +8,7 @@
*/
/* insn_attr_t is defined in inat.h */
-#include "inat.h"
+#include "inat.h" /* Different from kernel */
struct insn_field {
union {
diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
index dded93a2bc89..b112d68c5513 100755
--- a/tools/perf/check-headers.sh
+++ b/tools/perf/check-headers.sh
@@ -137,8 +137,8 @@ check include/uapi/linux/mman.h '-I "^#include <\(uapi/\)*asm/mman.h>"'
check include/linux/build_bug.h '-I "^#\(ifndef\|endif\)\( \/\/\)* static_assert$"'
check include/linux/ctype.h '-I "isdigit("'
check lib/ctype.c '-I "^EXPORT_SYMBOL" -I "^#include <linux/export.h>" -B'
-check arch/x86/include/asm/inat.h '-I "^#include [\"<]\(asm/\)*inat_types.h[\">]"'
-check arch/x86/include/asm/insn.h '-I "^#include [\"<]\(asm/\)*inat.h[\">]"'
+check arch/x86/include/asm/inat.h '-I "^.*/\* Different from \(tools\|kernel\) \*/"'
+check arch/x86/include/asm/insn.h '-I "^.*/\* Different from \(tools\|kernel\) \*/"'
check arch/x86/lib/inat.c '-I "^#include [\"<]\(../include/\)*asm/insn.h[\">]"'
check arch/x86/lib/insn.c '-I "^#include [\"<]\(../include/\)*asm/in\(at\|sn\).h[\">]" -I "^#include [\"<]\(../include/\)*asm/emulate_prefix.h[\">]"'
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v1 03/19] x86/insn: Add an insn_decode() API
2021-01-06 5:21 ` Masami Hiramatsu
@ 2021-01-08 18:59 ` Borislav Petkov
2021-01-12 11:34 ` Masami Hiramatsu
0 siblings, 1 reply; 42+ messages in thread
From: Borislav Petkov @ 2021-01-08 18:59 UTC (permalink / raw)
To: Masami Hiramatsu; +Cc: Andy Lutomirski, X86 ML, LKML, Arnaldo Carvalho de Melo
On Wed, Jan 06, 2021 at 02:21:14PM +0900, Masami Hiramatsu wrote:
> So I think it is possible to introduce a keyword in a comment
> for ignoring sync check something like below. This will allow us
> a generic pattern matching.
>
> The keyword is just an example, "no-sync-check" etc. is OK.
>
> What would you think about it?
Yeah, I'd prefer a single keyword which to slap everywhere, see below.
The patch is only for demonstration, though, it is not complete.
And while playing with that after having commented out INSN_MODE_KERN in
the tools/ version, I realized that the build would always fail because
insn.c references it:
In file included from arch/x86/decode.c:12:
arch/x86/../../../arch/x86/lib/insn.c: In function ‘insn_decode’:
arch/x86/../../../arch/x86/lib/insn.c:751:11: error: ‘INSN_MODE_KERN’ undeclared (first use in this function); did you mean ‘INSN_MODE_64’?
751 | if (m == INSN_MODE_KERN)
| ^~~~~~~~~~~~~~
| INSN_MODE_64
and making that work would turn pretty ugly because I wanna avoid
slapping that __ignore_sync_check__ or whatever on more than one line.
So I need to think about a better solution here...
---
diff --git a/arch/x86/include/asm/inat.h b/arch/x86/include/asm/inat.h
index 4cf2ad521f65..b56c5741581a 100644
--- a/arch/x86/include/asm/inat.h
+++ b/arch/x86/include/asm/inat.h
@@ -6,7 +6,7 @@
*
* Written by Masami Hiramatsu <mhiramat@redhat.com>
*/
-#include <asm/inat_types.h>
+#include <asm/inat_types.h> /* __ignore_sync_check__ */
/*
* Internal bits. Don't use bitmasks directly, because these bits are
diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
index 9f1910284861..601eac7a4973 100644
--- a/arch/x86/include/asm/insn.h
+++ b/arch/x86/include/asm/insn.h
@@ -8,7 +8,7 @@
*/
/* insn_attr_t is defined in inat.h */
-#include <asm/inat.h>
+#include <asm/inat.h> /* __ignore_sync_check__ */
struct insn_field {
union {
@@ -99,7 +99,7 @@ enum insn_mode {
INSN_MODE_32,
INSN_MODE_64,
/* Mode is determined by the current kernel build. */
- INSN_MODE_KERN,
+ INSN_MODE_KERN, /* __ignore_sync_check__ */
INSN_NUM_MODES,
};
diff --git a/arch/x86/lib/inat.c b/arch/x86/lib/inat.c
index 12539fca75c4..b0f3b2a62ae2 100644
--- a/arch/x86/lib/inat.c
+++ b/arch/x86/lib/inat.c
@@ -4,7 +4,7 @@
*
* Written by Masami Hiramatsu <mhiramat@redhat.com>
*/
-#include <asm/insn.h>
+#include <asm/insn.h> /* __ignore_sync_check__ */
/* Attribute tables are generated from opcode map */
#include "inat-tables.c"
diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
index 2ab1d0256313..1295003fb4f7 100644
--- a/arch/x86/lib/insn.c
+++ b/arch/x86/lib/insn.c
@@ -10,13 +10,13 @@
#else
#include <string.h>
#endif
-#include <asm/inat.h>
-#include <asm/insn.h>
+#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>
+#include <asm/emulate_prefix.h> /* __ignore_sync_check__ */
/* Verify next sizeof(t) bytes can be on the same instruction */
#define validate_next(t, insn, n) \
diff --git a/tools/arch/x86/include/asm/inat.h b/tools/arch/x86/include/asm/inat.h
index 877827b7c2c3..a61051400311 100644
--- a/tools/arch/x86/include/asm/inat.h
+++ b/tools/arch/x86/include/asm/inat.h
@@ -6,7 +6,7 @@
*
* Written by Masami Hiramatsu <mhiramat@redhat.com>
*/
-#include "inat_types.h"
+#include "inat_types.h" /* __ignore_sync_check__ */
/*
* Internal bits. Don't use bitmasks directly, because these bits are
diff --git a/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
index f8772b371452..b12329de4e6e 100644
--- a/tools/arch/x86/include/asm/insn.h
+++ b/tools/arch/x86/include/asm/insn.h
@@ -8,7 +8,7 @@
*/
/* insn_attr_t is defined in inat.h */
-#include "inat.h"
+#include "inat.h" /* __ignore_sync_check__ */
struct insn_field {
union {
@@ -99,7 +99,7 @@ enum insn_mode {
INSN_MODE_32,
INSN_MODE_64,
/* Mode is determined by the current kernel build. */
- INSN_MODE_KERN,
+ /* INSN_MODE_KERN, __ignore_sync_check__ */
INSN_NUM_MODES,
};
diff --git a/tools/arch/x86/lib/inat.c b/tools/arch/x86/lib/inat.c
index 4f5ed49e1b4e..dfbcc6405941 100644
--- a/tools/arch/x86/lib/inat.c
+++ b/tools/arch/x86/lib/inat.c
@@ -4,7 +4,7 @@
*
* Written by Masami Hiramatsu <mhiramat@redhat.com>
*/
-#include "../include/asm/insn.h"
+#include "../include/asm/insn.h" /* __ignore_sync_check__ */
/* Attribute tables are generated from opcode map */
#include "inat-tables.c"
diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c
index c224e1569034..0824ae531019 100644
--- a/tools/arch/x86/lib/insn.c
+++ b/tools/arch/x86/lib/insn.c
@@ -10,13 +10,13 @@
#else
#include <string.h>
#endif
-#include "../include/asm/inat.h"
-#include "../include/asm/insn.h"
+#include "../include/asm/inat.h" /* __ignore_sync_check__ */
+#include "../include/asm/insn.h" /* __ignore_sync_check__ */
#include <linux/errno.h>
#include <linux/kconfig.h>
-#include "../include/asm/emulate_prefix.h"
+#include "../include/asm/emulate_prefix.h" /* __ignore_sync_check__ */
/* Verify next sizeof(t) bytes can be on the same instruction */
#define validate_next(t, insn, n) \
diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
index dded93a2bc89..46ee37c87a80 100755
--- a/tools/perf/check-headers.sh
+++ b/tools/perf/check-headers.sh
@@ -75,6 +75,13 @@ include/uapi/asm-generic/mman-common.h
include/uapi/asm-generic/unistd.h
'
+SYNC_CHECK_FILES='
+arch/x86/include/asm/inat.h
+arch/x86/include/asm/insn.h
+arch/x86/lib/inat.c
+arch/x86/lib/insn.c
+'
+
# These copies are under tools/perf/trace/beauty/ as they are not used to in
# building object files only by scripts in tools/perf/trace/beauty/ to generate
# tables that then gets included in .c files for things like id->string syscall
@@ -129,6 +136,10 @@ for i in $FILES; do
check $i -B
done
+for i in $SYNC_CHECK_FILES; do
+ check $i '-I "^.*\/*.*__ignore_sync_check__ \*/.*$"'
+done
+
# diff with extra ignore lines
check arch/x86/lib/memcpy_64.S '-I "^EXPORT_SYMBOL" -I "^#include <asm/export.h>" -I"^SYM_FUNC_START\(_LOCAL\)*(memcpy_\(erms\|orig\))"'
check arch/x86/lib/memset_64.S '-I "^EXPORT_SYMBOL" -I "^#include <asm/export.h>" -I"^SYM_FUNC_START\(_LOCAL\)*(memset_\(erms\|orig\))"'
@@ -137,10 +148,6 @@ check include/uapi/linux/mman.h '-I "^#include <\(uapi/\)*asm/mman.h>"'
check include/linux/build_bug.h '-I "^#\(ifndef\|endif\)\( \/\/\)* static_assert$"'
check include/linux/ctype.h '-I "isdigit("'
check lib/ctype.c '-I "^EXPORT_SYMBOL" -I "^#include <linux/export.h>" -B'
-check arch/x86/include/asm/inat.h '-I "^#include [\"<]\(asm/\)*inat_types.h[\">]"'
-check arch/x86/include/asm/insn.h '-I "^#include [\"<]\(asm/\)*inat.h[\">]"'
-check arch/x86/lib/inat.c '-I "^#include [\"<]\(../include/\)*asm/insn.h[\">]"'
-check arch/x86/lib/insn.c '-I "^#include [\"<]\(../include/\)*asm/in\(at\|sn\).h[\">]" -I "^#include [\"<]\(../include/\)*asm/emulate_prefix.h[\">]"'
# diff non-symmetric files
check_2 tools/perf/arch/x86/entry/syscalls/syscall_64.tbl arch/x86/entry/syscalls/syscall_64.tbl
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v1 03/19] x86/insn: Add an insn_decode() API
2021-01-08 18:59 ` Borislav Petkov
@ 2021-01-12 11:34 ` Masami Hiramatsu
2021-01-13 18:06 ` Borislav Petkov
0 siblings, 1 reply; 42+ messages in thread
From: Masami Hiramatsu @ 2021-01-12 11:34 UTC (permalink / raw)
To: Borislav Petkov; +Cc: Andy Lutomirski, X86 ML, LKML, Arnaldo Carvalho de Melo
On Fri, 8 Jan 2021 19:59:50 +0100
Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Jan 06, 2021 at 02:21:14PM +0900, Masami Hiramatsu wrote:
> > So I think it is possible to introduce a keyword in a comment
> > for ignoring sync check something like below. This will allow us
> > a generic pattern matching.
> >
> > The keyword is just an example, "no-sync-check" etc. is OK.
> >
> > What would you think about it?
>
> Yeah, I'd prefer a single keyword which to slap everywhere, see below.
> The patch is only for demonstration, though, it is not complete.
Yes, that looks good to me too.
>
> And while playing with that after having commented out INSN_MODE_KERN in
> the tools/ version, I realized that the build would always fail because
> insn.c references it:
>
> In file included from arch/x86/decode.c:12:
> arch/x86/../../../arch/x86/lib/insn.c: In function ‘insn_decode’:
> arch/x86/../../../arch/x86/lib/insn.c:751:11: error: ‘INSN_MODE_KERN’ undeclared (first use in this function); did you mean ‘INSN_MODE_64’?
> 751 | if (m == INSN_MODE_KERN)
> | ^~~~~~~~~~~~~~
> | INSN_MODE_64
>
> and making that work would turn pretty ugly because I wanna avoid
> slapping that __ignore_sync_check__ or whatever on more than one line.
>
> So I need to think about a better solution here...
Hmm, instead of removing INSN_MODE_KERN, if you just return an error,
it will change one line.
if (m == INSN_MODE_KERN)
return -EINVAL; /* __ignore_sync_check__ */
else
insn_init(insn, kaddr, buf_len, m == INSN_MODE_64);
Or, add one definition before that line.
#define INSN_MODE_KERN -1 /* __ignore_sync_check__ */
Thank you,
>
> ---
> diff --git a/arch/x86/include/asm/inat.h b/arch/x86/include/asm/inat.h
> index 4cf2ad521f65..b56c5741581a 100644
> --- a/arch/x86/include/asm/inat.h
> +++ b/arch/x86/include/asm/inat.h
> @@ -6,7 +6,7 @@
> *
> * Written by Masami Hiramatsu <mhiramat@redhat.com>
> */
> -#include <asm/inat_types.h>
> +#include <asm/inat_types.h> /* __ignore_sync_check__ */
>
> /*
> * Internal bits. Don't use bitmasks directly, because these bits are
> diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
> index 9f1910284861..601eac7a4973 100644
> --- a/arch/x86/include/asm/insn.h
> +++ b/arch/x86/include/asm/insn.h
> @@ -8,7 +8,7 @@
> */
>
> /* insn_attr_t is defined in inat.h */
> -#include <asm/inat.h>
> +#include <asm/inat.h> /* __ignore_sync_check__ */
>
> struct insn_field {
> union {
> @@ -99,7 +99,7 @@ enum insn_mode {
> INSN_MODE_32,
> INSN_MODE_64,
> /* Mode is determined by the current kernel build. */
> - INSN_MODE_KERN,
> + INSN_MODE_KERN, /* __ignore_sync_check__ */
> INSN_NUM_MODES,
> };
>
> diff --git a/arch/x86/lib/inat.c b/arch/x86/lib/inat.c
> index 12539fca75c4..b0f3b2a62ae2 100644
> --- a/arch/x86/lib/inat.c
> +++ b/arch/x86/lib/inat.c
> @@ -4,7 +4,7 @@
> *
> * Written by Masami Hiramatsu <mhiramat@redhat.com>
> */
> -#include <asm/insn.h>
> +#include <asm/insn.h> /* __ignore_sync_check__ */
>
> /* Attribute tables are generated from opcode map */
> #include "inat-tables.c"
> diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
> index 2ab1d0256313..1295003fb4f7 100644
> --- a/arch/x86/lib/insn.c
> +++ b/arch/x86/lib/insn.c
> @@ -10,13 +10,13 @@
> #else
> #include <string.h>
> #endif
> -#include <asm/inat.h>
> -#include <asm/insn.h>
> +#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>
> +#include <asm/emulate_prefix.h> /* __ignore_sync_check__ */
>
> /* Verify next sizeof(t) bytes can be on the same instruction */
> #define validate_next(t, insn, n) \
> diff --git a/tools/arch/x86/include/asm/inat.h b/tools/arch/x86/include/asm/inat.h
> index 877827b7c2c3..a61051400311 100644
> --- a/tools/arch/x86/include/asm/inat.h
> +++ b/tools/arch/x86/include/asm/inat.h
> @@ -6,7 +6,7 @@
> *
> * Written by Masami Hiramatsu <mhiramat@redhat.com>
> */
> -#include "inat_types.h"
> +#include "inat_types.h" /* __ignore_sync_check__ */
>
> /*
> * Internal bits. Don't use bitmasks directly, because these bits are
> diff --git a/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
> index f8772b371452..b12329de4e6e 100644
> --- a/tools/arch/x86/include/asm/insn.h
> +++ b/tools/arch/x86/include/asm/insn.h
> @@ -8,7 +8,7 @@
> */
>
> /* insn_attr_t is defined in inat.h */
> -#include "inat.h"
> +#include "inat.h" /* __ignore_sync_check__ */
>
> struct insn_field {
> union {
> @@ -99,7 +99,7 @@ enum insn_mode {
> INSN_MODE_32,
> INSN_MODE_64,
> /* Mode is determined by the current kernel build. */
> - INSN_MODE_KERN,
> + /* INSN_MODE_KERN, __ignore_sync_check__ */
> INSN_NUM_MODES,
> };
>
> diff --git a/tools/arch/x86/lib/inat.c b/tools/arch/x86/lib/inat.c
> index 4f5ed49e1b4e..dfbcc6405941 100644
> --- a/tools/arch/x86/lib/inat.c
> +++ b/tools/arch/x86/lib/inat.c
> @@ -4,7 +4,7 @@
> *
> * Written by Masami Hiramatsu <mhiramat@redhat.com>
> */
> -#include "../include/asm/insn.h"
> +#include "../include/asm/insn.h" /* __ignore_sync_check__ */
>
> /* Attribute tables are generated from opcode map */
> #include "inat-tables.c"
> diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c
> index c224e1569034..0824ae531019 100644
> --- a/tools/arch/x86/lib/insn.c
> +++ b/tools/arch/x86/lib/insn.c
> @@ -10,13 +10,13 @@
> #else
> #include <string.h>
> #endif
> -#include "../include/asm/inat.h"
> -#include "../include/asm/insn.h"
> +#include "../include/asm/inat.h" /* __ignore_sync_check__ */
> +#include "../include/asm/insn.h" /* __ignore_sync_check__ */
>
> #include <linux/errno.h>
> #include <linux/kconfig.h>
>
> -#include "../include/asm/emulate_prefix.h"
> +#include "../include/asm/emulate_prefix.h" /* __ignore_sync_check__ */
>
> /* Verify next sizeof(t) bytes can be on the same instruction */
> #define validate_next(t, insn, n) \
> diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
> index dded93a2bc89..46ee37c87a80 100755
> --- a/tools/perf/check-headers.sh
> +++ b/tools/perf/check-headers.sh
> @@ -75,6 +75,13 @@ include/uapi/asm-generic/mman-common.h
> include/uapi/asm-generic/unistd.h
> '
>
> +SYNC_CHECK_FILES='
> +arch/x86/include/asm/inat.h
> +arch/x86/include/asm/insn.h
> +arch/x86/lib/inat.c
> +arch/x86/lib/insn.c
> +'
> +
> # These copies are under tools/perf/trace/beauty/ as they are not used to in
> # building object files only by scripts in tools/perf/trace/beauty/ to generate
> # tables that then gets included in .c files for things like id->string syscall
> @@ -129,6 +136,10 @@ for i in $FILES; do
> check $i -B
> done
>
> +for i in $SYNC_CHECK_FILES; do
> + check $i '-I "^.*\/*.*__ignore_sync_check__ \*/.*$"'
> +done
> +
> # diff with extra ignore lines
> check arch/x86/lib/memcpy_64.S '-I "^EXPORT_SYMBOL" -I "^#include <asm/export.h>" -I"^SYM_FUNC_START\(_LOCAL\)*(memcpy_\(erms\|orig\))"'
> check arch/x86/lib/memset_64.S '-I "^EXPORT_SYMBOL" -I "^#include <asm/export.h>" -I"^SYM_FUNC_START\(_LOCAL\)*(memset_\(erms\|orig\))"'
> @@ -137,10 +148,6 @@ check include/uapi/linux/mman.h '-I "^#include <\(uapi/\)*asm/mman.h>"'
> check include/linux/build_bug.h '-I "^#\(ifndef\|endif\)\( \/\/\)* static_assert$"'
> check include/linux/ctype.h '-I "isdigit("'
> check lib/ctype.c '-I "^EXPORT_SYMBOL" -I "^#include <linux/export.h>" -B'
> -check arch/x86/include/asm/inat.h '-I "^#include [\"<]\(asm/\)*inat_types.h[\">]"'
> -check arch/x86/include/asm/insn.h '-I "^#include [\"<]\(asm/\)*inat.h[\">]"'
> -check arch/x86/lib/inat.c '-I "^#include [\"<]\(../include/\)*asm/insn.h[\">]"'
> -check arch/x86/lib/insn.c '-I "^#include [\"<]\(../include/\)*asm/in\(at\|sn\).h[\">]" -I "^#include [\"<]\(../include/\)*asm/emulate_prefix.h[\">]"'
>
> # diff non-symmetric files
> check_2 tools/perf/arch/x86/entry/syscalls/syscall_64.tbl arch/x86/entry/syscalls/syscall_64.tbl
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v1 03/19] x86/insn: Add an insn_decode() API
2021-01-12 11:34 ` Masami Hiramatsu
@ 2021-01-13 18:06 ` Borislav Petkov
0 siblings, 0 replies; 42+ messages in thread
From: Borislav Petkov @ 2021-01-13 18:06 UTC (permalink / raw)
To: Masami Hiramatsu; +Cc: Andy Lutomirski, X86 ML, LKML, Arnaldo Carvalho de Melo
On Tue, Jan 12, 2021 at 08:34:46PM +0900, Masami Hiramatsu wrote:
> Or, add one definition before that line.
>
> #define INSN_MODE_KERN -1 /* __ignore_sync_check__ */
I like that idea, thanks! And it seems to work:
---
diff --git a/arch/x86/include/asm/inat.h b/arch/x86/include/asm/inat.h
index 4cf2ad521f65..b56c5741581a 100644
--- a/arch/x86/include/asm/inat.h
+++ b/arch/x86/include/asm/inat.h
@@ -6,7 +6,7 @@
*
* Written by Masami Hiramatsu <mhiramat@redhat.com>
*/
-#include <asm/inat_types.h>
+#include <asm/inat_types.h> /* __ignore_sync_check__ */
/*
* Internal bits. Don't use bitmasks directly, because these bits are
diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
index 9f1910284861..6df0d3da0d86 100644
--- a/arch/x86/include/asm/insn.h
+++ b/arch/x86/include/asm/insn.h
@@ -8,7 +8,7 @@
*/
/* insn_attr_t is defined in inat.h */
-#include <asm/inat.h>
+#include <asm/inat.h> /* __ignore_sync_check__ */
struct insn_field {
union {
diff --git a/arch/x86/lib/inat.c b/arch/x86/lib/inat.c
index 12539fca75c4..b0f3b2a62ae2 100644
--- a/arch/x86/lib/inat.c
+++ b/arch/x86/lib/inat.c
@@ -4,7 +4,7 @@
*
* Written by Masami Hiramatsu <mhiramat@redhat.com>
*/
-#include <asm/insn.h>
+#include <asm/insn.h> /* __ignore_sync_check__ */
/* Attribute tables are generated from opcode map */
#include "inat-tables.c"
diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
index 2ab1d0256313..aa6ee796a987 100644
--- a/arch/x86/lib/insn.c
+++ b/arch/x86/lib/insn.c
@@ -10,13 +10,13 @@
#else
#include <string.h>
#endif
-#include <asm/inat.h>
-#include <asm/insn.h>
+#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>
+#include <asm/emulate_prefix.h> /* __ignore_sync_check__ */
/* Verify next sizeof(t) bytes can be on the same instruction */
#define validate_next(t, insn, n) \
@@ -748,6 +748,8 @@ int insn_decode(struct insn *insn, const void *kaddr, int buf_len, enum insn_mod
{
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
diff --git a/tools/arch/x86/include/asm/inat.h b/tools/arch/x86/include/asm/inat.h
index 877827b7c2c3..a61051400311 100644
--- a/tools/arch/x86/include/asm/inat.h
+++ b/tools/arch/x86/include/asm/inat.h
@@ -6,7 +6,7 @@
*
* Written by Masami Hiramatsu <mhiramat@redhat.com>
*/
-#include "inat_types.h"
+#include "inat_types.h" /* __ignore_sync_check__ */
/*
* Internal bits. Don't use bitmasks directly, because these bits are
diff --git a/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
index f8772b371452..4f219e3ae817 100644
--- a/tools/arch/x86/include/asm/insn.h
+++ b/tools/arch/x86/include/asm/insn.h
@@ -8,7 +8,7 @@
*/
/* insn_attr_t is defined in inat.h */
-#include "inat.h"
+#include "inat.h" /* __ignore_sync_check__ */
struct insn_field {
union {
diff --git a/tools/arch/x86/lib/inat.c b/tools/arch/x86/lib/inat.c
index 4f5ed49e1b4e..dfbcc6405941 100644
--- a/tools/arch/x86/lib/inat.c
+++ b/tools/arch/x86/lib/inat.c
@@ -4,7 +4,7 @@
*
* Written by Masami Hiramatsu <mhiramat@redhat.com>
*/
-#include "../include/asm/insn.h"
+#include "../include/asm/insn.h" /* __ignore_sync_check__ */
/* Attribute tables are generated from opcode map */
#include "inat-tables.c"
diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c
index c224e1569034..13e8615edc15 100644
--- a/tools/arch/x86/lib/insn.c
+++ b/tools/arch/x86/lib/insn.c
@@ -10,13 +10,13 @@
#else
#include <string.h>
#endif
-#include "../include/asm/inat.h"
-#include "../include/asm/insn.h"
+#include "../include/asm/inat.h" /* __ignore_sync_check__ */
+#include "../include/asm/insn.h" /* __ignore_sync_check__ */
#include <linux/errno.h>
#include <linux/kconfig.h>
-#include "../include/asm/emulate_prefix.h"
+#include "../include/asm/emulate_prefix.h" /* __ignore_sync_check__ */
/* Verify next sizeof(t) bytes can be on the same instruction */
#define validate_next(t, insn, n) \
@@ -748,6 +748,8 @@ int insn_decode(struct insn *insn, const void *kaddr, int buf_len, enum insn_mod
{
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
diff --git a/tools/objtool/sync-check.sh b/tools/objtool/sync-check.sh
index 606a4b5e929f..4bbabaecab14 100755
--- a/tools/objtool/sync-check.sh
+++ b/tools/objtool/sync-check.sh
@@ -16,11 +16,14 @@ arch/x86/include/asm/emulate_prefix.h
arch/x86/lib/x86-opcode-map.txt
arch/x86/tools/gen-insn-attr-x86.awk
include/linux/static_call_types.h
-arch/x86/include/asm/inat.h -I '^#include [\"<]\(asm/\)*inat_types.h[\">]'
-arch/x86/include/asm/insn.h -I '^#include [\"<]\(asm/\)*inat.h[\">]'
-arch/x86/lib/inat.c -I '^#include [\"<]\(../include/\)*asm/insn.h[\">]'
-arch/x86/lib/insn.c -I '^#include [\"<]\(../include/\)*asm/in\(at\|sn\).h[\">]' -I '^#include [\"<]\(../include/\)*asm/emulate_prefix.h[\">]'
"
+
+SYNC_CHECK_FILES='
+arch/x86/include/asm/inat.h
+arch/x86/include/asm/insn.h
+arch/x86/lib/inat.c
+arch/x86/lib/insn.c
+'
fi
check_2 () {
@@ -63,3 +66,9 @@ while read -r file_entry; do
done <<EOF
$FILES
EOF
+
+if [ "$SRCARCH" = "x86" ]; then
+ for i in $SYNC_CHECK_FILES; do
+ check $i '-I "^.*\/\*.*__ignore_sync_check__.*\*\/.*$"'
+ done
+fi
diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
index dded93a2bc89..07857dfb4d91 100755
--- a/tools/perf/check-headers.sh
+++ b/tools/perf/check-headers.sh
@@ -75,6 +75,13 @@ include/uapi/asm-generic/mman-common.h
include/uapi/asm-generic/unistd.h
'
+SYNC_CHECK_FILES='
+arch/x86/include/asm/inat.h
+arch/x86/include/asm/insn.h
+arch/x86/lib/inat.c
+arch/x86/lib/insn.c
+'
+
# These copies are under tools/perf/trace/beauty/ as they are not used to in
# building object files only by scripts in tools/perf/trace/beauty/ to generate
# tables that then gets included in .c files for things like id->string syscall
@@ -129,6 +136,10 @@ for i in $FILES; do
check $i -B
done
+for i in $SYNC_CHECK_FILES; do
+ check $i '-I "^.*\/\*.*__ignore_sync_check__.*\*\/.*$"'
+done
+
# diff with extra ignore lines
check arch/x86/lib/memcpy_64.S '-I "^EXPORT_SYMBOL" -I "^#include <asm/export.h>" -I"^SYM_FUNC_START\(_LOCAL\)*(memcpy_\(erms\|orig\))"'
check arch/x86/lib/memset_64.S '-I "^EXPORT_SYMBOL" -I "^#include <asm/export.h>" -I"^SYM_FUNC_START\(_LOCAL\)*(memset_\(erms\|orig\))"'
@@ -137,10 +148,6 @@ check include/uapi/linux/mman.h '-I "^#include <\(uapi/\)*asm/mman.h>"'
check include/linux/build_bug.h '-I "^#\(ifndef\|endif\)\( \/\/\)* static_assert$"'
check include/linux/ctype.h '-I "isdigit("'
check lib/ctype.c '-I "^EXPORT_SYMBOL" -I "^#include <linux/export.h>" -B'
-check arch/x86/include/asm/inat.h '-I "^#include [\"<]\(asm/\)*inat_types.h[\">]"'
-check arch/x86/include/asm/insn.h '-I "^#include [\"<]\(asm/\)*inat.h[\">]"'
-check arch/x86/lib/inat.c '-I "^#include [\"<]\(../include/\)*asm/insn.h[\">]"'
-check arch/x86/lib/insn.c '-I "^#include [\"<]\(../include/\)*asm/in\(at\|sn\).h[\">]" -I "^#include [\"<]\(../include/\)*asm/emulate_prefix.h[\">]"'
# diff non-symmetric files
check_2 tools/perf/arch/x86/entry/syscalls/syscall_64.tbl arch/x86/entry/syscalls/syscall_64.tbl
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v1 04/19] x86/insn-eval: Handle return values from the decoder
2020-12-23 17:42 [PATCH v1 00/19] x86/insn: Add an insn_decode() API Borislav Petkov
` (2 preceding siblings ...)
2020-12-23 17:42 ` [PATCH v1 03/19] x86/insn: Add an insn_decode() API Borislav Petkov
@ 2020-12-23 17:42 ` Borislav Petkov
2020-12-28 18:51 ` Sean Christopherson
2020-12-23 17:42 ` [PATCH v1 05/19] x86/boot/compressed/sev-es: Convert to insn_decode() Borislav Petkov
` (15 subsequent siblings)
19 siblings, 1 reply; 42+ messages in thread
From: Borislav Petkov @ 2020-12-23 17:42 UTC (permalink / raw)
To: Andy Lutomirski, Masami Hiramatsu; +Cc: X86 ML, LKML
From: Borislav Petkov <bp@suse.de>
Now that the different instruction-inspecting functions return a value,
test that and return early from callers if error has been encountered.
While at it, do not call insn_get_modrm() when calling
insn_get_displacement() because latter will make sure to call
insn_get_modrm() if ModRM hasn't been parsed yet.
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/lib/insn-eval.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
index 265d23a0c334..7e49aaf5454c 100644
--- a/arch/x86/lib/insn-eval.c
+++ b/arch/x86/lib/insn-eval.c
@@ -1106,18 +1106,21 @@ static int get_eff_addr_modrm_16(struct insn *insn, struct pt_regs *regs,
* @base_offset will have a register, as an offset from the base of pt_regs,
* that can be used to resolve the associated segment.
*
- * -EINVAL on error.
+ * Negative value on error.
*/
static int get_eff_addr_sib(struct insn *insn, struct pt_regs *regs,
int *base_offset, long *eff_addr)
{
long base, indx;
int indx_offset;
+ int ret;
if (insn->addr_bytes != 8 && insn->addr_bytes != 4)
return -EINVAL;
- insn_get_modrm(insn);
+ ret = insn_get_modrm(insn);
+ if (ret)
+ return ret;
if (!insn->modrm.nbytes)
return -EINVAL;
@@ -1125,7 +1128,9 @@ static int get_eff_addr_sib(struct insn *insn, struct pt_regs *regs,
if (X86_MODRM_MOD(insn->modrm.value) > 2)
return -EINVAL;
- insn_get_sib(insn);
+ ret = insn_get_sib(insn);
+ if (ret)
+ return ret;
if (!insn->sib.nbytes)
return -EINVAL;
@@ -1194,8 +1199,8 @@ static void __user *get_addr_ref_16(struct insn *insn, struct pt_regs *regs)
short eff_addr;
long tmp;
- insn_get_modrm(insn);
- insn_get_displacement(insn);
+ if (insn_get_displacement(insn))
+ goto out;
if (insn->addr_bytes != 2)
goto out;
@@ -1491,7 +1496,9 @@ bool insn_decode_regs(struct insn *insn, struct pt_regs *regs,
insn->addr_bytes = INSN_CODE_SEG_ADDR_SZ(seg_defs);
insn->opnd_bytes = INSN_CODE_SEG_OPND_SZ(seg_defs);
- insn_get_length(insn);
+ if (insn_get_length(insn))
+ return false;
+
if (buf_size < insn->length)
return false;
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v1 04/19] x86/insn-eval: Handle return values from the decoder
2020-12-23 17:42 ` [PATCH v1 04/19] x86/insn-eval: Handle return values from the decoder Borislav Petkov
@ 2020-12-28 18:51 ` Sean Christopherson
2020-12-28 19:06 ` Borislav Petkov
0 siblings, 1 reply; 42+ messages in thread
From: Sean Christopherson @ 2020-12-28 18:51 UTC (permalink / raw)
To: Borislav Petkov; +Cc: Andy Lutomirski, Masami Hiramatsu, X86 ML, LKML
On Wed, Dec 23, 2020, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
>
> Now that the different instruction-inspecting functions return a value,
> test that and return early from callers if error has been encountered.
>
> While at it, do not call insn_get_modrm() when calling
> insn_get_displacement() because latter will make sure to call
> insn_get_modrm() if ModRM hasn't been parsed yet.
>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
> arch/x86/lib/insn-eval.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
> index 265d23a0c334..7e49aaf5454c 100644
> --- a/arch/x86/lib/insn-eval.c
> +++ b/arch/x86/lib/insn-eval.c
> @@ -1106,18 +1106,21 @@ static int get_eff_addr_modrm_16(struct insn *insn, struct pt_regs *regs,
> * @base_offset will have a register, as an offset from the base of pt_regs,
> * that can be used to resolve the associated segment.
> *
> - * -EINVAL on error.
> + * Negative value on error.
> */
> static int get_eff_addr_sib(struct insn *insn, struct pt_regs *regs,
> int *base_offset, long *eff_addr)
> {
> long base, indx;
> int indx_offset;
> + int ret;
>
> if (insn->addr_bytes != 8 && insn->addr_bytes != 4)
> return -EINVAL;
>
> - insn_get_modrm(insn);
> + ret = insn_get_modrm(insn);
This patch is incomplete/inconsistent, and arguably wrong.
- get_eff_addr_reg() and get_eff_addr_modrm() still ignore the return of
insn_get_modrm() after this patch.
- Calling insn_get_modrm() from get_eff_addr_sib() is unnecessary (unless the
caller passed uninitialized garbage in @insn) as get_eff_addr_sib() is
called if and only if sib.nbytes!=0, and sib.nbytes can be non-zero if and
only if the modrm and sib have been got.
- get_addr_ref_16() does insn_get_displacement, i.e. guarantees the modrm is
parsed, while the 32/64 variants do not.
What about adding a prereq patch (or three) to call insn_get_displacement() in
insn_get_addr_ref() prior to switching on insn->addr_bytes? Then the various
internal helpers could be changed to either omit the sanity checks entirely or
WARN on invalid calls? Or better yet, add an INSN_WARN_ON() macro that compiles
out the checks by default? E.g. something like:
diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
index 7e49aaf5454c..348969146e0f 100644
--- a/arch/x86/lib/insn-eval.c
+++ b/arch/x86/lib/insn-eval.c
@@ -928,12 +928,8 @@ static int get_seg_base_limit(struct insn *insn, struct pt_regs *regs,
diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
index 7e49aaf5454c..348969146e0f 100644
--- a/arch/x86/lib/insn-eval.c
+++ b/arch/x86/lib/insn-eval.c
@@ -928,12 +928,8 @@ static int get_seg_base_limit(struct insn *insn, struct pt_regs *regs,
static int get_eff_addr_reg(struct insn *insn, struct pt_regs *regs,
int *regoff, long *eff_addr)
{
- insn_get_modrm(insn);
-
- if (!insn->modrm.nbytes)
- return -EINVAL;
-
- if (X86_MODRM_MOD(insn->modrm.value) != 3)
+ if (INSN_WARN_ON(!insn->modrm.got || !insn->modrm.nbytes ||
+ X86_MODRM_MOD(insn->modrm.value) != 3)
return -EINVAL;
*regoff = get_reg_offset(insn, regs, REG_TYPE_RM);
@@ -978,15 +974,9 @@ static int get_eff_addr_modrm(struct insn *insn, struct pt_regs *regs,
{
long tmp;
- if (insn->addr_bytes != 8 && insn->addr_bytes != 4)
- return -EINVAL;
-
- insn_get_modrm(insn);
-
- if (!insn->modrm.nbytes)
- return -EINVAL;
-
- if (X86_MODRM_MOD(insn->modrm.value) > 2)
+ if (INSN_WARN_ON((insn->addr_bytes != 8 && insn->addr_bytes != 4) ||
+ !insn->modrm.got || !insn->modrm.nbytes ||
+ X86_MODRM_MOD(insn->modrm.value) > 2)
return -EINVAL;
*regoff = get_reg_offset(insn, regs, REG_TYPE_RM);
@@ -1046,15 +1036,8 @@ static int get_eff_addr_modrm_16(struct insn *insn, struct pt_regs *regs,
int addr_offset1, addr_offset2, ret;
short addr1 = 0, addr2 = 0, displacement;
- if (insn->addr_bytes != 2)
- return -EINVAL;
-
- insn_get_modrm(insn);
-
- if (!insn->modrm.nbytes)
- return -EINVAL;
-
- if (X86_MODRM_MOD(insn->modrm.value) > 2)
+ if (WARN_ON_ONCE(insn->addr_bytes != 2 || !insn->modrm.got ||
+ !insn->modrm.nbytes || insn->modrm.value > 2))
return -EINVAL;
ret = get_reg_offset_16(insn, regs, &addr_offset1, &addr_offset2);
@@ -1199,10 +1182,7 @@ static void __user *get_addr_ref_16(struct insn *insn, struct pt_regs *regs)
short eff_addr;
long tmp;
- if (insn_get_displacement(insn))
- goto out;
-
- if (insn->addr_bytes != 2)
+ if (INSN_WARN_ON(insn->addr_bytes != 2))
goto out;
if (X86_MODRM_MOD(insn->modrm.value) == 3) {
@@ -1263,7 +1243,7 @@ static void __user *get_addr_ref_32(struct insn *insn, struct pt_regs *regs)
long tmp;
int ret;
- if (insn->addr_bytes != 4)
+ if (INSN_WARN_ON(insn->addr_bytes != 4))
goto out;
if (X86_MODRM_MOD(insn->modrm.value) == 3) {
@@ -1356,7 +1336,7 @@ static void __user *get_addr_ref_64(struct insn *insn, struct pt_regs *regs)
int regoff, ret;
long eff_addr;
- if (insn->addr_bytes != 8)
+ if (INSN_WARN_ON(insn->addr_bytes != 8))
goto out;
if (X86_MODRM_MOD(insn->modrm.value) == 3) {
@@ -1408,6 +1388,9 @@ void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs)
if (!insn || !regs)
return (void __user *)-1L;
+ if (insn_get_displacement(insn))
+ return (void __user *)-1L;
+
switch (insn->addr_bytes) {
case 2:
return get_addr_ref_16(insn, regs);
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v1 04/19] x86/insn-eval: Handle return values from the decoder
2020-12-28 18:51 ` Sean Christopherson
@ 2020-12-28 19:06 ` Borislav Petkov
0 siblings, 0 replies; 42+ messages in thread
From: Borislav Petkov @ 2020-12-28 19:06 UTC (permalink / raw)
To: Sean Christopherson; +Cc: Andy Lutomirski, Masami Hiramatsu, X86 ML, LKML
On Mon, Dec 28, 2020 at 10:51:15AM -0800, Sean Christopherson wrote:
> This patch is incomplete/inconsistent, and arguably wrong.
>
> - get_eff_addr_reg() and get_eff_addr_modrm() still ignore the return of
> insn_get_modrm() after this patch.
Ah, will fix, thx.
> - Calling insn_get_modrm() from get_eff_addr_sib() is unnecessary (unless the
> caller passed uninitialized garbage in @insn) as get_eff_addr_sib() is
> called if and only if sib.nbytes!=0, and sib.nbytes can be non-zero if and
> only if the modrm and sib have been got.
>
> - get_addr_ref_16() does insn_get_displacement, i.e. guarantees the modrm is
> parsed, while the 32/64 variants do not.
>
> What about adding a prereq patch (or three) to call insn_get_displacement() in
> insn_get_addr_ref() prior to switching on insn->addr_bytes? Then the various
> internal helpers could be changed to either omit the sanity checks entirely or
> WARN on invalid calls? Or better yet, add an INSN_WARN_ON() macro that compiles
> out the checks by default? E.g. something like:
So the idea is one construction site at a time (that's a German saying :)).
This set deals with whacking the insn decoder into returning
proper error/success values. The next set should do
simplifications/fixes/cleanups/whatever but not all at the same time for
obvious reasons.
So yeah, I'm all for omitting useless code but let's do that ontop.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v1 05/19] x86/boot/compressed/sev-es: Convert to insn_decode()
2020-12-23 17:42 [PATCH v1 00/19] x86/insn: Add an insn_decode() API Borislav Petkov
` (3 preceding siblings ...)
2020-12-23 17:42 ` [PATCH v1 04/19] x86/insn-eval: Handle return values from the decoder Borislav Petkov
@ 2020-12-23 17:42 ` Borislav Petkov
2020-12-23 17:42 ` [PATCH v1 06/19] perf/x86/intel/ds: Check insn_get_length() retval Borislav Petkov
` (14 subsequent siblings)
19 siblings, 0 replies; 42+ messages in thread
From: Borislav Petkov @ 2020-12-23 17:42 UTC (permalink / raw)
To: Andy Lutomirski, Masami Hiramatsu; +Cc: X86 ML, LKML
From: Borislav Petkov <bp@suse.de>
Other than simplifying the code there should be no functional changes
resulting from this.
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/boot/compressed/sev-es.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/arch/x86/boot/compressed/sev-es.c b/arch/x86/boot/compressed/sev-es.c
index 27826c265aab..801c626fc3d4 100644
--- a/arch/x86/boot/compressed/sev-es.c
+++ b/arch/x86/boot/compressed/sev-es.c
@@ -78,16 +78,15 @@ static inline void sev_es_wr_ghcb_msr(u64 val)
static enum es_result vc_decode_insn(struct es_em_ctxt *ctxt)
{
char buffer[MAX_INSN_SIZE];
- enum es_result ret;
+ int ret;
memcpy(buffer, (unsigned char *)ctxt->regs->ip, MAX_INSN_SIZE);
- insn_init(&ctxt->insn, buffer, MAX_INSN_SIZE, 1);
- insn_get_length(&ctxt->insn);
+ ret = insn_decode(&ctxt->insn, buffer, MAX_INSN_SIZE, INSN_MODE_64);
+ if (ret < 0)
+ return ES_DECODE_FAILED;
- ret = ctxt->insn.immediate.got ? ES_OK : ES_DECODE_FAILED;
-
- return ret;
+ return ES_OK;
}
static enum es_result vc_write_mem(struct es_em_ctxt *ctxt,
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v1 06/19] perf/x86/intel/ds: Check insn_get_length() retval
2020-12-23 17:42 [PATCH v1 00/19] x86/insn: Add an insn_decode() API Borislav Petkov
` (4 preceding siblings ...)
2020-12-23 17:42 ` [PATCH v1 05/19] x86/boot/compressed/sev-es: Convert to insn_decode() Borislav Petkov
@ 2020-12-23 17:42 ` Borislav Petkov
2021-01-04 13:19 ` Peter Zijlstra
2020-12-23 17:42 ` [PATCH v1 07/19] perf/x86/intel/ds: Check return values of insn decoder functions Borislav Petkov
` (13 subsequent siblings)
19 siblings, 1 reply; 42+ messages in thread
From: Borislav Petkov @ 2020-12-23 17:42 UTC (permalink / raw)
To: Andy Lutomirski, Masami Hiramatsu; +Cc: X86 ML, LKML
From: Borislav Petkov <bp@suse.de>
intel_pmu_pebs_fixup_ip() needs only the insn length so use the
appropriate helper instead of a full decode. A full decode differs only
in running insn_complete() on the decoded insn but that is not needed
here.
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/events/intel/ds.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
index 67dbc91bccfe..3786b4e07078 100644
--- a/arch/x86/events/intel/ds.c
+++ b/arch/x86/events/intel/ds.c
@@ -1265,14 +1265,14 @@ static int intel_pmu_pebs_fixup_ip(struct pt_regs *regs)
is_64bit = kernel_ip(to) || any_64bit_mode(regs);
#endif
insn_init(&insn, kaddr, size, is_64bit);
- insn_get_length(&insn);
+
/*
* Make sure there was not a problem decoding the
* instruction and getting the length. This is
* doubly important because we have an infinite
* loop if insn.length=0.
*/
- if (!insn.length)
+ if (insn_get_length(&insn) || !insn.length)
break;
to += insn.length;
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v1 06/19] perf/x86/intel/ds: Check insn_get_length() retval
2020-12-23 17:42 ` [PATCH v1 06/19] perf/x86/intel/ds: Check insn_get_length() retval Borislav Petkov
@ 2021-01-04 13:19 ` Peter Zijlstra
2021-01-19 10:40 ` Borislav Petkov
0 siblings, 1 reply; 42+ messages in thread
From: Peter Zijlstra @ 2021-01-04 13:19 UTC (permalink / raw)
To: Borislav Petkov; +Cc: Andy Lutomirski, Masami Hiramatsu, X86 ML, LKML
On Wed, Dec 23, 2020 at 06:42:20PM +0100, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
>
> intel_pmu_pebs_fixup_ip() needs only the insn length so use the
> appropriate helper instead of a full decode. A full decode differs only
> in running insn_complete() on the decoded insn but that is not needed
> here.
>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
> arch/x86/events/intel/ds.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
> index 67dbc91bccfe..3786b4e07078 100644
> --- a/arch/x86/events/intel/ds.c
> +++ b/arch/x86/events/intel/ds.c
> @@ -1265,14 +1265,14 @@ static int intel_pmu_pebs_fixup_ip(struct pt_regs *regs)
> is_64bit = kernel_ip(to) || any_64bit_mode(regs);
> #endif
> insn_init(&insn, kaddr, size, is_64bit);
> - insn_get_length(&insn);
> +
> /*
> * Make sure there was not a problem decoding the
> * instruction and getting the length. This is
> * doubly important because we have an infinite
> * loop if insn.length=0.
> */
> - if (!insn.length)
> + if (insn_get_length(&insn) || !insn.length)
Do we really still need the !insn.length? That is, it *should* be
impossible to not fail insn_get_length() and still have a 0 length,
seeing how x86 doesn't have 0 length instructions.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v1 06/19] perf/x86/intel/ds: Check insn_get_length() retval
2021-01-04 13:19 ` Peter Zijlstra
@ 2021-01-19 10:40 ` Borislav Petkov
0 siblings, 0 replies; 42+ messages in thread
From: Borislav Petkov @ 2021-01-19 10:40 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Andy Lutomirski, Masami Hiramatsu, X86 ML, LKML
On Mon, Jan 04, 2021 at 02:19:19PM +0100, Peter Zijlstra wrote:
> Do we really still need the !insn.length? That is, it *should* be
> impossible to not fail insn_get_length() and still have a 0 length,
> seeing how x86 doesn't have 0 length instructions.
I was responding to the "doubly important" thing in the comment scarying
me about an infinite loop and thus left the length check in, in case
the insn decoder would have a bug and return success but still have
insn.length 0.
With the length check the endless loop won't happen but let's be
brave here ... :-)
So removed.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v1 07/19] perf/x86/intel/ds: Check return values of insn decoder functions
2020-12-23 17:42 [PATCH v1 00/19] x86/insn: Add an insn_decode() API Borislav Petkov
` (5 preceding siblings ...)
2020-12-23 17:42 ` [PATCH v1 06/19] perf/x86/intel/ds: Check insn_get_length() retval Borislav Petkov
@ 2020-12-23 17:42 ` Borislav Petkov
2020-12-23 17:42 ` [PATCH v1 08/19] x86/alternative: Use insn_decode() Borislav Petkov
` (12 subsequent siblings)
19 siblings, 0 replies; 42+ messages in thread
From: Borislav Petkov @ 2020-12-23 17:42 UTC (permalink / raw)
To: Andy Lutomirski, Masami Hiramatsu; +Cc: X86 ML, LKML
From: Borislav Petkov <bp@suse.de>
branch_type() doesn't need to call the full insn_decode() because it
doesn't need it in all cases thus leave the calls separate.
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/events/intel/lbr.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
index 21890dacfcfe..9ecf5028fb8f 100644
--- a/arch/x86/events/intel/lbr.c
+++ b/arch/x86/events/intel/lbr.c
@@ -1224,8 +1224,7 @@ static int branch_type(unsigned long from, unsigned long to, int abort)
is64 = kernel_ip((unsigned long)addr) || any_64bit_mode(current_pt_regs());
#endif
insn_init(&insn, addr, bytes_read, is64);
- insn_get_opcode(&insn);
- if (!insn.opcode.got)
+ if (insn_get_opcode(&insn))
return X86_BR_ABORT;
switch (insn.opcode.bytes[0]) {
@@ -1262,8 +1261,7 @@ static int branch_type(unsigned long from, unsigned long to, int abort)
ret = X86_BR_INT;
break;
case 0xe8: /* call near rel */
- insn_get_immediate(&insn);
- if (insn.immediate1.value == 0) {
+ if (insn_get_immediate(&insn) || insn.immediate1.value == 0) {
/* zero length call */
ret = X86_BR_ZERO_CALL;
break;
@@ -1279,7 +1277,9 @@ static int branch_type(unsigned long from, unsigned long to, int abort)
ret = X86_BR_JMP;
break;
case 0xff: /* call near absolute, call far absolute ind */
- insn_get_modrm(&insn);
+ if (insn_get_modrm(&insn))
+ return X86_BR_ABORT;
+
ext = (insn.modrm.bytes[0] >> 3) & 0x7;
switch (ext) {
case 2: /* near ind call */
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v1 08/19] x86/alternative: Use insn_decode()
2020-12-23 17:42 [PATCH v1 00/19] x86/insn: Add an insn_decode() API Borislav Petkov
` (6 preceding siblings ...)
2020-12-23 17:42 ` [PATCH v1 07/19] perf/x86/intel/ds: Check return values of insn decoder functions Borislav Petkov
@ 2020-12-23 17:42 ` Borislav Petkov
2020-12-23 17:42 ` [PATCH v1 09/19] x86/mce: Convert to insn_decode() Borislav Petkov
` (11 subsequent siblings)
19 siblings, 0 replies; 42+ messages in thread
From: Borislav Petkov @ 2020-12-23 17:42 UTC (permalink / raw)
To: Andy Lutomirski, Masami Hiramatsu; +Cc: X86 ML, LKML
From: Borislav Petkov <bp@suse.de>
No functional changes, just simplification.
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/kernel/alternative.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 8d778e46725d..ce28c5c1deba 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -1274,15 +1274,15 @@ static void text_poke_loc_init(struct text_poke_loc *tp, void *addr,
const void *opcode, size_t len, const void *emulate)
{
struct insn insn;
+ int ret;
memcpy((void *)tp->text, opcode, len);
if (!emulate)
emulate = opcode;
- kernel_insn_init(&insn, emulate, MAX_INSN_SIZE);
- insn_get_length(&insn);
+ ret = insn_decode(&insn, emulate, MAX_INSN_SIZE, INSN_MODE_KERN);
- BUG_ON(!insn_complete(&insn));
+ BUG_ON(ret < 0);
BUG_ON(len != insn.length);
tp->rel_addr = addr - (void *)_stext;
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v1 09/19] x86/mce: Convert to insn_decode()
2020-12-23 17:42 [PATCH v1 00/19] x86/insn: Add an insn_decode() API Borislav Petkov
` (7 preceding siblings ...)
2020-12-23 17:42 ` [PATCH v1 08/19] x86/alternative: Use insn_decode() Borislav Petkov
@ 2020-12-23 17:42 ` Borislav Petkov
2020-12-23 17:42 ` [PATCH v1 10/19] x86/kprobes: " Borislav Petkov
` (10 subsequent siblings)
19 siblings, 0 replies; 42+ messages in thread
From: Borislav Petkov @ 2020-12-23 17:42 UTC (permalink / raw)
To: Andy Lutomirski, Masami Hiramatsu; +Cc: X86 ML, LKML
From: Borislav Petkov <bp@suse.de>
Simplify code, no functional changes.
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/kernel/cpu/mce/severity.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/severity.c b/arch/x86/kernel/cpu/mce/severity.c
index 83df991314c5..a2136ced9d73 100644
--- a/arch/x86/kernel/cpu/mce/severity.c
+++ b/arch/x86/kernel/cpu/mce/severity.c
@@ -218,15 +218,15 @@ static struct severity {
static bool is_copy_from_user(struct pt_regs *regs)
{
u8 insn_buf[MAX_INSN_SIZE];
- struct insn insn;
unsigned long addr;
+ struct insn insn;
+ int ret;
if (copy_from_kernel_nofault(insn_buf, (void *)regs->ip, MAX_INSN_SIZE))
return false;
- kernel_insn_init(&insn, insn_buf, MAX_INSN_SIZE);
- insn_get_opcode(&insn);
- if (!insn.opcode.got)
+ ret = insn_decode(&insn, insn_buf, MAX_INSN_SIZE, INSN_MODE_KERN);
+ if (ret < 0)
return false;
switch (insn.opcode.value) {
@@ -234,10 +234,6 @@ static bool is_copy_from_user(struct pt_regs *regs)
case 0x8A: case 0x8B:
/* MOVZ mem,reg */
case 0xB60F: case 0xB70F:
- insn_get_modrm(&insn);
- insn_get_sib(&insn);
- if (!insn.modrm.got || !insn.sib.got)
- return false;
addr = (unsigned long)insn_get_addr_ref(&insn, regs);
break;
/* REP MOVS */
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v1 10/19] x86/kprobes: Convert to insn_decode()
2020-12-23 17:42 [PATCH v1 00/19] x86/insn: Add an insn_decode() API Borislav Petkov
` (8 preceding siblings ...)
2020-12-23 17:42 ` [PATCH v1 09/19] x86/mce: Convert to insn_decode() Borislav Petkov
@ 2020-12-23 17:42 ` Borislav Petkov
2020-12-23 17:42 ` [PATCH v1 11/19] x86/sev-es: " Borislav Petkov
` (9 subsequent siblings)
19 siblings, 0 replies; 42+ messages in thread
From: Borislav Petkov @ 2020-12-23 17:42 UTC (permalink / raw)
To: Andy Lutomirski, Masami Hiramatsu; +Cc: X86 ML, LKML
From: Borislav Petkov <bp@suse.de>
Simplify code, improve decoding error checking.
Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
---
arch/x86/kernel/kprobes/core.c | 17 +++++++++++------
arch/x86/kernel/kprobes/opt.c | 9 +++++++--
2 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index a65e9e97857f..1cf4b532b798 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -285,6 +285,8 @@ static int can_probe(unsigned long paddr)
/* Decode instructions */
addr = paddr - offset;
while (addr < paddr) {
+ int ret;
+
/*
* Check if the instruction has been modified by another
* kprobe, in which case we replace the breakpoint by the
@@ -296,8 +298,10 @@ static int can_probe(unsigned long paddr)
__addr = recover_probed_instruction(buf, addr);
if (!__addr)
return 0;
- kernel_insn_init(&insn, (void *)__addr, MAX_INSN_SIZE);
- insn_get_length(&insn);
+
+ ret = insn_decode(&insn, (void *)__addr, MAX_INSN_SIZE, INSN_MODE_KERN);
+ if (ret < 0)
+ return 0;
/*
* Another debugging subsystem might insert this breakpoint.
@@ -340,8 +344,8 @@ static int is_IF_modifier(kprobe_opcode_t *insn)
int __copy_instruction(u8 *dest, u8 *src, u8 *real, struct insn *insn)
{
kprobe_opcode_t buf[MAX_INSN_SIZE];
- unsigned long recovered_insn =
- recover_probed_instruction(buf, (unsigned long)src);
+ unsigned long recovered_insn = recover_probed_instruction(buf, (unsigned long)src);
+ int ret;
if (!recovered_insn || !insn)
return 0;
@@ -351,8 +355,9 @@ int __copy_instruction(u8 *dest, u8 *src, u8 *real, struct insn *insn)
MAX_INSN_SIZE))
return 0;
- kernel_insn_init(insn, dest, MAX_INSN_SIZE);
- insn_get_length(insn);
+ ret = insn_decode(insn, dest, MAX_INSN_SIZE, INSN_MODE_KERN);
+ if (ret < 0)
+ return 0;
/* We can not probe force emulate prefixed instruction */
if (insn_has_emulate_prefix(insn))
diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index 08eb23074f92..4299fc865732 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -312,6 +312,8 @@ static int can_optimize(unsigned long paddr)
addr = paddr - offset;
while (addr < paddr - offset + size) { /* Decode until function end */
unsigned long recovered_insn;
+ int ret;
+
if (search_exception_tables(addr))
/*
* Since some fixup code will jumps into this function,
@@ -321,8 +323,11 @@ static int can_optimize(unsigned long paddr)
recovered_insn = recover_probed_instruction(buf, addr);
if (!recovered_insn)
return 0;
- kernel_insn_init(&insn, (void *)recovered_insn, MAX_INSN_SIZE);
- insn_get_length(&insn);
+
+ ret = insn_decode(&insn, (void *)recovered_insn, MAX_INSN_SIZE, INSN_MODE_KERN);
+ if (ret < 0)
+ return 0;
+
/*
* In the case of detecting unknown breakpoint, this could be
* a padding INT3 between functions. Let's check that all the
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v1 11/19] x86/sev-es: Convert to insn_decode()
2020-12-23 17:42 [PATCH v1 00/19] x86/insn: Add an insn_decode() API Borislav Petkov
` (9 preceding siblings ...)
2020-12-23 17:42 ` [PATCH v1 10/19] x86/kprobes: " Borislav Petkov
@ 2020-12-23 17:42 ` Borislav Petkov
2020-12-25 10:50 ` kernel test robot
2020-12-23 17:42 ` [PATCH v1 12/19] x86/traps: " Borislav Petkov
` (8 subsequent siblings)
19 siblings, 1 reply; 42+ messages in thread
From: Borislav Petkov @ 2020-12-23 17:42 UTC (permalink / raw)
To: Andy Lutomirski, Masami Hiramatsu; +Cc: X86 ML, LKML
From: Borislav Petkov <bp@suse.de>
Simplify code, no functional changes.
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/kernel/sev-es.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kernel/sev-es.c b/arch/x86/kernel/sev-es.c
index 37736486603e..564cc9fc693d 100644
--- a/arch/x86/kernel/sev-es.c
+++ b/arch/x86/kernel/sev-es.c
@@ -244,8 +244,7 @@ static int vc_fetch_insn_kernel(struct es_em_ctxt *ctxt,
static enum es_result vc_decode_insn(struct es_em_ctxt *ctxt)
{
char buffer[MAX_INSN_SIZE];
- enum es_result ret;
- int res;
+ int res, ret;
if (user_mode(ctxt->regs)) {
res = insn_fetch_from_user(ctxt->regs, buffer);
@@ -267,13 +266,13 @@ static enum es_result vc_decode_insn(struct es_em_ctxt *ctxt)
return ES_EXCEPTION;
}
- insn_init(&ctxt->insn, buffer, MAX_INSN_SIZE - res, 1);
- insn_get_length(&ctxt->insn);
+ ret = insn_decode(&ctxt->insn, buffer, MAX_INSN_SIZE - res, INSN_MODE_64);
}
- ret = ctxt->insn.immediate.got ? ES_OK : ES_DECODE_FAILED;
-
- return ret;
+ if (ret < 0)
+ return ES_DECODE_FAILED;
+ else
+ return ES_OK;
}
static enum es_result vc_write_mem(struct es_em_ctxt *ctxt,
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v1 11/19] x86/sev-es: Convert to insn_decode()
2020-12-23 17:42 ` [PATCH v1 11/19] x86/sev-es: " Borislav Petkov
@ 2020-12-25 10:50 ` kernel test robot
2020-12-25 12:33 ` Borislav Petkov
0 siblings, 1 reply; 42+ messages in thread
From: kernel test robot @ 2020-12-25 10:50 UTC (permalink / raw)
To: Borislav Petkov, Andy Lutomirski, Masami Hiramatsu
Cc: kbuild-all, clang-built-linux, X86 ML, LKML
[-- Attachment #1: Type: text/plain, Size: 5246 bytes --]
Hi Borislav,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.10 next-20201223]
[cannot apply to tip/perf/core tip/x86/core luto/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Borislav-Petkov/x86-insn-Add-an-insn_decode-API/20201224-014846
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 614cb5894306cfa2c7d9b6168182876ff5948735
config: x86_64-randconfig-a016-20201223 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project cee1e7d14f4628d6174b33640d502bff3b54ae45)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/9cc93591504c88c42ab10903fc69062fc1461ed0
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Borislav-Petkov/x86-insn-Add-an-insn_decode-API/20201224-014846
git checkout 9cc93591504c88c42ab10903fc69062fc1461ed0
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> arch/x86/kernel/sev-es.c:258:7: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (!insn_decode_regs(&ctxt->insn, ctxt->regs, buffer, res))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/x86/kernel/sev-es.c:272:6: note: uninitialized use occurs here
if (ret < 0)
^~~
arch/x86/kernel/sev-es.c:258:3: note: remove the 'if' if its condition is always true
if (!insn_decode_regs(&ctxt->insn, ctxt->regs, buffer, res))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/x86/kernel/sev-es.c:247:14: note: initialize the variable 'ret' to silence this warning
int res, ret;
^
= 0
1 warning generated.
vim +258 arch/x86/kernel/sev-es.c
f980f9c31a923e9 Joerg Roedel 2020-09-07 243
f980f9c31a923e9 Joerg Roedel 2020-09-07 244 static enum es_result vc_decode_insn(struct es_em_ctxt *ctxt)
f980f9c31a923e9 Joerg Roedel 2020-09-07 245 {
f980f9c31a923e9 Joerg Roedel 2020-09-07 246 char buffer[MAX_INSN_SIZE];
9cc93591504c88c Borislav Petkov 2020-12-23 247 int res, ret;
f980f9c31a923e9 Joerg Roedel 2020-09-07 248
5e3427a7bc432ed Joerg Roedel 2020-09-07 249 if (user_mode(ctxt->regs)) {
5e3427a7bc432ed Joerg Roedel 2020-09-07 250 res = insn_fetch_from_user(ctxt->regs, buffer);
5e3427a7bc432ed Joerg Roedel 2020-09-07 251 if (!res) {
5e3427a7bc432ed Joerg Roedel 2020-09-07 252 ctxt->fi.vector = X86_TRAP_PF;
5e3427a7bc432ed Joerg Roedel 2020-09-07 253 ctxt->fi.error_code = X86_PF_INSTR | X86_PF_USER;
5e3427a7bc432ed Joerg Roedel 2020-09-07 254 ctxt->fi.cr2 = ctxt->regs->ip;
5e3427a7bc432ed Joerg Roedel 2020-09-07 255 return ES_EXCEPTION;
5e3427a7bc432ed Joerg Roedel 2020-09-07 256 }
5e3427a7bc432ed Joerg Roedel 2020-09-07 257
63d702bf108e3ad Borislav Petkov 2020-12-23 @258 if (!insn_decode_regs(&ctxt->insn, ctxt->regs, buffer, res))
5e3427a7bc432ed Joerg Roedel 2020-09-07 259 return ES_DECODE_FAILED;
5e3427a7bc432ed Joerg Roedel 2020-09-07 260 } else {
f980f9c31a923e9 Joerg Roedel 2020-09-07 261 res = vc_fetch_insn_kernel(ctxt, buffer);
5e3427a7bc432ed Joerg Roedel 2020-09-07 262 if (res) {
f980f9c31a923e9 Joerg Roedel 2020-09-07 263 ctxt->fi.vector = X86_TRAP_PF;
5e3427a7bc432ed Joerg Roedel 2020-09-07 264 ctxt->fi.error_code = X86_PF_INSTR;
f980f9c31a923e9 Joerg Roedel 2020-09-07 265 ctxt->fi.cr2 = ctxt->regs->ip;
f980f9c31a923e9 Joerg Roedel 2020-09-07 266 return ES_EXCEPTION;
f980f9c31a923e9 Joerg Roedel 2020-09-07 267 }
f980f9c31a923e9 Joerg Roedel 2020-09-07 268
9cc93591504c88c Borislav Petkov 2020-12-23 269 ret = insn_decode(&ctxt->insn, buffer, MAX_INSN_SIZE - res, INSN_MODE_64);
5e3427a7bc432ed Joerg Roedel 2020-09-07 270 }
f980f9c31a923e9 Joerg Roedel 2020-09-07 271
9cc93591504c88c Borislav Petkov 2020-12-23 272 if (ret < 0)
9cc93591504c88c Borislav Petkov 2020-12-23 273 return ES_DECODE_FAILED;
9cc93591504c88c Borislav Petkov 2020-12-23 274 else
9cc93591504c88c Borislav Petkov 2020-12-23 275 return ES_OK;
f980f9c31a923e9 Joerg Roedel 2020-09-07 276 }
f980f9c31a923e9 Joerg Roedel 2020-09-07 277
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27692 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v1 11/19] x86/sev-es: Convert to insn_decode()
2020-12-25 10:50 ` kernel test robot
@ 2020-12-25 12:33 ` Borislav Petkov
2020-12-28 19:15 ` Sean Christopherson
0 siblings, 1 reply; 42+ messages in thread
From: Borislav Petkov @ 2020-12-25 12:33 UTC (permalink / raw)
To: kernel test robot
Cc: Andy Lutomirski, Masami Hiramatsu, kbuild-all, clang-built-linux,
X86 ML, LKML
On Fri, Dec 25, 2020 at 06:50:33PM +0800, kernel test robot wrote:
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
>
> All warnings (new ones prefixed by >>):
>
> >> arch/x86/kernel/sev-es.c:258:7: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
> if (!insn_decode_regs(&ctxt->insn, ctxt->regs, buffer, res))
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Yeah, good catch, thanks for reporting.
Frankly, the readability and "extensiblity" of that function can be
improved by splitting the two cases (diff ontop):
---
diff --git a/arch/x86/kernel/sev-es.c b/arch/x86/kernel/sev-es.c
index 564cc9fc693d..ea47037f1624 100644
--- a/arch/x86/kernel/sev-es.c
+++ b/arch/x86/kernel/sev-es.c
@@ -241,40 +241,53 @@ static int vc_fetch_insn_kernel(struct es_em_ctxt *ctxt,
return copy_from_kernel_nofault(buffer, (unsigned char *)ctxt->regs->ip, MAX_INSN_SIZE);
}
-static enum es_result vc_decode_insn(struct es_em_ctxt *ctxt)
+static enum es_result __vc_decode_user_insn(struct es_em_ctxt *ctxt)
{
char buffer[MAX_INSN_SIZE];
- int res, ret;
-
- if (user_mode(ctxt->regs)) {
- res = insn_fetch_from_user(ctxt->regs, buffer);
- if (!res) {
- ctxt->fi.vector = X86_TRAP_PF;
- ctxt->fi.error_code = X86_PF_INSTR | X86_PF_USER;
- ctxt->fi.cr2 = ctxt->regs->ip;
- return ES_EXCEPTION;
- }
+ int res;
- if (!insn_decode_regs(&ctxt->insn, ctxt->regs, buffer, res))
- return ES_DECODE_FAILED;
- } else {
- res = vc_fetch_insn_kernel(ctxt, buffer);
- if (res) {
- ctxt->fi.vector = X86_TRAP_PF;
- ctxt->fi.error_code = X86_PF_INSTR;
- ctxt->fi.cr2 = ctxt->regs->ip;
- return ES_EXCEPTION;
- }
+ res = insn_fetch_from_user(ctxt->regs, buffer);
+ if (!res) {
+ ctxt->fi.vector = X86_TRAP_PF;
+ ctxt->fi.error_code = X86_PF_INSTR | X86_PF_USER;
+ ctxt->fi.cr2 = ctxt->regs->ip;
+ return ES_EXCEPTION;
+ }
+
+ if (!insn_decode_regs(&ctxt->insn, ctxt->regs, buffer, res))
+ return ES_DECODE_FAILED;
+ else
+ return ES_OK;
+}
+
+static enum es_result __vc_decode_kern_insn(struct es_em_ctxt *ctxt)
+{
+ char buffer[MAX_INSN_SIZE];
+ int res;
- ret = insn_decode(&ctxt->insn, buffer, MAX_INSN_SIZE - res, INSN_MODE_64);
+ res = vc_fetch_insn_kernel(ctxt, buffer);
+ if (res) {
+ ctxt->fi.vector = X86_TRAP_PF;
+ ctxt->fi.error_code = X86_PF_INSTR;
+ ctxt->fi.cr2 = ctxt->regs->ip;
+ return ES_EXCEPTION;
}
- if (ret < 0)
+ res = insn_decode(&ctxt->insn, buffer, MAX_INSN_SIZE - res, INSN_MODE_64);
+ if (res < 0)
return ES_DECODE_FAILED;
else
return ES_OK;
}
+static enum es_result vc_decode_insn(struct es_em_ctxt *ctxt)
+{
+ if (user_mode(ctxt->regs))
+ return __vc_decode_user_insn(ctxt);
+ else
+ return __vc_decode_kern_insn(ctxt);
+}
+
static enum es_result vc_write_mem(struct es_em_ctxt *ctxt,
char *dst, char *buf, size_t size)
{
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v1 11/19] x86/sev-es: Convert to insn_decode()
2020-12-25 12:33 ` Borislav Petkov
@ 2020-12-28 19:15 ` Sean Christopherson
2021-01-21 16:58 ` Borislav Petkov
0 siblings, 1 reply; 42+ messages in thread
From: Sean Christopherson @ 2020-12-28 19:15 UTC (permalink / raw)
To: Borislav Petkov
Cc: kernel test robot, Andy Lutomirski, Masami Hiramatsu, kbuild-all,
clang-built-linux, X86 ML, LKML
On Fri, Dec 25, 2020, Borislav Petkov wrote:
> On Fri, Dec 25, 2020 at 06:50:33PM +0800, kernel test robot wrote:
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <lkp@intel.com>
> >
> > All warnings (new ones prefixed by >>):
> >
> > >> arch/x86/kernel/sev-es.c:258:7: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
> > if (!insn_decode_regs(&ctxt->insn, ctxt->regs, buffer, res))
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Yeah, good catch, thanks for reporting.
>
> Frankly, the readability and "extensiblity" of that function can be
> improved by splitting the two cases (diff ontop):
Alternatively, could the kernel case use insn_decode_regs()? If
vc_fetch_insn_kernel() were also modified to mirror insn_fetch_from_user(), the
two code paths could be unified except for the the fetch and the PFEC. E.g.
static int vc_fetch_insn_kernel(struct es_em_ctxt *ctxt,
unsigned char *buffer)
{
if (copy_from_kernel_nofault(buffer, (unsigned char *)ctxt->regs->ip, MAX_INSN_SIZE))
return 0;
return MAX_INSN_SIZE;
}
static enum es_result vc_decode_insn(struct es_em_ctxt *ctxt)
{
char buffer[MAX_INSN_SIZE];
int nbytes;
if (user_mode(ctxt->regs))
nbytes = insn_fetch_from_user(ctxt->regs, buffer);
else
nbytes = vc_fetch_insn_kernel(ctxt, buffer);
if (!nbytes) {
ctxt->fi.vector = X86_TRAP_PF;
ctxt->fi.error_code = X86_PF_INSTR;
if (user_mode(ctxt->regs))
ctxt->fi.error_code |= X86_PF_USER;
ctxt->fi.cr2 = ctxt->regs->ip;
return ES_EXCEPTION;
}
if (!insn_decode_regs(&ctxt->insn, ctxt->regs, buffer, nbytes))
return ES_DECODE_FAILED;
return ctxt->insn.immediate.got ? ES_OK : ES_DECODE_FAILED;
}
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v1 11/19] x86/sev-es: Convert to insn_decode()
2020-12-28 19:15 ` Sean Christopherson
@ 2021-01-21 16:58 ` Borislav Petkov
2021-01-21 22:35 ` Sean Christopherson
0 siblings, 1 reply; 42+ messages in thread
From: Borislav Petkov @ 2021-01-21 16:58 UTC (permalink / raw)
To: Sean Christopherson
Cc: kernel test robot, Andy Lutomirski, Masami Hiramatsu, kbuild-all,
clang-built-linux, X86 ML, LKML
On Mon, Dec 28, 2020 at 11:15:11AM -0800, Sean Christopherson wrote:
> Alternatively, could the kernel case use insn_decode_regs()? If
> vc_fetch_insn_kernel() were also modified to mirror insn_fetch_from_user(), the
> two code paths could be unified except for the the fetch and the PFEC. E.g.
Personal Firearms Eligibility Check?
In any case, I prefer simple, easy to follow code at a quick glance.
Stuff like...
>
> static int vc_fetch_insn_kernel(struct es_em_ctxt *ctxt,
> unsigned char *buffer)
> {
> if (copy_from_kernel_nofault(buffer, (unsigned char *)ctxt->regs->ip, MAX_INSN_SIZE))
> return 0;
>
> return MAX_INSN_SIZE;
> }
>
> static enum es_result vc_decode_insn(struct es_em_ctxt *ctxt)
> {
> char buffer[MAX_INSN_SIZE];
> int nbytes;
>
> if (user_mode(ctxt->regs))
> nbytes = insn_fetch_from_user(ctxt->regs, buffer);
> else
> nbytes = vc_fetch_insn_kernel(ctxt, buffer);
>
> if (!nbytes) {
> ctxt->fi.vector = X86_TRAP_PF;
> ctxt->fi.error_code = X86_PF_INSTR;
> if (user_mode(ctxt->regs))
... this second repeated check here is not what I would call that.
But this is my personal preference only so it's up for a vote now.
:-)
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v1 11/19] x86/sev-es: Convert to insn_decode()
2021-01-21 16:58 ` Borislav Petkov
@ 2021-01-21 22:35 ` Sean Christopherson
0 siblings, 0 replies; 42+ messages in thread
From: Sean Christopherson @ 2021-01-21 22:35 UTC (permalink / raw)
To: Borislav Petkov
Cc: kernel test robot, Andy Lutomirski, Masami Hiramatsu, kbuild-all,
clang-built-linux, X86 ML, LKML
On Thu, Jan 21, 2021, Borislav Petkov wrote:
> On Mon, Dec 28, 2020 at 11:15:11AM -0800, Sean Christopherson wrote:
> > Alternatively, could the kernel case use insn_decode_regs()? If
> > vc_fetch_insn_kernel() were also modified to mirror insn_fetch_from_user(), the
> > two code paths could be unified except for the the fetch and the PFEC. E.g.
>
> Personal Firearms Eligibility Check?
Ha, Page Fault Error Code.
> In any case, I prefer simple, easy to follow code at a quick glance.
> Stuff like...
>
> >
> > static int vc_fetch_insn_kernel(struct es_em_ctxt *ctxt,
> > unsigned char *buffer)
> > {
> > if (copy_from_kernel_nofault(buffer, (unsigned char *)ctxt->regs->ip, MAX_INSN_SIZE))
> > return 0;
> >
> > return MAX_INSN_SIZE;
> > }
> >
> > static enum es_result vc_decode_insn(struct es_em_ctxt *ctxt)
> > {
> > char buffer[MAX_INSN_SIZE];
> > int nbytes;
> >
> > if (user_mode(ctxt->regs))
> > nbytes = insn_fetch_from_user(ctxt->regs, buffer);
> > else
> > nbytes = vc_fetch_insn_kernel(ctxt, buffer);
> >
> > if (!nbytes) {
> > ctxt->fi.vector = X86_TRAP_PF;
> > ctxt->fi.error_code = X86_PF_INSTR;
> > if (user_mode(ctxt->regs))
>
> ... this second repeated check here is not what I would call that.
But then you're stuck maintaining two separate flows that do the same thing.
> But this is my personal preference only so it's up for a vote now.
Rock Paper Scissors?
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v1 12/19] x86/traps: Convert to insn_decode()
2020-12-23 17:42 [PATCH v1 00/19] x86/insn: Add an insn_decode() API Borislav Petkov
` (10 preceding siblings ...)
2020-12-23 17:42 ` [PATCH v1 11/19] x86/sev-es: " Borislav Petkov
@ 2020-12-23 17:42 ` Borislav Petkov
2020-12-23 17:42 ` [PATCH v1 13/19] x86/uprobes: " Borislav Petkov
` (7 subsequent siblings)
19 siblings, 0 replies; 42+ messages in thread
From: Borislav Petkov @ 2020-12-23 17:42 UTC (permalink / raw)
To: Andy Lutomirski, Masami Hiramatsu; +Cc: X86 ML, LKML
From: Borislav Petkov <bp@suse.de>
Simplify code, no functional changes.
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/kernel/traps.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index fb55981f2a0d..861962e1b6e8 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -497,14 +497,15 @@ static enum kernel_gp_hint get_kernel_gp_address(struct pt_regs *regs,
{
u8 insn_buf[MAX_INSN_SIZE];
struct insn insn;
+ int ret;
if (copy_from_kernel_nofault(insn_buf, (void *)regs->ip,
MAX_INSN_SIZE))
return GP_NO_HINT;
- kernel_insn_init(&insn, insn_buf, MAX_INSN_SIZE);
- insn_get_modrm(&insn);
- insn_get_sib(&insn);
+ ret = insn_decode(&insn, insn_buf, MAX_INSN_SIZE, INSN_MODE_KERN);
+ if (ret < 0)
+ return GP_NO_HINT;
*addr = (unsigned long)insn_get_addr_ref(&insn, regs);
if (*addr == -1UL)
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v1 13/19] x86/uprobes: Convert to insn_decode()
2020-12-23 17:42 [PATCH v1 00/19] x86/insn: Add an insn_decode() API Borislav Petkov
` (11 preceding siblings ...)
2020-12-23 17:42 ` [PATCH v1 12/19] x86/traps: " Borislav Petkov
@ 2020-12-23 17:42 ` Borislav Petkov
2020-12-23 17:42 ` [PATCH v1 14/19] x86/tools/insn_decoder_test: " Borislav Petkov
` (6 subsequent siblings)
19 siblings, 0 replies; 42+ messages in thread
From: Borislav Petkov @ 2020-12-23 17:42 UTC (permalink / raw)
To: Andy Lutomirski, Masami Hiramatsu; +Cc: X86 ML, LKML
From: Borislav Petkov <bp@suse.de>
Simplify code, no functional changes.
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/kernel/uprobes.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index a2b413394917..b63cf8f7745e 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -276,12 +276,12 @@ static bool is_prefix_bad(struct insn *insn)
static int uprobe_init_insn(struct arch_uprobe *auprobe, struct insn *insn, bool x86_64)
{
+ enum insn_mode m = x86_64 ? INSN_MODE_64 : INSN_MODE_32;
u32 volatile *good_insns;
+ int ret;
- insn_init(insn, auprobe->insn, sizeof(auprobe->insn), x86_64);
- /* has the side-effect of processing the entire instruction */
- insn_get_length(insn);
- if (!insn_complete(insn))
+ ret = insn_decode(insn, auprobe->insn, sizeof(auprobe->insn), m);
+ if (ret < 0)
return -ENOEXEC;
if (is_prefix_bad(insn))
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v1 14/19] x86/tools/insn_decoder_test: Convert to insn_decode()
2020-12-23 17:42 [PATCH v1 00/19] x86/insn: Add an insn_decode() API Borislav Petkov
` (12 preceding siblings ...)
2020-12-23 17:42 ` [PATCH v1 13/19] x86/uprobes: " Borislav Petkov
@ 2020-12-23 17:42 ` Borislav Petkov
2020-12-23 17:42 ` [PATCH v1 15/19] tools/objtool: " Borislav Petkov
` (5 subsequent siblings)
19 siblings, 0 replies; 42+ messages in thread
From: Borislav Petkov @ 2020-12-23 17:42 UTC (permalink / raw)
To: Andy Lutomirski, Masami Hiramatsu; +Cc: X86 ML, LKML
From: Borislav Petkov <bp@suse.de>
Simplify code, no functional changes.
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/tools/insn_decoder_test.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/arch/x86/tools/insn_decoder_test.c b/arch/x86/tools/insn_decoder_test.c
index 34eda63c124b..472540aeabc2 100644
--- a/arch/x86/tools/insn_decoder_test.c
+++ b/arch/x86/tools/insn_decoder_test.c
@@ -120,7 +120,7 @@ int main(int argc, char **argv)
while (fgets(line, BUFSIZE, stdin)) {
char copy[BUFSIZE], *s, *tab1, *tab2;
- int nb = 0;
+ int nb = 0, ret;
unsigned int b;
if (line[0] == '<') {
@@ -148,10 +148,12 @@ int main(int argc, char **argv)
} else
break;
}
+
/* Decode an instruction */
- insn_init(&insn, insn_buff, sizeof(insn_buff), x86_64);
- insn_get_length(&insn);
- if (insn.length != nb) {
+ ret = insn_decode(&insn, insn_buff, sizeof(insn_buff),
+ x86_64 ? INSN_MODE_64 : INSN_MODE_32);
+
+ if (ret < 0 || insn.length != nb) {
warnings++;
pr_warn("Found an x86 instruction decoder bug, "
"please report this.\n", sym);
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v1 15/19] tools/objtool: Convert to insn_decode()
2020-12-23 17:42 [PATCH v1 00/19] x86/insn: Add an insn_decode() API Borislav Petkov
` (13 preceding siblings ...)
2020-12-23 17:42 ` [PATCH v1 14/19] x86/tools/insn_decoder_test: " Borislav Petkov
@ 2020-12-23 17:42 ` Borislav Petkov
2020-12-23 17:42 ` [PATCH v1 16/19] x86/tools/insn_sanity: " Borislav Petkov
` (4 subsequent siblings)
19 siblings, 0 replies; 42+ messages in thread
From: Borislav Petkov @ 2020-12-23 17:42 UTC (permalink / raw)
To: Andy Lutomirski, Masami Hiramatsu; +Cc: X86 ML, LKML
From: Borislav Petkov <bp@suse.de>
Simplify code, no functional changes.
Signed-off-by: Borislav Petkov <bp@suse.de>
---
tools/objtool/arch/x86/decode.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index cde9c36e40ae..67ee8d2a9e5c 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -90,7 +90,7 @@ int arch_decode_instruction(const struct elf *elf, const struct section *sec,
struct list_head *ops_list)
{
struct insn insn;
- int x86_64, sign;
+ int x86_64, sign, ret;
unsigned char op1, op2, rex = 0, rex_b = 0, rex_r = 0, rex_w = 0,
rex_x = 0, modrm = 0, modrm_mod = 0, modrm_rm = 0,
modrm_reg = 0, sib = 0;
@@ -101,10 +101,9 @@ int arch_decode_instruction(const struct elf *elf, const struct section *sec,
if (x86_64 == -1)
return -1;
- insn_init(&insn, sec->data->d_buf + offset, maxlen, x86_64);
- insn_get_length(&insn);
-
- if (!insn_complete(&insn)) {
+ ret = insn_decode(&insn, sec->data->d_buf + offset, maxlen,
+ x86_64 ? INSN_MODE_64 : INSN_MODE_32);
+ if (ret < 0) {
WARN("can't decode instruction at %s:0x%lx", sec->name, offset);
return -1;
}
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v1 16/19] x86/tools/insn_sanity: Convert to insn_decode()
2020-12-23 17:42 [PATCH v1 00/19] x86/insn: Add an insn_decode() API Borislav Petkov
` (14 preceding siblings ...)
2020-12-23 17:42 ` [PATCH v1 15/19] tools/objtool: " Borislav Petkov
@ 2020-12-23 17:42 ` Borislav Petkov
2020-12-23 17:42 ` [PATCH v1 17/19] tools/perf: " Borislav Petkov
` (3 subsequent siblings)
19 siblings, 0 replies; 42+ messages in thread
From: Borislav Petkov @ 2020-12-23 17:42 UTC (permalink / raw)
To: Andy Lutomirski, Masami Hiramatsu; +Cc: X86 ML, LKML
From: Borislav Petkov <bp@suse.de>
Simplify code, no functional changes.
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/tools/insn_sanity.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/x86/tools/insn_sanity.c b/arch/x86/tools/insn_sanity.c
index 185ceba9d289..51309df285b4 100644
--- a/arch/x86/tools/insn_sanity.c
+++ b/arch/x86/tools/insn_sanity.c
@@ -222,8 +222,8 @@ static void parse_args(int argc, char **argv)
int main(int argc, char **argv)
{
+ int insns = 0, ret;
struct insn insn;
- int insns = 0;
int errors = 0;
unsigned long i;
unsigned char insn_buff[MAX_INSN_SIZE * 2];
@@ -241,15 +241,15 @@ int main(int argc, char **argv)
continue;
/* Decode an instruction */
- insn_init(&insn, insn_buff, sizeof(insn_buff), x86_64);
- insn_get_length(&insn);
+ ret = insn_decode(&insn, insn_buff, sizeof(insn_buff),
+ x86_64 ? INSN_MODE_64 : INSN_MODE_32);
if (insn.next_byte <= insn.kaddr ||
insn.kaddr + MAX_INSN_SIZE < insn.next_byte) {
/* Access out-of-range memory */
dump_stream(stderr, "Error: Found an access violation", i, insn_buff, &insn);
errors++;
- } else if (verbose && !insn_complete(&insn))
+ } else if (verbose && ret < 0)
dump_stream(stdout, "Info: Found an undecodable input", i, insn_buff, &insn);
else if (verbose >= 2)
dump_insn(stdout, &insn);
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v1 17/19] tools/perf: Convert to insn_decode()
2020-12-23 17:42 [PATCH v1 00/19] x86/insn: Add an insn_decode() API Borislav Petkov
` (15 preceding siblings ...)
2020-12-23 17:42 ` [PATCH v1 16/19] x86/tools/insn_sanity: " Borislav Petkov
@ 2020-12-23 17:42 ` Borislav Petkov
2020-12-23 17:42 ` [PATCH v1 18/19] x86/insn: Remove kernel_insn_init() Borislav Petkov
` (2 subsequent siblings)
19 siblings, 0 replies; 42+ messages in thread
From: Borislav Petkov @ 2020-12-23 17:42 UTC (permalink / raw)
To: Andy Lutomirski, Masami Hiramatsu; +Cc: X86 ML, LKML, Arnaldo Carvalho de Melo
From: Borislav Petkov <bp@suse.de>
Simplify code, no functional changes.
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/arch/x86/tests/insn-x86.c | 9 ++++-----
tools/perf/arch/x86/util/archinsn.c | 9 +++++----
.../intel-pt-decoder/intel-pt-insn-decoder.c | 17 ++++++++++-------
3 files changed, 19 insertions(+), 16 deletions(-)
diff --git a/tools/perf/arch/x86/tests/insn-x86.c b/tools/perf/arch/x86/tests/insn-x86.c
index 745f29adb14b..6c7970b3acb6 100644
--- a/tools/perf/arch/x86/tests/insn-x86.c
+++ b/tools/perf/arch/x86/tests/insn-x86.c
@@ -95,13 +95,12 @@ static int get_branch(const char *branch_str)
static int test_data_item(struct test_data *dat, int x86_64)
{
struct intel_pt_insn intel_pt_insn;
+ int op, branch, ret;
struct insn insn;
- int op, branch;
- insn_init(&insn, dat->data, MAX_INSN_SIZE, x86_64);
- insn_get_length(&insn);
-
- if (!insn_complete(&insn)) {
+ ret = insn_decode(&insn, dat->data, MAX_INSN_SIZE,
+ x86_64 ? INSN_MODE_64 : INSN_MODE_32);
+ if (ret < 0) {
pr_debug("Failed to decode: %s\n", dat->asm_rep);
return -1;
}
diff --git a/tools/perf/arch/x86/util/archinsn.c b/tools/perf/arch/x86/util/archinsn.c
index 3e6791531ca5..9fb12e8e67eb 100644
--- a/tools/perf/arch/x86/util/archinsn.c
+++ b/tools/perf/arch/x86/util/archinsn.c
@@ -11,7 +11,7 @@ void arch_fetch_insn(struct perf_sample *sample,
struct machine *machine)
{
struct insn insn;
- int len;
+ int len, ret;
bool is64bit = false;
if (!sample->ip)
@@ -19,8 +19,9 @@ void arch_fetch_insn(struct perf_sample *sample,
len = thread__memcpy(thread, machine, sample->insn, sample->ip, sizeof(sample->insn), &is64bit);
if (len <= 0)
return;
- insn_init(&insn, sample->insn, len, is64bit);
- insn_get_length(&insn);
- if (insn_complete(&insn) && insn.length <= len)
+
+ ret = insn_decode(&insn, sample->insn, len,
+ is64bit ? INSN_MODE_64 : INSN_MODE_32);
+ if (ret >= 0 && insn.length <= len)
sample->insn_len = insn.length;
}
diff --git a/tools/perf/util/intel-pt-decoder/intel-pt-insn-decoder.c b/tools/perf/util/intel-pt-decoder/intel-pt-insn-decoder.c
index fb8a3558d3d5..56b42545946e 100644
--- a/tools/perf/util/intel-pt-decoder/intel-pt-insn-decoder.c
+++ b/tools/perf/util/intel-pt-decoder/intel-pt-insn-decoder.c
@@ -158,11 +158,13 @@ int intel_pt_get_insn(const unsigned char *buf, size_t len, int x86_64,
struct intel_pt_insn *intel_pt_insn)
{
struct insn insn;
+ int ret;
- insn_init(&insn, buf, len, x86_64);
- insn_get_length(&insn);
- if (!insn_complete(&insn) || insn.length > len)
+ ret = insn_decode(&insn, buf, len,
+ x86_64 ? INSN_MODE_64 : INSN_MODE_32);
+ if (ret < 0 || insn.length > len)
return -1;
+
intel_pt_insn_decoder(&insn, intel_pt_insn);
if (insn.length < INTEL_PT_INSN_BUF_SZ)
memcpy(intel_pt_insn->buf, buf, insn.length);
@@ -183,12 +185,13 @@ const char *dump_insn(struct perf_insn *x, uint64_t ip __maybe_unused,
u8 *inbuf, int inlen, int *lenp)
{
struct insn insn;
- int n, i;
+ int n, i, ret;
int left;
- insn_init(&insn, inbuf, inlen, x->is64bit);
- insn_get_length(&insn);
- if (!insn_complete(&insn) || insn.length > inlen)
+ ret = insn_decode(&insn, inbuf, inlen,
+ x->is64bit ? INSN_MODE_64 : INSN_MODE_32);
+
+ if (ret < 0 || insn.length > inlen)
return "<bad>";
if (lenp)
*lenp = insn.length;
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v1 18/19] x86/insn: Remove kernel_insn_init()
2020-12-23 17:42 [PATCH v1 00/19] x86/insn: Add an insn_decode() API Borislav Petkov
` (16 preceding siblings ...)
2020-12-23 17:42 ` [PATCH v1 17/19] tools/perf: " Borislav Petkov
@ 2020-12-23 17:42 ` Borislav Petkov
2020-12-23 17:42 ` [PATCH v1 19/19] x86/insn: Make insn_complete() static Borislav Petkov
2020-12-27 15:26 ` [PATCH v1 00/19] x86/insn: Add an insn_decode() API Tom Lendacky
19 siblings, 0 replies; 42+ messages in thread
From: Borislav Petkov @ 2020-12-23 17:42 UTC (permalink / raw)
To: Andy Lutomirski, Masami Hiramatsu; +Cc: X86 ML, LKML
From: Borislav Petkov <bp@suse.de>
Now that it is not needed anymore, drop it.
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/include/asm/insn.h | 11 -----------
tools/arch/x86/include/asm/insn.h | 11 -----------
2 files changed, 22 deletions(-)
diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
index 088aa90e9158..f01d835b6908 100644
--- a/arch/x86/include/asm/insn.h
+++ b/arch/x86/include/asm/insn.h
@@ -114,17 +114,6 @@ static inline void insn_get_attribute(struct insn *insn)
/* Instruction uses RIP-relative addressing */
extern int insn_rip_relative(struct insn *insn);
-/* Init insn for kernel text */
-static inline void kernel_insn_init(struct insn *insn,
- const void *kaddr, int buf_len)
-{
-#ifdef CONFIG_X86_64
- insn_init(insn, kaddr, buf_len, 1);
-#else /* CONFIG_X86_32 */
- insn_init(insn, kaddr, buf_len, 0);
-#endif
-}
-
static inline int insn_is_avx(struct insn *insn)
{
if (!insn->prefixes.got)
diff --git a/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
index ed43bf01ebcc..870795752d6f 100644
--- a/tools/arch/x86/include/asm/insn.h
+++ b/tools/arch/x86/include/asm/insn.h
@@ -114,17 +114,6 @@ static inline void insn_get_attribute(struct insn *insn)
/* Instruction uses RIP-relative addressing */
extern int insn_rip_relative(struct insn *insn);
-/* Init insn for kernel text */
-static inline void kernel_insn_init(struct insn *insn,
- const void *kaddr, int buf_len)
-{
-#ifdef CONFIG_X86_64
- insn_init(insn, kaddr, buf_len, 1);
-#else /* CONFIG_X86_32 */
- insn_init(insn, kaddr, buf_len, 0);
-#endif
-}
-
static inline int insn_is_avx(struct insn *insn)
{
if (!insn->prefixes.got)
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v1 19/19] x86/insn: Make insn_complete() static
2020-12-23 17:42 [PATCH v1 00/19] x86/insn: Add an insn_decode() API Borislav Petkov
` (17 preceding siblings ...)
2020-12-23 17:42 ` [PATCH v1 18/19] x86/insn: Remove kernel_insn_init() Borislav Petkov
@ 2020-12-23 17:42 ` Borislav Petkov
2020-12-27 15:26 ` [PATCH v1 00/19] x86/insn: Add an insn_decode() API Tom Lendacky
19 siblings, 0 replies; 42+ messages in thread
From: Borislav Petkov @ 2020-12-23 17:42 UTC (permalink / raw)
To: Andy Lutomirski, Masami Hiramatsu; +Cc: X86 ML, LKML
From: Borislav Petkov <bp@suse.de>
... and move it above the only place it is used.
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/include/asm/insn.h | 7 -------
arch/x86/lib/insn.c | 7 +++++++
tools/arch/x86/include/asm/insn.h | 7 -------
tools/arch/x86/lib/insn.c | 7 +++++++
4 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
index f01d835b6908..9f1910284861 100644
--- a/arch/x86/include/asm/insn.h
+++ b/arch/x86/include/asm/insn.h
@@ -133,13 +133,6 @@ static inline int insn_has_emulate_prefix(struct insn *insn)
return !!insn->emulate_prefix_size;
}
-/* Ensure this instruction is decoded completely */
-static inline int insn_complete(struct insn *insn)
-{
- return insn->opcode.got && insn->modrm.got && insn->sib.got &&
- insn->displacement.got && insn->immediate.got;
-}
-
static inline insn_byte_t insn_vex_m_bits(struct insn *insn)
{
if (insn->vex_prefix.nbytes == 2) /* 2 bytes VEX */
diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
index b373aadcdd02..2ab1d0256313 100644
--- a/arch/x86/lib/insn.c
+++ b/arch/x86/lib/insn.c
@@ -726,6 +726,13 @@ int insn_get_length(struct insn *insn)
return 0;
}
+/* Ensure this instruction is decoded completely */
+static inline int insn_complete(struct insn *insn)
+{
+ return insn->opcode.got && insn->modrm.got && insn->sib.got &&
+ insn->displacement.got && insn->immediate.got;
+}
+
/**
* insn_decode() - Decode an x86 instruction
* @insn: &struct insn to be initialized
diff --git a/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
index 870795752d6f..f8772b371452 100644
--- a/tools/arch/x86/include/asm/insn.h
+++ b/tools/arch/x86/include/asm/insn.h
@@ -133,13 +133,6 @@ static inline int insn_has_emulate_prefix(struct insn *insn)
return !!insn->emulate_prefix_size;
}
-/* Ensure this instruction is decoded completely */
-static inline int insn_complete(struct insn *insn)
-{
- return insn->opcode.got && insn->modrm.got && insn->sib.got &&
- insn->displacement.got && insn->immediate.got;
-}
-
static inline insn_byte_t insn_vex_m_bits(struct insn *insn)
{
if (insn->vex_prefix.nbytes == 2) /* 2 bytes VEX */
diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c
index a2f9d9e177f2..c224e1569034 100644
--- a/tools/arch/x86/lib/insn.c
+++ b/tools/arch/x86/lib/insn.c
@@ -726,6 +726,13 @@ int insn_get_length(struct insn *insn)
return 0;
}
+/* Ensure this instruction is decoded completely */
+static inline int insn_complete(struct insn *insn)
+{
+ return insn->opcode.got && insn->modrm.got && insn->sib.got &&
+ insn->displacement.got && insn->immediate.got;
+}
+
/**
* insn_decode() - Decode an x86 instruction
* @insn: &struct insn to be initialized
--
2.29.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v1 00/19] x86/insn: Add an insn_decode() API
2020-12-23 17:42 [PATCH v1 00/19] x86/insn: Add an insn_decode() API Borislav Petkov
` (18 preceding siblings ...)
2020-12-23 17:42 ` [PATCH v1 19/19] x86/insn: Make insn_complete() static Borislav Petkov
@ 2020-12-27 15:26 ` Tom Lendacky
2021-02-03 12:00 ` Borislav Petkov
19 siblings, 1 reply; 42+ messages in thread
From: Tom Lendacky @ 2020-12-27 15:26 UTC (permalink / raw)
To: Borislav Petkov, Andy Lutomirski, Masami Hiramatsu; +Cc: X86 ML, LKML
On 12/23/20 11:42 AM, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
>
> Hi,
>
> here's v1 with the requested change to return -ENODATA on short input to
> the decoder. The rest is as in the previous submission.
>
> Only lightly tested.
>
> Thx.
>
> changelog:
> ==========
>
> That is, provided this is how we want to control what the instruction
> decoder decodes - by supplying the length of the buffer it should look
> at.
>
> We could also say that probably there should be a way to say "decode
> only the first insn in the buffer and ignore the rest". That is all up
> to the use cases so I'm looking for suggestions here.
That's the way it works today, right? One instruction, no matter the
length of the buffer (assuming the length is long enough to include a full
instruction)?
Because the callers of the decode may rely on parsing only the current
instruction (like SEV-ES), it should probably default to that (although
most of the call points are being updated so you could supply a boolean to
indicate one vs many instructions). The caller doesn't necessarily know
the length of the instruction, so it may provide a buffer of max
instruction length.
Also, if you want to parse more than one instruction at a time, wouldn't
you need to maintain register context within the parsing, I don't think
that is done today. Or you could chain together some instruction contexts
to identify each instruction that was parsed?
Thanks,
Tom
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v1 00/19] x86/insn: Add an insn_decode() API
2020-12-27 15:26 ` [PATCH v1 00/19] x86/insn: Add an insn_decode() API Tom Lendacky
@ 2021-02-03 12:00 ` Borislav Petkov
0 siblings, 0 replies; 42+ messages in thread
From: Borislav Petkov @ 2021-02-03 12:00 UTC (permalink / raw)
To: Tom Lendacky; +Cc: Andy Lutomirski, Masami Hiramatsu, X86 ML, LKML
On Sun, Dec 27, 2020 at 09:26:04AM -0600, Tom Lendacky wrote:
> > We could also say that probably there should be a way to say "decode
> > only the first insn in the buffer and ignore the rest". That is all up
> > to the use cases so I'm looking for suggestions here.
>
> That's the way it works today, right? One instruction, no matter the length
> of the buffer (assuming the length is long enough to include a full
> instruction)?
>
> Because the callers of the decode may rely on parsing only the current
> instruction (like SEV-ES), it should probably default to that (although most
> of the call points are being updated so you could supply a boolean to
> indicate one vs many instructions). The caller doesn't necessarily know the
> length of the instruction, so it may provide a buffer of max instruction
> length.
>
> Also, if you want to parse more than one instruction at a time, wouldn't you
> need to maintain register context within the parsing, I don't think that is
> done today. Or you could chain together some instruction contexts to
> identify each instruction that was parsed?
Yah, let's leave it all to the one who actually needs multiple insn
parsing.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 42+ messages in thread