[PATCH v3 0/3] x86/insn: Fix not using prefixes.nbytes for loop over prefixes.bytes

[PATCH v3 0/3] x86/insn: Fix not using prefixes.nbytes for loop over prefixes.bytes

[Masami Hiramatsu]

Hi,

Here are the 3rd version of patches to fix the wrong loop boundary
check on insn.prefixes.bytes[] array.

The previous version is here;

https://lkml.kernel.org/r/160697102582.3146288.10127018634865687932.stgit@devnote2

In this version, I've fixed some comments, added NUM_INSN_FIELD_BYTES
and MAX_LEGACY_PREFIX_GROUPS macros and commented on it.

Kees Cook got a syzbot warning and found this issue and there were 
similar wrong boundary check patterns in the x86 code.

Since the insn.prefixes.nbytes can be bigger than the size of
insn.prefixes.bytes[] when a same prefix is repeated, we have to
check whether the insn.prefixes.bytes[i] != 0 (*) and i < 4 instead
of insn.prefixes.nbytes.

(*) Note that insn.prefixes.bytes[] should be zeroed in insn_init()
before decoding, and 0x00 is not a legacy prefix. So if you see 0
on insn.prefix.bytes[], it indicates the end of the array. Or,
if the prefixes.bytes[] is filled with prefix bytes, we can check
the index is less than 4.

Thank you,

---

Masami Hiramatsu (3):
      x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes
      x86/insn-eval: Fix not using prefixes.nbytes for loop over prefixes.bytes
      x86/sev-es: Fix not using prefixes.nbytes for loop over prefixes.bytes


 arch/x86/boot/compressed/sev-es.c |    5 ++--
 arch/x86/include/asm/insn.h       |   49 ++++++++++++++++++++++++++++++++++++-
 arch/x86/kernel/uprobes.c         |   10 +++++---
 arch/x86/lib/insn-eval.c          |   10 ++++----
 arch/x86/lib/insn.c               |    2 +-
 tools/arch/x86/include/asm/insn.h |   49 ++++++++++++++++++++++++++++++++++++-
 tools/arch/x86/lib/insn.c         |    2 +-
 7 files changed, 111 insertions(+), 16 deletions(-)

-- 
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

[PATCH v3 1/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes

[Masami Hiramatsu]

Since the insn.prefixes.nbytes can be bigger than the size of
insn.prefixes.bytes[] when a same prefix is repeated, we have to
check whether the insn.prefixes.bytes[i] != 0 and i < 4(*) instead
of insn.prefixes.nbytes.
(*) 4 is the number of maximum legacy prefix groups on one instruction.
This introduces for_each_insn_prefix() macro for this purpose.

Fixes: 2b1444983508 ("uprobes, mm, x86: Add the ability to install and remove uprobes breakpoints")
Reported-by: syzbot+9b64b619f10f19d19a7c@syzkaller.appspotmail.com
Debugged-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: stable@vger.kernel.org
---
 Changes in v3:
  - Add NUM_INSN_FIELD_BYTES and MAX_LEGACY_PREFIX_GROUPS macros
    and comments on it.
 Changes in v2:
  - Add for_each_insn_prefix() macro and fix to check index first.
---
 arch/x86/include/asm/insn.h       |   49 ++++++++++++++++++++++++++++++++++++-
 arch/x86/kernel/uprobes.c         |   10 +++++---
 arch/x86/lib/insn.c               |    2 +-
 tools/arch/x86/include/asm/insn.h |   49 ++++++++++++++++++++++++++++++++++++-
 tools/arch/x86/lib/insn.c         |    2 +-
 5 files changed, 104 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
index 5c1ae3eff9d4..516d9d03027c 100644
--- a/arch/x86/include/asm/insn.h
+++ b/arch/x86/include/asm/insn.h
@@ -10,10 +10,13 @@
 /* insn_attr_t is defined in inat.h */
 #include <asm/inat.h>
 
+/* This must be sizeof(insn_value_t) / sizeof(insn_byte_t) */
+#define NUM_INSN_FIELD_BYTES	4
+
 struct insn_field {
 	union {
 		insn_value_t value;
-		insn_byte_t bytes[4];
+		insn_byte_t bytes[NUM_INSN_FIELD_BYTES];
 	};
 	/* !0 if we've run insn_get_xxx() for this field */
 	unsigned char got;
@@ -58,6 +61,32 @@ struct insn {
 };
 
 #define MAX_INSN_SIZE	15
+/*
+ * Intel SDM Vol.2 2.1.1 states,
+ * Instruction prefixes are divided into four groups, each with a set of
+ * allowable prefix codes. For each instruction, it only useful to include
+ * up to one prefix code from each of the four groups (Groups 1, 2, 3, 4).
+ *
+ * Also, AMD APM Vol.3 1.2.1 states,
+ * The legacy prefixes are organized into five groups, ... An instruction
+ * encoding may include a maximum of one prefix from each of the five groups.
+ *
+ * However, AMD APM classifies REP* and LOCK prefix in different groups,
+ * but those prefixes are supported by different instructions. Thus,
+ * there is no chance to co-exist them on the same instruction.
+ *
+ * So it can be expected that there is 4 groups of legacy prefixes in
+ * maximum on one instruction.
+ */
+#define MAX_LEGACY_PREFIX_GROUPS	4
+
+/*
+ * Make sure the number of max legacy prefix groups does not exceed the
+ * size of insn.prefixes.bytes[].
+ */
+#if MAX_LEGACY_PREFIX_GROUPS > NUM_INSN_FIELD_BYTES
+ #error "Error: Max number of prefix groups exceeds the prefix field size"
+#endif
 
 #define X86_MODRM_MOD(modrm) (((modrm) & 0xc0) >> 6)
 #define X86_MODRM_REG(modrm) (((modrm) & 0x38) >> 3)
@@ -201,6 +230,24 @@ static inline int insn_offset_immediate(struct insn *insn)
 	return insn_offset_displacement(insn) + insn->displacement.nbytes;
 }
 
+/**
+ * for_each_insn_prefix() -- Iterate prefixes in the instruction
+ * @insn: Pointer to struct insn.
+ * @idx:  Index storage.
+ * @prefix: Prefix byte.
+ *
+ * Iterate prefix bytes of given @insn. Each prefix byte is stored in @prefix
+ * and the index is stored in @idx (note that this @idx is just for a cursor,
+ * do not change it.)
+ * Since prefixes.nbytes can be bigger than NUM_INSN_FIELD_BYTES when some
+ * prefixes are repeated, it can not be used for looping over the prefixes.
+ */
+#define for_each_insn_prefix(insn, idx, prefix)				\
+	for (idx = 0;							\
+	     idx < MAX_LEGACY_PREFIX_GROUPS &&				\
+	     (prefix = insn->prefixes.bytes[idx]) != 0;			\
+	     idx++)
+
 #define POP_SS_OPCODE 0x1f
 #define MOV_SREG_OPCODE 0x8e
 
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 3fdaa042823d..138bdb1fd136 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -255,12 +255,13 @@ static volatile u32 good_2byte_insns[256 / 32] = {
 
 static bool is_prefix_bad(struct insn *insn)
 {
+	insn_byte_t p;
 	int i;
 
-	for (i = 0; i < insn->prefixes.nbytes; i++) {
+	for_each_insn_prefix(insn, i, p) {
 		insn_attr_t attr;
 
-		attr = inat_get_opcode_attribute(insn->prefixes.bytes[i]);
+		attr = inat_get_opcode_attribute(p);
 		switch (attr) {
 		case INAT_MAKE_PREFIX(INAT_PFX_ES):
 		case INAT_MAKE_PREFIX(INAT_PFX_CS):
@@ -715,6 +716,7 @@ static const struct uprobe_xol_ops push_xol_ops = {
 static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
 {
 	u8 opc1 = OPCODE1(insn);
+	insn_byte_t p;
 	int i;
 
 	switch (opc1) {
@@ -746,8 +748,8 @@ static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
 	 * Intel and AMD behavior differ in 64-bit mode: Intel ignores 66 prefix.
 	 * No one uses these insns, reject any branch insns with such prefix.
 	 */
-	for (i = 0; i < insn->prefixes.nbytes; i++) {
-		if (insn->prefixes.bytes[i] == 0x66)
+	for_each_insn_prefix(insn, i, p) {
+		if (p == 0x66)
 			return -ENOTSUPP;
 	}
 
diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
index 404279563891..c68c2a0c82d8 100644
--- a/arch/x86/lib/insn.c
+++ b/arch/x86/lib/insn.c
@@ -119,7 +119,7 @@ void insn_get_prefixes(struct insn *insn)
 		for (i = 0; i < nb; i++)
 			if (prefixes->bytes[i] == b)
 				goto found;
-		if (nb == 4)
+		if (nb == MAX_LEGACY_PREFIX_GROUPS)
 			/* Invalid instruction */
 			break;
 		prefixes->bytes[nb++] = b;
diff --git a/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
index 568854b14d0a..c968c8f30915 100644
--- a/tools/arch/x86/include/asm/insn.h
+++ b/tools/arch/x86/include/asm/insn.h
@@ -10,10 +10,13 @@
 /* insn_attr_t is defined in inat.h */
 #include "inat.h"
 
+/* This must be sizeof(insn_value_t) / sizeof(insn_byte_t) */
+#define NUM_INSN_FIELD_BYTES	4
+
 struct insn_field {
 	union {
 		insn_value_t value;
-		insn_byte_t bytes[4];
+		insn_byte_t bytes[NUM_INSN_FIELD_BYTES];
 	};
 	/* !0 if we've run insn_get_xxx() for this field */
 	unsigned char got;
@@ -58,6 +61,32 @@ struct insn {
 };
 
 #define MAX_INSN_SIZE	15
+/*
+ * Intel SDM Vol.2 2.1.1 states,
+ * Instruction prefixes are divided into four groups, each with a set of
+ * allowable prefix codes. For each instruction, it only useful to include
+ * up to one prefix code from each of the four groups (Groups 1, 2, 3, 4).
+ *
+ * Also, AMD APM Vol.3 1.2.1 states,
+ * The legacy prefixes are organized into five groups, ... An instruction
+ * encoding may include a maximum of one prefix from each of the five groups.
+ *
+ * However, AMD APM classifies REP* and LOCK prefix in different groups,
+ * but those prefixes are supported by different instructions. Thus,
+ * there is no chance to co-exist them on the same instruction.
+ *
+ * So it can be expected that there is 4 groups of legacy prefixes in
+ * maximum on one instruction.
+ */
+#define MAX_LEGACY_PREFIX_GROUPS	4
+
+/*
+ * Make sure the number of max legacy prefix groups does not exceed the
+ * size of insn.prefixes.bytes[].
+ */
+#if MAX_LEGACY_PREFIX_GROUPS > NUM_INSN_FIELD_BYTES
+ #error "Error: Max number of prefix groups exceeds the prefix field size"
+#endif
 
 #define X86_MODRM_MOD(modrm) (((modrm) & 0xc0) >> 6)
 #define X86_MODRM_REG(modrm) (((modrm) & 0x38) >> 3)
@@ -201,6 +230,24 @@ static inline int insn_offset_immediate(struct insn *insn)
 	return insn_offset_displacement(insn) + insn->displacement.nbytes;
 }
 
+/**
+ * for_each_insn_prefix() -- Iterate prefixes in the instruction
+ * @insn: Pointer to struct insn.
+ * @idx:  Index storage.
+ * @prefix: Prefix byte.
+ *
+ * Iterate prefix bytes of given @insn. Each prefix byte is stored in @prefix
+ * and the index is stored in @idx (note that this @idx is just for a cursor,
+ * do not change it.)
+ * Since prefixes.nbytes can be bigger than NUM_INSN_FIELD_BYTES when some
+ * prefixes are repeated, it can not be used for looping over the prefixes.
+ */
+#define for_each_insn_prefix(insn, idx, prefix)				\
+	for (idx = 0;							\
+	     idx < MAX_LEGACY_PREFIX_GROUPS &&				\
+	     (prefix = insn->prefixes.bytes[idx]) != 0;			\
+	     idx++)
+
 #define POP_SS_OPCODE 0x1f
 #define MOV_SREG_OPCODE 0x8e
 
diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c
index 0151dfc6da61..2f2def4f2ee7 100644
--- a/tools/arch/x86/lib/insn.c
+++ b/tools/arch/x86/lib/insn.c
@@ -119,7 +119,7 @@ void insn_get_prefixes(struct insn *insn)
 		for (i = 0; i < nb; i++)
 			if (prefixes->bytes[i] == b)
 				goto found;
-		if (nb == 4)
+		if (nb == MAX_LEGACY_PREFIX_GROUPS)
 			/* Invalid instruction */
 			break;
 		prefixes->bytes[nb++] = b;

Re: [PATCH v3 1/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes

[Borislav Petkov]

On Fri, Dec 04, 2020 at 07:55:20PM +0900, Masami Hiramatsu wrote:
> +/**
> + * for_each_insn_prefix() -- Iterate prefixes in the instruction
> + * @insn: Pointer to struct insn.
> + * @idx:  Index storage.
> + * @prefix: Prefix byte.
> + *
> + * Iterate prefix bytes of given @insn. Each prefix byte is stored in @prefix
> + * and the index is stored in @idx (note that this @idx is just for a cursor,
> + * do not change it.)
> + * Since prefixes.nbytes can be bigger than NUM_INSN_FIELD_BYTES when some
> + * prefixes are repeated, it can not be used for looping over the prefixes.
> + */
> +#define for_each_insn_prefix(insn, idx, prefix)				\
> +	for (idx = 0;							\
> +	     idx < MAX_LEGACY_PREFIX_GROUPS &&				\

The problem I see here is that you check for the index limit to be
< MAX_LEGACY_PREFIX_GROUPS but the array itself is defined using
NUM_INSN_FIELD_BYTES, and that is confusing.

I guess this should be:

#define MAX_LEGACY_PREFIX_GROUPS	4
#define NUM_INSN_FIELD_BYTES		MAX_LEGACY_PREFIX_GROUPS

and later, iff the legacy prefixes array size needs separating from the
insn field array size, then the defines would need to change too.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v3 1/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes

[Masami Hiramatsu]

On Fri, 4 Dec 2020 16:05:22 +0100
Borislav Petkov <bp@alien8.de> wrote:

> On Fri, Dec 04, 2020 at 07:55:20PM +0900, Masami Hiramatsu wrote:
> > +/**
> > + * for_each_insn_prefix() -- Iterate prefixes in the instruction
> > + * @insn: Pointer to struct insn.
> > + * @idx:  Index storage.
> > + * @prefix: Prefix byte.
> > + *
> > + * Iterate prefix bytes of given @insn. Each prefix byte is stored in @prefix
> > + * and the index is stored in @idx (note that this @idx is just for a cursor,
> > + * do not change it.)
> > + * Since prefixes.nbytes can be bigger than NUM_INSN_FIELD_BYTES when some
> > + * prefixes are repeated, it can not be used for looping over the prefixes.
> > + */
> > +#define for_each_insn_prefix(insn, idx, prefix)				\
> > +	for (idx = 0;							\
> > +	     idx < MAX_LEGACY_PREFIX_GROUPS &&				\
> 
> The problem I see here is that you check for the index limit to be
> < MAX_LEGACY_PREFIX_GROUPS but the array itself is defined using
> NUM_INSN_FIELD_BYTES, and that is confusing.

Yeah, I considered that once. If I know the number of legacy prefix
groups never exceed the size of prefixes.bytes, then we would better
use the max number of legacy prefix here (because we are looping on
the bytes from the prefix groups).
That is why I added #error check in this patch.

> I guess this should be:
> 
> #define MAX_LEGACY_PREFIX_GROUPS	4
> #define NUM_INSN_FIELD_BYTES		MAX_LEGACY_PREFIX_GROUPS
> 
> and later, iff the legacy prefixes array size needs separating from the
> insn field array size, then the defines would need to change too.

No, those have different meaning. NUM_INSN_FIELD_BYTES means
sizeof(s32) / sizeof(u8), which comes from the definition of
insn_field data structure. But MAX_LEGACY_PREFIX_GROUPS comes
from the x86 ISA.

In the future, if x86 ISA is expanded and add a legacy prefix
groups, then we have to add new insn_prefix_field data structure,
which size will not depend on NUM_INSN_FIELD_BYTES, but still
depend on MAX_LEGACY_PREFIX_GROUPS (and that will be 5).

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v3 1/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes

[Borislav Petkov]

On Sat, Dec 05, 2020 at 09:10:32AM +0900, Masami Hiramatsu wrote:
> In the future, if x86 ISA is expanded and add a legacy prefix
> groups,

Very unlikely.

> then we have to add new insn_prefix_field data structure, which
> size will not depend on NUM_INSN_FIELD_BYTES, but still depend on
> MAX_LEGACY_PREFIX_GROUPS (and that will be 5).

Isn't that what I'm saying too?

Bottomline is, legacy prefixes should not use insn_field but a separate
element which array size is independent of insn_byte_t bytes[4].

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[PATCH v3 2/3] x86/insn-eval: Fix not using prefixes.nbytes for loop over prefixes.bytes

[Masami Hiramatsu]

Since the insn.prefixes.nbytes can be bigger than the size of
insn.prefixes.bytes[] when a same prefix is repeated, we have to
check whether the insn.prefixes.bytes[i] != 0 and i < 4 instead
of insn.prefixes.nbytes.

Fixes: 32d0b95300db ("x86/insn-eval: Add utility functions to get segment selector")
Reported-by: syzbot+9b64b619f10f19d19a7c@syzkaller.appspotmail.com
Debugged-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: stable@vger.kernel.org
---
 arch/x86/lib/insn-eval.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
index 58f7fb95c7f4..4229950a5d78 100644
--- a/arch/x86/lib/insn-eval.c
+++ b/arch/x86/lib/insn-eval.c
@@ -63,13 +63,12 @@ static bool is_string_insn(struct insn *insn)
  */
 bool insn_has_rep_prefix(struct insn *insn)
 {
+	insn_byte_t p;
 	int i;
 
 	insn_get_prefixes(insn);
 
-	for (i = 0; i < insn->prefixes.nbytes; i++) {
-		insn_byte_t p = insn->prefixes.bytes[i];
-
+	for_each_insn_prefix(insn, i, p) {
 		if (p == 0xf2 || p == 0xf3)
 			return true;
 	}
@@ -95,14 +94,15 @@ static int get_seg_reg_override_idx(struct insn *insn)
 {
 	int idx = INAT_SEG_REG_DEFAULT;
 	int num_overrides = 0, i;
+	insn_byte_t p;
 
 	insn_get_prefixes(insn);
 
 	/* Look for any segment override prefixes. */
-	for (i = 0; i < insn->prefixes.nbytes; i++) {
+	for_each_insn_prefix(insn, i, p) {
 		insn_attr_t attr;
 
-		attr = inat_get_opcode_attribute(insn->prefixes.bytes[i]);
+		attr = inat_get_opcode_attribute(p);
 		switch (attr) {
 		case INAT_MAKE_PREFIX(INAT_PFX_CS):
 			idx = INAT_SEG_REG_CS;

[PATCH v3 3/3] x86/sev-es: Fix not using prefixes.nbytes for loop over prefixes.bytes

[Masami Hiramatsu]

Since the insn.prefixes.nbytes can be bigger than the size of
insn.prefixes.bytes[] when a same prefix is repeated, we have to
check whether the i < 4 and insn.prefixes.bytes[i] != 0 instead
of insn.prefixes.nbytes.

Fixes: 25189d08e516 ("x86/sev-es: Add support for handling IOIO exceptions")
Reported-by: syzbot+9b64b619f10f19d19a7c@syzkaller.appspotmail.com
Debugged-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/boot/compressed/sev-es.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/x86/boot/compressed/sev-es.c b/arch/x86/boot/compressed/sev-es.c
index 954cb2702e23..27826c265aab 100644
--- a/arch/x86/boot/compressed/sev-es.c
+++ b/arch/x86/boot/compressed/sev-es.c
@@ -32,13 +32,12 @@ struct ghcb *boot_ghcb;
  */
 static bool insn_has_rep_prefix(struct insn *insn)
 {
+	insn_byte_t p;
 	int i;
 
 	insn_get_prefixes(insn);
 
-	for (i = 0; i < insn->prefixes.nbytes; i++) {
-		insn_byte_t p = insn->prefixes.bytes[i];
-
+	for_each_insn_prefix(insn, i, p) {
 		if (p == 0xf2 || p == 0xf3)
 			return true;
 	}

Re: [PATCH v3 0/3] x86/insn: Fix not using prefixes.nbytes for loop over prefixes.bytes

[Borislav Petkov]

On Fri, Dec 04, 2020 at 07:55:09PM +0900, Masami Hiramatsu wrote:
> Hi,
> 
> Here are the 3rd version of patches to fix the wrong loop boundary
> check on insn.prefixes.bytes[] array.

Ok, so I've committed the version with ARRAY_SIZE to keep it as small
as possible for stable. Let's discuss the new changes here ontop, once
those urgent fixes go up.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v3 0/3] x86/insn: Fix not using prefixes.nbytes for loop over prefixes.bytes

[Masami Hiramatsu]

On Fri, 4 Dec 2020 16:02:21 +0100
Borislav Petkov <bp@alien8.de> wrote:

> On Fri, Dec 04, 2020 at 07:55:09PM +0900, Masami Hiramatsu wrote:
> > Hi,
> > 
> > Here are the 3rd version of patches to fix the wrong loop boundary
> > check on insn.prefixes.bytes[] array.
> 
> Ok, so I've committed the version with ARRAY_SIZE to keep it as small
> as possible for stable. Let's discuss the new changes here ontop, once
> those urgent fixes go up.

Agreed. BTW, please fix to keep the #include "inat.h" for tools build.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>