From: Borislav Petkov <[email protected]>
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:
==========
v0:
---
https://lkml.kernel.org/r/[email protected]
here's what I had in mind, finally split into proper patches. The final
goal is for all users of the decoder to simply call insn_decode() and
look at its retval. Simple.
As to amluto's question about detecting partial insns, see the diff
below.
Running that gives:
insn buffer:
0x48 0xcf 0x48 0x83 0x90 0x90 0x90 0x90 0x90 0x90 0x90 0x90 0x90 0x90 0x90
supplied buf size: 15, ret 0
supplied buf size: 2, ret 0
supplied buf size: 3, ret 0
supplied buf size: 4, ret 0
and the return value is always success.
Which means that that buf_len that gets supplied to the decoder
functions doesn't really work and I need to look into it.
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.
In any case, at least the case where I give it
0x48 0xcf 0x48 0x83
and say that buf size is 4, should return an error because the second
insn is incomplete. So I need to go look at that now.
Thx.
Borislav Petkov (19):
x86/insn: Rename insn_decode() to insn_decode_regs()
x86/insn: Add @buf_len param to insn_init() kernel-doc comment
x86/insn: Add an insn_decode() API
x86/insn-eval: Handle return values from the decoder
x86/boot/compressed/sev-es: Convert to insn_decode()
perf/x86/intel/ds: Check insn_get_length() retval
perf/x86/intel/ds: Check return values of insn decoder functions
x86/alternative: Use insn_decode()
x86/mce: Convert to insn_decode()
x86/kprobes: Convert to insn_decode()
x86/sev-es: Convert to insn_decode()
x86/traps: Convert to insn_decode()
x86/uprobes: Convert to insn_decode()
x86/tools/insn_decoder_test: Convert to insn_decode()
tools/objtool: Convert to insn_decode()
x86/tools/insn_sanity: Convert to insn_decode()
tools/perf: Convert to insn_decode()
x86/insn: Remove kernel_insn_init()
x86/insn: Make insn_complete() static
arch/x86/boot/compressed/sev-es.c | 11 +-
arch/x86/events/intel/ds.c | 4 +-
arch/x86/events/intel/lbr.c | 10 +-
arch/x86/include/asm/insn-eval.h | 4 +-
arch/x86/include/asm/insn.h | 42 ++-
arch/x86/kernel/alternative.c | 6 +-
arch/x86/kernel/cpu/mce/severity.c | 12 +-
arch/x86/kernel/kprobes/core.c | 17 +-
arch/x86/kernel/kprobes/opt.c | 9 +-
arch/x86/kernel/sev-es.c | 15 +-
arch/x86/kernel/traps.c | 7 +-
arch/x86/kernel/umip.c | 2 +-
arch/x86/kernel/uprobes.c | 8 +-
arch/x86/lib/insn-eval.c | 25 +-
arch/x86/lib/insn.c | 247 ++++++++++++++----
arch/x86/tools/insn_decoder_test.c | 10 +-
arch/x86/tools/insn_sanity.c | 8 +-
tools/arch/x86/include/asm/insn.h | 42 ++-
tools/arch/x86/lib/insn.c | 247 ++++++++++++++----
tools/include/linux/kconfig.h | 73 ++++++
tools/objtool/arch/x86/decode.c | 9 +-
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 +-
24 files changed, 595 insertions(+), 248 deletions(-)
create mode 100644 tools/include/linux/kconfig.h
--
2.29.2
From: Borislav Petkov <[email protected]>
Simplify code, no functional changes.
Signed-off-by: Borislav Petkov <[email protected]>
---
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
From: Borislav Petkov <[email protected]>
Simplify code, improve decoding error checking.
Signed-off-by: Borislav Petkov <[email protected]>
Acked-by: Masami Hiramatsu <[email protected]>
---
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
From: Borislav Petkov <[email protected]>
No functional changes, just simplification.
Signed-off-by: Borislav Petkov <[email protected]>
---
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
From: Borislav Petkov <[email protected]>
Simplify code, no functional changes.
Signed-off-by: Borislav Petkov <[email protected]>
---
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
From: Borislav Petkov <[email protected]>
Simplify code, no functional changes.
Signed-off-by: Borislav Petkov <[email protected]>
---
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
From: Borislav Petkov <[email protected]>
Now that it is not needed anymore, drop it.
Signed-off-by: Borislav Petkov <[email protected]>
---
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
From: Borislav Petkov <[email protected]>
Simplify code, no functional changes.
Signed-off-by: Borislav Petkov <[email protected]>
---
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
From: Borislav Petkov <[email protected]>
Other than simplifying the code there should be no functional changes
resulting from this.
Signed-off-by: Borislav Petkov <[email protected]>
---
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
From: Borislav Petkov <[email protected]>
It wasn't documented so add it. No functional changes.
Signed-off-by: Borislav Petkov <[email protected]>
---
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
From: Borislav Petkov <[email protected]>
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 <[email protected]>
---
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
From: Borislav Petkov <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
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/[email protected]
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 <[email protected]>
>
> 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
On 12/23/20 11:42 AM, Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
>
> 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
>
Hi,
On Wed, 23 Dec 2020 18:42:17 +0100
Borislav Petkov <[email protected]> wrote:
> From: Borislav Petkov <[email protected]>
>
> 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 <[email protected]>
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 <[email protected]>
> ---
> 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 <[email protected]>
On Wed, 23 Dec 2020 18:42:16 +0100
Borislav Petkov <[email protected]> wrote:
> From: Borislav Petkov <[email protected]>
>
> It wasn't documented so add it. No functional changes.
>
Thank you for fixing!
Acked-by: Masami Hiramatsu <[email protected]>
> Signed-off-by: Borislav Petkov <[email protected]>
> ---
> 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 <[email protected]>
On Wed, Dec 23, 2020, Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
>
> 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".
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 <[email protected]>
> >
> > 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;
}
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
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
On Tue, 29 Dec 2020 21:06:54 +0100
Borislav Petkov <[email protected]> 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 <[email protected]>
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
On Wed, 30 Dec 2020 10:28:33 +0100
Borislav Petkov <[email protected]> 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 <[email protected]>
*/
-#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 <[email protected]>
*/
-#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 <[email protected]>
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 <[email protected]>
*/
-#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 <[email protected]>
*/
-#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 <[email protected]>
*/
-#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 <[email protected]>
*/
-#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
On Fri, 8 Jan 2021 19:59:50 +0100
Borislav Petkov <[email protected]> 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 <[email protected]>
> */
> -#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 <[email protected]>
> */
> -#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 <[email protected]>
> */
> -#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 <[email protected]>
> */
> -#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 <[email protected]>
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 <[email protected]>
*/
-#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 <[email protected]>
*/
-#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 <[email protected]>
*/
-#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 <[email protected]>
*/
-#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
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
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?
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