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

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

[Masami Hiramatsu]

Hi,

Here are the 2nd 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/160689905099.3084105.7880450206184269465.stgit@devnote2

In this version, I introduced for_each_insn_prefix() macro to
for looping on the prefixes in the given instruction and fixed
out-of-bounds-read issue by checking index first. Also, I sorted 
the patches so that the oldest commit fix becomes the first patch
because it will go into the older stable kernel and that introduces
the new iteration macro.

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       |   15 +++++++++++++++
 arch/x86/kernel/uprobes.c         |   10 ++++++----
 arch/x86/lib/insn-eval.c          |   10 +++++-----
 4 files changed, 28 insertions(+), 12 deletions(-)

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

[PATCH v2 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.
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 v2:
  - Add for_each_insn_prefix() macro and fix to check index first.
---
 arch/x86/include/asm/insn.h |   15 +++++++++++++++
 arch/x86/kernel/uprobes.c   |   10 ++++++----
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
index 5c1ae3eff9d4..e6b38ebd3a1c 100644
--- a/arch/x86/include/asm/insn.h
+++ b/arch/x86/include/asm/insn.h
@@ -201,6 +201,21 @@ 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 4 if some prefixes are repeated,
+ * we can not use it for looping over the prefixes.
+ */
+#define for_each_insn_prefix(insn, idx, prefix)	\
+	for (idx = 0; idx < 4 && (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;
 	}
 

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

[Borislav Petkov]

On Thu, Dec 03, 2020 at 01:50:37PM +0900, Masami Hiramatsu wrote:
> 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.
> 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 v2:
>   - Add for_each_insn_prefix() macro and fix to check index first.
> ---
>  arch/x86/include/asm/insn.h |   15 +++++++++++++++
>  arch/x86/kernel/uprobes.c   |   10 ++++++----
>  2 files changed, 21 insertions(+), 4 deletions(-)

Warning: Kernel ABI header at 'tools/arch/x86/include/asm/insn.h' differs from latest version at 'arch/x86/include/asm/insn.h'

> diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
> index 5c1ae3eff9d4..e6b38ebd3a1c 100644
> --- a/arch/x86/include/asm/insn.h
> +++ b/arch/x86/include/asm/insn.h
> @@ -201,6 +201,21 @@ 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 4 if some prefixes are repeated,
> + * we can not use it for looping over the prefixes.

Please use passive voice: no "we" or "I", etc,

> + */
> +#define for_each_insn_prefix(insn, idx, prefix)	\
> +	for (idx = 0; idx < 4 && (prefix = insn->prefixes.bytes[idx]) != 0; idx++)

Btw, looking at the struct insn definition, that prefixes member should
have a comment above it that those are the legacy prefixes which can be
<= 4. But that's minor.

In any case, I'll fix up the minor issues now but pls remember to do
them in the future.

Thx.

-- 
Regards/Gruss,
    Boris.

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

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

[Borislav Petkov]

On Thu, Dec 03, 2020 at 01:37:57PM +0100, Borislav Petkov wrote:
> Btw, looking at the struct insn definition, that prefixes member should
> have a comment above it that those are the legacy prefixes which can be
> <= 4. But that's minor.

And that naked 4 is poking my eye too - It'd be better if it were
NUM_LEGACY_PREFIXES.

-- 
Regards/Gruss,
    Boris.

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

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

[Borislav Petkov]

So it ended up like this:

---
From 5014e4e902778d63ce392f864b3654baa4b72384 Mon Sep 17 00:00:00 2001
From: Masami Hiramatsu <mhiramat@kernel.org>
Date: Thu, 3 Dec 2020 13:50:37 +0900
Subject: [PATCH] x86/uprobes: Do not use prefixes.nbytes when looping over
 prefixes.bytes

Since insn.prefixes.nbytes can be bigger than the size of
insn.prefixes.bytes[] when a prefix is repeated, the proper check must
be

  insn.prefixes.bytes[i] != 0 and i < 4

instead of using insn.prefixes.nbytes.

Introduce a for_each_insn_prefix() macro for this purpose. Debugged by
Kees Cook <keescook@chromium.org>.

 [ bp: Massage commit message, add NUM_LEGACY_PREFIXES, sync with the
   respective header in tools/ and drop "we". ]

Fixes: 2b1444983508 ("uprobes, mm, x86: Add the ability to install and remove uprobes breakpoints")
Reported-by: syzbot+9b64b619f10f19d19a7c@syzkaller.appspotmail.com
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/160697103739.3146288.7437620795200799020.stgit@devnote2
---
 arch/x86/include/asm/insn.h       | 16 ++++++++++++++++
 arch/x86/kernel/uprobes.c         | 10 ++++++----
 tools/arch/x86/include/asm/insn.h | 18 +++++++++++++++++-
 3 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
index 5c1ae3eff9d4..fe8e862004d3 100644
--- a/arch/x86/include/asm/insn.h
+++ b/arch/x86/include/asm/insn.h
@@ -58,6 +58,7 @@ struct insn {
 };
 
 #define MAX_INSN_SIZE	15
+#define NUM_LEGACY_PREFIXES 4
 
 #define X86_MODRM_MOD(modrm) (((modrm) & 0xc0) >> 6)
 #define X86_MODRM_REG(modrm) (((modrm) & 0x38) >> 3)
@@ -201,6 +202,21 @@ 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_LEGACY_PREFIXES if some prefixes
+ * are repeated, it cannot be used for looping over the prefixes.
+ */
+#define for_each_insn_prefix(insn, idx, prefix)	\
+	for (idx = 0; idx < NUM_LEGACY_PREFIXES && (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/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
index 568854b14d0a..fe8e862004d3 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 <asm/inat.h>
 
 struct insn_field {
 	union {
@@ -58,6 +58,7 @@ struct insn {
 };
 
 #define MAX_INSN_SIZE	15
+#define NUM_LEGACY_PREFIXES 4
 
 #define X86_MODRM_MOD(modrm) (((modrm) & 0xc0) >> 6)
 #define X86_MODRM_REG(modrm) (((modrm) & 0x38) >> 3)
@@ -201,6 +202,21 @@ 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_LEGACY_PREFIXES if some prefixes
+ * are repeated, it cannot be used for looping over the prefixes.
+ */
+#define for_each_insn_prefix(insn, idx, prefix)	\
+	for (idx = 0; idx < NUM_LEGACY_PREFIXES && (prefix = insn->prefixes.bytes[idx]) != 0; idx++)
+
 #define POP_SS_OPCODE 0x1f
 #define MOV_SREG_OPCODE 0x8e
 
-- 
2.21.0


-- 
Regards/Gruss,
    Boris.

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

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

[Tom Lendacky]

On 12/3/20 6:48 AM, Borislav Petkov wrote:
> So it ended up like this:
> 
> ---
>  From 5014e4e902778d63ce392f864b3654baa4b72384 Mon Sep 17 00:00:00 2001
> From: Masami Hiramatsu <mhiramat@kernel.org>
> Date: Thu, 3 Dec 2020 13:50:37 +0900
> Subject: [PATCH] x86/uprobes: Do not use prefixes.nbytes when looping over
>   prefixes.bytes
> 
> Since insn.prefixes.nbytes can be bigger than the size of
> insn.prefixes.bytes[] when a prefix is repeated, the proper check must
> be
> 
>    insn.prefixes.bytes[i] != 0 and i < 4
> 
> instead of using insn.prefixes.nbytes.
> 
> Introduce a for_each_insn_prefix() macro for this purpose. Debugged by
> Kees Cook <keescook@chromium.org>.
> 
>   [ bp: Massage commit message, add NUM_LEGACY_PREFIXES, sync with the
>     respective header in tools/ and drop "we". ]
> 
> Fixes: 2b1444983508 ("uprobes, mm, x86: Add the ability to install and remove uprobes breakpoints")
> Reported-by: syzbot+9b64b619f10f19d19a7c@syzkaller.appspotmail.com
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Cc: stable@vger.kernel.org
> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.kernel.org%2Fr%2F160697103739.3146288.7437620795200799020.stgit%40devnote2&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7Ce8ec706c564245542b4408d89789b727%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637425965056484231%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=csaC0C2cszr45mKES42CHeZjC4TnEJtKrMa0gSmHjn8%3D&amp;reserved=0
> ---
>   arch/x86/include/asm/insn.h       | 16 ++++++++++++++++
>   arch/x86/kernel/uprobes.c         | 10 ++++++----
>   tools/arch/x86/include/asm/insn.h | 18 +++++++++++++++++-
>   3 files changed, 39 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
> index 5c1ae3eff9d4..fe8e862004d3 100644
> --- a/arch/x86/include/asm/insn.h
> +++ b/arch/x86/include/asm/insn.h
> @@ -58,6 +58,7 @@ struct insn {
>   };
>   
>   #define MAX_INSN_SIZE	15
> +#define NUM_LEGACY_PREFIXES 4
>   
>   #define X86_MODRM_MOD(modrm) (((modrm) & 0xc0) >> 6)
>   #define X86_MODRM_REG(modrm) (((modrm) & 0x38) >> 3)
> @@ -201,6 +202,21 @@ 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_LEGACY_PREFIXES if some prefixes
> + * are repeated, it cannot be used for looping over the prefixes.
> + */
> +#define for_each_insn_prefix(insn, idx, prefix)	\
> +	for (idx = 0; idx < NUM_LEGACY_PREFIXES && (prefix = insn->prefixes.bytes[idx]) != 0; idx++)

Since this is based on the array size, can

	idx < NUM_LEGACY_PREFIXES

be replaced with:

	idx < ARRAY_SIZE(insn->prefixes.bytes)

?

Thanks,
Tom

> +
>   #define POP_SS_OPCODE 0x1f
>   #define MOV_SREG_OPCODE 0x8e
>   

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

[Borislav Petkov]

On Thu, Dec 03, 2020 at 10:45:48AM -0600, Tom Lendacky wrote:
> Since this is based on the array size, can
> 
> 	idx < NUM_LEGACY_PREFIXES
> 
> be replaced with:
> 
> 	idx < ARRAY_SIZE(insn->prefixes.bytes)

Actually, this needs another change:

struct insn_field {
        union {
                insn_value_t value;
                insn_byte_t bytes[NUM_LEGACY_PREFIXES];

because you can have max. 4 legacy prefixes and then we can do either of
the checks above.

Mine is shorter tho. :-)

-- 
Regards/Gruss,
    Boris.

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

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

[Borislav Petkov]

On Thu, Dec 03, 2020 at 05:54:20PM +0100, Borislav Petkov wrote:
> On Thu, Dec 03, 2020 at 10:45:48AM -0600, Tom Lendacky wrote:
> > Since this is based on the array size, can
> > 
> > 	idx < NUM_LEGACY_PREFIXES
> > 
> > be replaced with:
> > 
> > 	idx < ARRAY_SIZE(insn->prefixes.bytes)
> 
> Actually, this needs another change:
> 
> struct insn_field {
>         union {
>                 insn_value_t value;
>                 insn_byte_t bytes[NUM_LEGACY_PREFIXES];

Blergh, spoke too soon. All those struct insn members are struct
insn_field.

insn.prefixes should probably be a separate array of explicit size
NUM_LEGACY_PREFIXES, not that insn_byte_t bytes[] gets enlarged in the
future for whatever reason, while the max legacy prefixes count will
remain 4.

-- 
Regards/Gruss,
    Boris.

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

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

[Tom Lendacky]

On 12/3/20 11:01 AM, Borislav Petkov wrote:
> On Thu, Dec 03, 2020 at 05:54:20PM +0100, Borislav Petkov wrote:
>> On Thu, Dec 03, 2020 at 10:45:48AM -0600, Tom Lendacky wrote:
>>> Since this is based on the array size, can
>>>
>>> 	idx < NUM_LEGACY_PREFIXES
>>>
>>> be replaced with:
>>>
>>> 	idx < ARRAY_SIZE(insn->prefixes.bytes)
>>
>> Actually, this needs another change:
>>
>> struct insn_field {
>>          union {
>>                  insn_value_t value;
>>                  insn_byte_t bytes[NUM_LEGACY_PREFIXES];
> 
> Blergh, spoke too soon. All those struct insn members are struct
> insn_field.
> 
> insn.prefixes should probably be a separate array of explicit size
> NUM_LEGACY_PREFIXES, not that insn_byte_t bytes[] gets enlarged in the
> future for whatever reason, while the max legacy prefixes count will
> remain 4.

Since that struct is used in multiple places, I think basing it on the 
array size is the best way to go. The main point of the check is just to 
be sure you don't read outside of the array.

Thanks,
Tom

> 

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

[Borislav Petkov]

On Thu, Dec 03, 2020 at 12:10:10PM -0600, Tom Lendacky wrote:
> Since that struct is used in multiple places, I think basing it on the array
> size is the best way to go. The main point of the check is just to be sure
> you don't read outside of the array.

Well, what happens if someone increases the array size of:

struct insn_field {
	union {
		insn_byte_t bytes[4];
				^^^^

?

That's why a separate array only for legacy prefixes would be better
in the long run. The array size check is good as a short-term fix for
stable.

I'd say.

-- 
Regards/Gruss,
    Boris.

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

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

[Tom Lendacky]

On 12/3/20 12:17 PM, Borislav Petkov wrote:
> On Thu, Dec 03, 2020 at 12:10:10PM -0600, Tom Lendacky wrote:
>> Since that struct is used in multiple places, I think basing it on the array
>> size is the best way to go. The main point of the check is just to be sure
>> you don't read outside of the array.
> 
> Well, what happens if someone increases the array size of:
> 
> struct insn_field {
> 	union {
> 		insn_byte_t bytes[4];
> 				^^^^
> 
> ?

I think we need to keep the parsing of the instruction separate from 
accessing the prefixes after (successfully) parsing it. This fix is merely 
making sure that we don't read outside the bounds of the array that 
currently holds the legacy prefixes.

> 
> That's why a separate array only for legacy prefixes would be better
> in the long run. The array size check is good as a short-term fix for
> stable.
> 
> I'd say.

According to Volume 3 of the AMD APM (Figure 1-2 on page 5), there could 
be as many as 5 legacy prefixes and it says that more than one prefix from 
each group is undefined behavior. The instruction parsing code doesn't 
seem to take into account the different prefix groups. So I agree with you 
that short term the array size check works, and long term, the legacy 
prefix support probably needs a closer look.

Thanks,
Tom

> 

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

[Masami Hiramatsu]

On Thu, 3 Dec 2020 12:49:46 -0600
Tom Lendacky <thomas.lendacky@amd.com> wrote:

> On 12/3/20 12:17 PM, Borislav Petkov wrote:
> > On Thu, Dec 03, 2020 at 12:10:10PM -0600, Tom Lendacky wrote:
> >> Since that struct is used in multiple places, I think basing it on the array
> >> size is the best way to go. The main point of the check is just to be sure
> >> you don't read outside of the array.
> > 
> > Well, what happens if someone increases the array size of:
> > 
> > struct insn_field {
> > 	union {
> > 		insn_byte_t bytes[4];
> > 				^^^^
> > 
> > ?
> 
> I think we need to keep the parsing of the instruction separate from 
> accessing the prefixes after (successfully) parsing it. This fix is merely 
> making sure that we don't read outside the bounds of the array that 
> currently holds the legacy prefixes.
> 
> > 
> > That's why a separate array only for legacy prefixes would be better
> > in the long run. The array size check is good as a short-term fix for
> > stable.
> > 
> > I'd say.
> 
> According to Volume 3 of the AMD APM (Figure 1-2 on page 5), there could 
> be as many as 5 legacy prefixes and it says that more than one prefix from 
> each group is undefined behavior. The instruction parsing code doesn't 
> seem to take into account the different prefix groups. So I agree with you 
> that short term the array size check works, and long term, the legacy 
> prefix support probably needs a closer look.

Hmm, there is a difference between Intel SDM and AMD APM.

Intel SDM vol.2

2.1.1 Instruction Prefixes
Instruction prefixes are divided into four groups, each with a set of allowable prefix codes. For each instruction, it
is only useful to include up to one prefix code from each of the four groups (Groups 1, 2, 3, 4).

AMD APM vol.3

1.2.1 Summary of Legacy Prefixes
Table 1-1 on page 7 shows the legacy prefixes. The legacy prefixes are organized into five groups, as
shown in the left-most column of Table 1-1. An instruction encoding may include a maximum of one
prefix from each of the five groups.

So, Intel CPU doesn't accept LOCK-REP because those are in a same prefix
group, but AMD says it is acceptable. Actually, insn.c only accepts the 
prefix up to 4, so if there is any instruction which has 5 prefixes,
it will fail to parse.

Note that anyway the same prefix can be repeated, we can see a good example
in K8_NOP*.

/* Opteron 64bit nops
   1: nop
   2: osp nop
   3: osp osp nop
   4: osp osp osp nop
*/

In this case, insn.c just store the 1 osp in the prefixes.bytes[], and
just increment prefixes.nbytes for the repeated prefixes.

Anyway, if there is LOCK-REP prefix combination, I have to introduce new
insn_field for legacy prefix.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

[Masami Hiramatsu]

On Fri, 4 Dec 2020 09:56:53 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Thu, 3 Dec 2020 12:49:46 -0600
> Tom Lendacky <thomas.lendacky@amd.com> wrote:
> 
> > On 12/3/20 12:17 PM, Borislav Petkov wrote:
> > > On Thu, Dec 03, 2020 at 12:10:10PM -0600, Tom Lendacky wrote:
> > >> Since that struct is used in multiple places, I think basing it on the array
> > >> size is the best way to go. The main point of the check is just to be sure
> > >> you don't read outside of the array.
> > > 
> > > Well, what happens if someone increases the array size of:
> > > 
> > > struct insn_field {
> > > 	union {
> > > 		insn_byte_t bytes[4];
> > > 				^^^^
> > > 
> > > ?
> > 
> > I think we need to keep the parsing of the instruction separate from 
> > accessing the prefixes after (successfully) parsing it. This fix is merely 
> > making sure that we don't read outside the bounds of the array that 
> > currently holds the legacy prefixes.
> > 
> > > 
> > > That's why a separate array only for legacy prefixes would be better
> > > in the long run. The array size check is good as a short-term fix for
> > > stable.
> > > 
> > > I'd say.
> > 
> > According to Volume 3 of the AMD APM (Figure 1-2 on page 5), there could 
> > be as many as 5 legacy prefixes and it says that more than one prefix from 
> > each group is undefined behavior. The instruction parsing code doesn't 
> > seem to take into account the different prefix groups. So I agree with you 
> > that short term the array size check works, and long term, the legacy 
> > prefix support probably needs a closer look.
> 
> Hmm, there is a difference between Intel SDM and AMD APM.
> 
> Intel SDM vol.2
> 
> 2.1.1 Instruction Prefixes
> Instruction prefixes are divided into four groups, each with a set of allowable prefix codes. For each instruction, it
> is only useful to include up to one prefix code from each of the four groups (Groups 1, 2, 3, 4).
> 
> AMD APM vol.3
> 
> 1.2.1 Summary of Legacy Prefixes
> Table 1-1 on page 7 shows the legacy prefixes. The legacy prefixes are organized into five groups, as
> shown in the left-most column of Table 1-1. An instruction encoding may include a maximum of one
> prefix from each of the five groups.
> 
> So, Intel CPU doesn't accept LOCK-REP because those are in a same prefix
> group, but AMD says it is acceptable. Actually, insn.c only accepts the 
> prefix up to 4, so if there is any instruction which has 5 prefixes,
> it will fail to parse.


OK, I got it. AMD APM's explanation is misleading. Intel SDM groups
the legacy prefixes in 1) Lock/Repeat/Bound 2) Segment override/branch hints,
3) Operand-size override 4) Address-size override. On the other hand,
AMD APM makes it 5 groups(See Table 1-1), 1) Operand-Size Override
2) Address-Size Override 3) Segment Override 4) Lock 5) Repeat.

So the difference is whether the Lock and Repeat is a group or not.

However, I found we must not see LOCK-REP prefix in the same instruction,
because the available instruction for LOCK and REP are different.

AMD APM vol.3
---
1.2.5 Lock Prefix
...
The LOCK prefix can only be used with forms of the following instructions that write a memory
operand: ADC, ADD, AND, BTC, BTR, BTS, CMPXCHG, CMPXCHG8B, CMPXCHG16B, DEC,
INC, NEG, NOT, OR, SBB, SUB, XADD, XCHG, and XOR. An invalid-opcode exception occurs if
the LOCK prefix is used with any other instruction.

1.2.6 Repeat Prefixes
The repeat prefixes cause repetition of certain instructions that load, store, move, input, or output
strings. The prefixes should only be used with such string instructions.
...
REP. ... The prefix can be used with the INS, LODS, MOVS, OUTS, and STOS instructions.
...
The REPE and REPZ prefixes can be used with the CMPS, CMPSB, CMPSD, CMPSW, SCAS,
SCASB, SCASD, and SCASW instructions.
...
The REPNE and REPNZ prefixes can be used with the CMPS, CMPSB, CMPSD, CMPSW, SCAS,
SCASB, SCASD, and SCASW instructions.
---

Thus, I think the current expectation -- legacy prefix will consist
of 4 different groups -- is good. No need to take care of LOCK-REP
combination.

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

[Borislav Petkov]

On Fri, Dec 04, 2020 at 09:56:53AM +0900, Masami Hiramatsu wrote:
> Hmm, there is a difference between Intel SDM and AMD APM.
> 
> Intel SDM vol.2
> 
> 2.1.1 Instruction Prefixes
> Instruction prefixes are divided into four groups, each with a set of allowable prefix codes. For each instruction, it
> is only useful to include up to one prefix code from each of the four groups (Groups 1, 2, 3, 4).
> 
> AMD APM vol.3
> 
> 1.2.1 Summary of Legacy Prefixes
> Table 1-1 on page 7 shows the legacy prefixes. The legacy prefixes are organized into five groups, as
> shown in the left-most column of Table 1-1. An instruction encoding may include a maximum of one
> prefix from each of the five groups.
> 
> So, Intel CPU doesn't accept LOCK-REP because those are in a same prefix
> group, but AMD says it is acceptable.

That would be a huge problem for code if both vendors would behave
differently wrt prefixes.

> Actually, insn.c only accepts the prefix up to 4, so if there is any
> instruction which has 5 prefixes, it will fail to parse.

Well, actually it looks more like a difference in how both vendors group
things:

AMD has 5 groups and Intel 4 by putting LOCK and REP together.

The most important aspect, however, is that you can have as many
prefixes as you want and there's no hardware limitation on the number -
I'm being told - just that you can overflow the instruction limit of 15
and then get a #GP for invalid insn. See here:

https://sandpile.org/x86/opc_enc.htm

note #1

with examples how you can overflow the 15 bytes limit even with a valid
insn.

> Note that anyway the same prefix can be repeated, we can see a good example
> in K8_NOP*.

Yap.

> In this case, insn.c just store the 1 osp in the prefixes.bytes[], and
> just increment prefixes.nbytes for the repeated prefixes.
> 
> Anyway, if there is LOCK-REP prefix combination, I have to introduce new
> insn_field for legacy prefix.

Well, the legacy prefixes field needs to be of 4 fields because REP and
LOCK really are two separate but mutually exclusive groups. Why?

They're used by a disjoint set of instructions, see the AMD doc for both
REP and LOCK prefixes.

Which means, you can either have a REP (exclusive or) LOCK but not both.

Which means, as a stable@ fix I can use Tom's ARRAY_SIZE() suggestion
and then later on we can make the legacy prefixes a separate struct.
Maybe even a struct with a bitfield:

struct legacy_prefixes {
        /* operand-size override: 0x66 */
        u8 os_over: 1,
        /* address-size override: 0x67 */
           as_over: 1,
        /*
         * segment override: 0x2e(CS), 0x3e(DS), 0x26(ES), 0x64(FS), 0x65(GS),
         * 0x36(SS)
         */
           s_over: 1,
        /* lock prefix: 0xf0 */
           lock:   1,
        /* repeat prefixes: 0xf2: REPNx, 0xf3: REPx */
           rep:    1,
	   __resv: 3;
};

or so which you can set to denote when you've seen the respective
prefixes.

But that we can discuss later.

-- 
Regards/Gruss,
    Boris.

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

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

[Masami Hiramatsu]

On Fri, 4 Dec 2020 12:06:44 +0100
Borislav Petkov <bp@alien8.de> wrote:

> On Fri, Dec 04, 2020 at 09:56:53AM +0900, Masami Hiramatsu wrote:
> > Hmm, there is a difference between Intel SDM and AMD APM.
> > 
> > Intel SDM vol.2
> > 
> > 2.1.1 Instruction Prefixes
> > Instruction prefixes are divided into four groups, each with a set of allowable prefix codes. For each instruction, it
> > is only useful to include up to one prefix code from each of the four groups (Groups 1, 2, 3, 4).
> > 
> > AMD APM vol.3
> > 
> > 1.2.1 Summary of Legacy Prefixes
> > Table 1-1 on page 7 shows the legacy prefixes. The legacy prefixes are organized into five groups, as
> > shown in the left-most column of Table 1-1. An instruction encoding may include a maximum of one
> > prefix from each of the five groups.
> > 
> > So, Intel CPU doesn't accept LOCK-REP because those are in a same prefix
> > group, but AMD says it is acceptable.
> 
> That would be a huge problem for code if both vendors would behave
> differently wrt prefixes.
> 
> > Actually, insn.c only accepts the prefix up to 4, so if there is any
> > instruction which has 5 prefixes, it will fail to parse.
> 
> Well, actually it looks more like a difference in how both vendors group
> things:
> 
> AMD has 5 groups and Intel 4 by putting LOCK and REP together.
> 
> The most important aspect, however, is that you can have as many
> prefixes as you want and there's no hardware limitation on the number -
> I'm being told - just that you can overflow the instruction limit of 15
> and then get a #GP for invalid insn. See here:
> 
> https://sandpile.org/x86/opc_enc.htm
> 
> note #1
> 
> with examples how you can overflow the 15 bytes limit even with a valid
> insn.
> 
> > Note that anyway the same prefix can be repeated, we can see a good example
> > in K8_NOP*.
> 
> Yap.
> 
> > In this case, insn.c just store the 1 osp in the prefixes.bytes[], and
> > just increment prefixes.nbytes for the repeated prefixes.
> > 
> > Anyway, if there is LOCK-REP prefix combination, I have to introduce new
> > insn_field for legacy prefix.
> 
> Well, the legacy prefixes field needs to be of 4 fields because REP and
> LOCK really are two separate but mutually exclusive groups. Why?
> 
> They're used by a disjoint set of instructions, see the AMD doc for both
> REP and LOCK prefixes.
> 
> Which means, you can either have a REP (exclusive or) LOCK but not both.

Yeah, I found that. So I think the "max number of legacy groups on one
instruction" is 4.

> Which means, as a stable@ fix I can use Tom's ARRAY_SIZE() suggestion
> and then later on we can make the legacy prefixes a separate struct.
> Maybe even a struct with a bitfield:

Sorry, but I don't think we need such optimization. It seems over-
optimized the code for me. Moreover, the last-prefix is meaningful
for switching the opcode, so we need to keep it.

Thank you,


> 
> struct legacy_prefixes {
>         /* operand-size override: 0x66 */
>         u8 os_over: 1,
>         /* address-size override: 0x67 */
>            as_over: 1,
>         /*
>          * segment override: 0x2e(CS), 0x3e(DS), 0x26(ES), 0x64(FS), 0x65(GS),
>          * 0x36(SS)
>          */
>            s_over: 1,
>         /* lock prefix: 0xf0 */
>            lock:   1,
>         /* repeat prefixes: 0xf2: REPNx, 0xf3: REPx */
>            rep:    1,
> 	   __resv: 3;
> };
> 
> or so which you can set to denote when you've seen the respective
> prefixes.
> 
> But that we can discuss later.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

[Masami Hiramatsu]

On Thu, 3 Dec 2020 10:45:48 -0600
Tom Lendacky <thomas.lendacky@amd.com> wrote:

> On 12/3/20 6:48 AM, Borislav Petkov wrote:
> > So it ended up like this:
> > 
> > ---
> >  From 5014e4e902778d63ce392f864b3654baa4b72384 Mon Sep 17 00:00:00 2001
> > From: Masami Hiramatsu <mhiramat@kernel.org>
> > Date: Thu, 3 Dec 2020 13:50:37 +0900
> > Subject: [PATCH] x86/uprobes: Do not use prefixes.nbytes when looping over
> >   prefixes.bytes
> > 
> > Since insn.prefixes.nbytes can be bigger than the size of
> > insn.prefixes.bytes[] when a prefix is repeated, the proper check must
> > be
> > 
> >    insn.prefixes.bytes[i] != 0 and i < 4
> > 
> > instead of using insn.prefixes.nbytes.
> > 
> > Introduce a for_each_insn_prefix() macro for this purpose. Debugged by
> > Kees Cook <keescook@chromium.org>.
> > 
> >   [ bp: Massage commit message, add NUM_LEGACY_PREFIXES, sync with the
> >     respective header in tools/ and drop "we". ]
> > 
> > Fixes: 2b1444983508 ("uprobes, mm, x86: Add the ability to install and remove uprobes breakpoints")
> > Reported-by: syzbot+9b64b619f10f19d19a7c@syzkaller.appspotmail.com
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > Signed-off-by: Borislav Petkov <bp@suse.de>
> > Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> > Cc: stable@vger.kernel.org
> > Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.kernel.org%2Fr%2F160697103739.3146288.7437620795200799020.stgit%40devnote2&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7Ce8ec706c564245542b4408d89789b727%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637425965056484231%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=csaC0C2cszr45mKES42CHeZjC4TnEJtKrMa0gSmHjn8%3D&amp;reserved=0
> > ---
> >   arch/x86/include/asm/insn.h       | 16 ++++++++++++++++
> >   arch/x86/kernel/uprobes.c         | 10 ++++++----
> >   tools/arch/x86/include/asm/insn.h | 18 +++++++++++++++++-
> >   3 files changed, 39 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
> > index 5c1ae3eff9d4..fe8e862004d3 100644
> > --- a/arch/x86/include/asm/insn.h
> > +++ b/arch/x86/include/asm/insn.h
> > @@ -58,6 +58,7 @@ struct insn {
> >   };
> >   
> >   #define MAX_INSN_SIZE	15
> > +#define NUM_LEGACY_PREFIXES 4
> >   
> >   #define X86_MODRM_MOD(modrm) (((modrm) & 0xc0) >> 6)
> >   #define X86_MODRM_REG(modrm) (((modrm) & 0x38) >> 3)
> > @@ -201,6 +202,21 @@ 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_LEGACY_PREFIXES if some prefixes
> > + * are repeated, it cannot be used for looping over the prefixes.
> > + */
> > +#define for_each_insn_prefix(insn, idx, prefix)	\
> > +	for (idx = 0; idx < NUM_LEGACY_PREFIXES && (prefix = insn->prefixes.bytes[idx]) != 0; idx++)
> 
> Since this is based on the array size, can
> 
> 	idx < NUM_LEGACY_PREFIXES
> 
> be replaced with:
> 
> 	idx < ARRAY_SIZE(insn->prefixes.bytes)

You're right.
There are 2 issues are mixed in this solution.

- 4 bytes limitation comes from the prefixes.bytes size, because
  it is mapped to insn_value_t.
  
struct insn_field {
        union {
                insn_value_t value;
                insn_byte_t bytes[4];
        };

- The restriction that one instruction can have up to four types
  of prefixes comes from Intel's specification.

Intel SDM Vol.2 
----
2.1.1 Instruction Prefixes
Instruction prefixes are divided into four groups, each with a set of allowable prefix codes. For each instruction, it
is only useful to include up to one prefix code from each of the four groups (Groups 1, 2, 3, 4).
----

So, we need 2 macros to fix this.

#define NUM_INSN_FIELD_BYTES	(sizeof(insn_value_t) / sizeof(insn_byte_t))
#define NUM_LEGACY_PREFIX_GROUPS	4	/* See Intel SDM Vol.2 2.2.1 Instruction Prefixes */

Thank you,

> 
> ?
> 
> Thanks,
> Tom
> 
> > +
> >   #define POP_SS_OPCODE 0x1f
> >   #define MOV_SREG_OPCODE 0x8e
> >   


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

[Masami Hiramatsu]

On Thu, 3 Dec 2020 13:37:57 +0100
Borislav Petkov <bp@alien8.de> wrote:

> On Thu, Dec 03, 2020 at 01:50:37PM +0900, Masami Hiramatsu wrote:
> > 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.
> > 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 v2:
> >   - Add for_each_insn_prefix() macro and fix to check index first.
> > ---
> >  arch/x86/include/asm/insn.h |   15 +++++++++++++++
> >  arch/x86/kernel/uprobes.c   |   10 ++++++----
> >  2 files changed, 21 insertions(+), 4 deletions(-)
> 
> Warning: Kernel ABI header at 'tools/arch/x86/include/asm/insn.h' differs from latest version at 'arch/x86/include/asm/insn.h'

Oops.

> 
> > diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
> > index 5c1ae3eff9d4..e6b38ebd3a1c 100644
> > --- a/arch/x86/include/asm/insn.h
> > +++ b/arch/x86/include/asm/insn.h
> > @@ -201,6 +201,21 @@ 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 4 if some prefixes are repeated,
> > + * we can not use it for looping over the prefixes.
> 
> Please use passive voice: no "we" or "I", etc,

OK.

> 
> > + */
> > +#define for_each_insn_prefix(insn, idx, prefix)	\
> > +	for (idx = 0; idx < 4 && (prefix = insn->prefixes.bytes[idx]) != 0; idx++)
> 
> Btw, looking at the struct insn definition, that prefixes member should
> have a comment above it that those are the legacy prefixes which can be
> <= 4. But that's minor.
> 
> In any case, I'll fix up the minor issues now but pls remember to do
> them in the future.

OK, I will add a macro with comment for it.

Thank you,

> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[tip: x86/urgent] x86/uprobes: Do not use prefixes.nbytes when looping over prefixes.bytes

[tip-bot2 for Masami Hiramatsu]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     9dc23f960adb9ce410ef835b32a2398fdb09c828
Gitweb:        https://git.kernel.org/tip/9dc23f960adb9ce410ef835b32a2398fdb09c828
Author:        Masami Hiramatsu <mhiramat@kernel.org>
AuthorDate:    Thu, 03 Dec 2020 13:50:37 +09:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Fri, 04 Dec 2020 14:32:57 +01:00

x86/uprobes: Do not use prefixes.nbytes when looping over prefixes.bytes

Since insn.prefixes.nbytes can be bigger than the size of
insn.prefixes.bytes[] when a prefix is repeated, the proper check must
be

  insn.prefixes.bytes[i] != 0 and i < 4

instead of using insn.prefixes.nbytes.

Introduce a for_each_insn_prefix() macro for this purpose. Debugged by
Kees Cook <keescook@chromium.org>.

 [ bp: Massage commit message, sync with the respective header in tools/
   and drop "we". ]

Fixes: 2b1444983508 ("uprobes, mm, x86: Add the ability to install and remove uprobes breakpoints")
Reported-by: syzbot+9b64b619f10f19d19a7c@syzkaller.appspotmail.com
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/160697103739.3146288.7437620795200799020.stgit@devnote2
---
 arch/x86/include/asm/insn.h       | 15 +++++++++++++++
 arch/x86/kernel/uprobes.c         | 10 ++++++----
 tools/arch/x86/include/asm/insn.h | 17 ++++++++++++++++-
 3 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
index 5c1ae3e..a8c3d28 100644
--- a/arch/x86/include/asm/insn.h
+++ b/arch/x86/include/asm/insn.h
@@ -201,6 +201,21 @@ 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 4 if some prefixes
+ * are repeated, it cannot be used for looping over the prefixes.
+ */
+#define for_each_insn_prefix(insn, idx, prefix)	\
+	for (idx = 0; idx < ARRAY_SIZE(insn->prefixes.bytes) && (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 3fdaa04..138bdb1 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/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
index 568854b..a8c3d28 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 <asm/inat.h>
 
 struct insn_field {
 	union {
@@ -201,6 +201,21 @@ 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 4 if some prefixes
+ * are repeated, it cannot be used for looping over the prefixes.
+ */
+#define for_each_insn_prefix(insn, idx, prefix)	\
+	for (idx = 0; idx < ARRAY_SIZE(insn->prefixes.bytes) && (prefix = insn->prefixes.bytes[idx]) != 0; idx++)
+
 #define POP_SS_OPCODE 0x1f
 #define MOV_SREG_OPCODE 0x8e
 

Re: [tip: x86/urgent] x86/uprobes: Do not use prefixes.nbytes when looping over prefixes.bytes

[Masami Hiramatsu]

On Fri, 04 Dec 2020 15:04:03 -0000
"tip-bot2 for Masami Hiramatsu" <tip-bot2@linutronix.de> wrote:

> The following commit has been merged into the x86/urgent branch of tip:
> 
> Commit-ID:     9dc23f960adb9ce410ef835b32a2398fdb09c828
> Gitweb:        https://git.kernel.org/tip/9dc23f960adb9ce410ef835b32a2398fdb09c828
> Author:        Masami Hiramatsu <mhiramat@kernel.org>
> AuthorDate:    Thu, 03 Dec 2020 13:50:37 +09:00
> Committer:     Borislav Petkov <bp@suse.de>
> CommitterDate: Fri, 04 Dec 2020 14:32:57 +01:00
> 
> x86/uprobes: Do not use prefixes.nbytes when looping over prefixes.bytes
> 
> Since insn.prefixes.nbytes can be bigger than the size of
> insn.prefixes.bytes[] when a prefix is repeated, the proper check must
> be
> 
>   insn.prefixes.bytes[i] != 0 and i < 4
> 
> instead of using insn.prefixes.nbytes.
> 
> Introduce a for_each_insn_prefix() macro for this purpose. Debugged by
> Kees Cook <keescook@chromium.org>.
> 
>  [ bp: Massage commit message, sync with the respective header in tools/
>    and drop "we". ]
> 
> Fixes: 2b1444983508 ("uprobes, mm, x86: Add the ability to install and remove uprobes breakpoints")
> Reported-by: syzbot+9b64b619f10f19d19a7c@syzkaller.appspotmail.com
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Cc: stable@vger.kernel.org
> Link: https://lkml.kernel.org/r/160697103739.3146288.7437620795200799020.stgit@devnote2
> ---
>  arch/x86/include/asm/insn.h       | 15 +++++++++++++++
>  arch/x86/kernel/uprobes.c         | 10 ++++++----
>  tools/arch/x86/include/asm/insn.h | 17 ++++++++++++++++-
>  3 files changed, 37 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
> index 5c1ae3e..a8c3d28 100644
> --- a/arch/x86/include/asm/insn.h
> +++ b/arch/x86/include/asm/insn.h
> @@ -201,6 +201,21 @@ 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 4 if some prefixes
> + * are repeated, it cannot be used for looping over the prefixes.
> + */
> +#define for_each_insn_prefix(insn, idx, prefix)	\
> +	for (idx = 0; idx < ARRAY_SIZE(insn->prefixes.bytes) && (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 3fdaa04..138bdb1 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/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
> index 568854b..a8c3d28 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 <asm/inat.h>

This may break tools/objtool build. Please keep "inat.h".

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [tip: x86/urgent] x86/uprobes: Do not use prefixes.nbytes when looping over prefixes.bytes

[Borislav Petkov]

On Sat, Dec 05, 2020 at 09:12:56AM +0900, Masami Hiramatsu wrote:
> This may break tools/objtool build. Please keep "inat.h".

How? Please elaborate.

Build tests are fine here.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [tip: x86/urgent] x86/uprobes: Do not use prefixes.nbytes when looping over prefixes.bytes

[Masami Hiramatsu]

On Sat, 5 Dec 2020 11:17:04 +0100
Borislav Petkov <bp@alien8.de> wrote:

> On Sat, Dec 05, 2020 at 09:12:56AM +0900, Masami Hiramatsu wrote:
> > This may break tools/objtool build. Please keep "inat.h".
> 
> How? Please elaborate.
> 
> Build tests are fine here.

Oops, sorry, it was for perf build.

Please refer commit 00a263902ac3 ("perf intel-pt: Use shared x86 insn decoder").

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [tip: x86/urgent] x86/uprobes: Do not use prefixes.nbytes when looping over prefixes.bytes

[Borislav Petkov]

( drop stable@ )

On Sun, Dec 06, 2020 at 12:53:25PM +0900, Masami Hiramatsu wrote:
> On Sat, 5 Dec 2020 11:17:04 +0100
> Borislav Petkov <bp@alien8.de> wrote:
> 
> > On Sat, Dec 05, 2020 at 09:12:56AM +0900, Masami Hiramatsu wrote:
> > > This may break tools/objtool build. Please keep "inat.h".
> > 
> > How? Please elaborate.
> > 
> > Build tests are fine here.
> 
> Oops, sorry, it was for perf build.
> 
> Please refer commit 00a263902ac3 ("perf intel-pt: Use shared x86 insn decoder").

Oh wow:

"This way we continue to be able to process perf.data files with Intel PT
 traces in distros other than x86."

acme, why is that? Can you explain pls?

It probably would be better to fix this so that copying insn.h to keep
it in sync won't cause any future breakages. Or the diffing check should
verify whether header paths are wrong in the tools/ version and fail if
so, so that we don't break it.

Hmmm.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg

Re: [tip: x86/urgent] x86/uprobes: Do not use prefixes.nbytes when looping over prefixes.bytes

[Arnaldo Carvalho de Melo]

Em Sun, Dec 06, 2020 at 10:02:50AM +0100, Borislav Petkov escreveu:
> ( drop stable@ )
> 
> On Sun, Dec 06, 2020 at 12:53:25PM +0900, Masami Hiramatsu wrote:
> > On Sat, 5 Dec 2020 11:17:04 +0100
> > Borislav Petkov <bp@alien8.de> wrote:
> > 
> > > On Sat, Dec 05, 2020 at 09:12:56AM +0900, Masami Hiramatsu wrote:
> > > > This may break tools/objtool build. Please keep "inat.h".
> > > 
> > > How? Please elaborate.
> > > 
> > > Build tests are fine here.
> > 
> > Oops, sorry, it was for perf build.
> > 
> > Please refer commit 00a263902ac3 ("perf intel-pt: Use shared x86 insn decoder").
> 
> Oh wow:
> 
> "This way we continue to be able to process perf.data files with Intel PT
>  traces in distros other than x86."
> 
> acme, why is that? Can you explain pls?
> 
> It probably would be better to fix this so that copying insn.h to keep
> it in sync won't cause any future breakages. Or the diffing check should
> verify whether header paths are wrong in the tools/ version and fail if
> so, so that we don't break it.

Trying to swap this back into my brain...

Humm, if I'm building this on, say, aarch64 then asm/ will not be
pointing to x86, right? Intel PT needs the x86 instruction decoder,
right?

I should've have wrote in the cset comment log if this was related to
cross build failures I encountered, can't remember now :-\

- Arnaldo

Re: [tip: x86/urgent] x86/uprobes: Do not use prefixes.nbytes when looping over prefixes.bytes

[Borislav Petkov]

On Wed, Dec 09, 2020 at 03:01:47PM -0300, Arnaldo Carvalho de Melo wrote:
> Trying to swap this back into my brain...

I know *exactly* what you mean. :)

> 
> Humm, if I'm building this on, say, aarch64 then asm/ will not be
> pointing to x86, right? Intel PT needs the x86 instruction decoder,
> right?

Yeah.

> I should've have wrote in the cset comment log if this was related to
> cross build failures I encountered, can't remember now :-\

I think that is it. There's inat.h in tools/arch/x86/include/asm/ too so
it needs to be exactly that one that gets included on other arches.

> And also it would be interesting to avoid updating both the kernel and
> the tools/ copy, otherwise one would have to test the tools build, which
> may break with such updates.
>
> The whole point of the copy is to avoid that, otherwise we could just
> use the kernel files directly.

Well, there's this diff -u thing which makes sure both copies are in sync.

Why did we ever copy the insn decoder to tools/?

There must've been some reason because otherwise we could probably use
the one in arch/x86/lib/, in tools/.

Yeah, this whole copying of headers back'n'forth is turning out to be
kinda hairy...

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg

Re: [tip: x86/urgent] x86/uprobes: Do not use prefixes.nbytes when looping over prefixes.bytes

[Arnaldo Carvalho de Melo]

Em Sat, Dec 05, 2020 at 09:12:56AM +0900, Masami Hiramatsu escreveu:
> On Fri, 04 Dec 2020 15:04:03 -0000
> "tip-bot2 for Masami Hiramatsu" <tip-bot2@linutronix.de> wrote:
> 
> > The following commit has been merged into the x86/urgent branch of tip:
> > 
> > Commit-ID:     9dc23f960adb9ce410ef835b32a2398fdb09c828
> > Gitweb:        https://git.kernel.org/tip/9dc23f960adb9ce410ef835b32a2398fdb09c828
> > Author:        Masami Hiramatsu <mhiramat@kernel.org>
> > AuthorDate:    Thu, 03 Dec 2020 13:50:37 +09:00
> > Committer:     Borislav Petkov <bp@suse.de>
> > CommitterDate: Fri, 04 Dec 2020 14:32:57 +01:00
> > 
> > x86/uprobes: Do not use prefixes.nbytes when looping over prefixes.bytes
> > 
> > Since insn.prefixes.nbytes can be bigger than the size of
> > insn.prefixes.bytes[] when a prefix is repeated, the proper check must
> > be
> > 
> >   insn.prefixes.bytes[i] != 0 and i < 4
> > 
> > instead of using insn.prefixes.nbytes.
> > 
> > Introduce a for_each_insn_prefix() macro for this purpose. Debugged by
> > Kees Cook <keescook@chromium.org>.
> > 
> >  [ bp: Massage commit message, sync with the respective header in tools/
> >    and drop "we". ]
> > 
> > Fixes: 2b1444983508 ("uprobes, mm, x86: Add the ability to install and remove uprobes breakpoints")
> > Reported-by: syzbot+9b64b619f10f19d19a7c@syzkaller.appspotmail.com
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > Signed-off-by: Borislav Petkov <bp@suse.de>
> > Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> > Cc: stable@vger.kernel.org
> > Link: https://lkml.kernel.org/r/160697103739.3146288.7437620795200799020.stgit@devnote2
> > ---
> >  arch/x86/include/asm/insn.h       | 15 +++++++++++++++
> >  arch/x86/kernel/uprobes.c         | 10 ++++++----
> >  tools/arch/x86/include/asm/insn.h | 17 ++++++++++++++++-
> >  3 files changed, 37 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
> > index 5c1ae3e..a8c3d28 100644
> > --- a/arch/x86/include/asm/insn.h
> > +++ b/arch/x86/include/asm/insn.h
> > @@ -201,6 +201,21 @@ 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 4 if some prefixes
> > + * are repeated, it cannot be used for looping over the prefixes.
> > + */
> > +#define for_each_insn_prefix(insn, idx, prefix)	\
> > +	for (idx = 0; idx < ARRAY_SIZE(insn->prefixes.bytes) && (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 3fdaa04..138bdb1 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/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
> > index 568854b..a8c3d28 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 <asm/inat.h>
> 
> This may break tools/objtool build. Please keep "inat.h".

And also it would be interesting to avoid updating both the kernel and
the tools/ copy, otherwise one would have to test the tools build, which
may break with such updates.

The whole point of the copy is to avoid that, otherwise we could just
use the kernel files directly.

- Arnaldo

[tip: x86/urgent] x86/uprobes: Do not use prefixes.nbytes when looping over prefixes.bytes

[tip-bot2 for Masami Hiramatsu]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     4e9a5ae8df5b3365183150f6df49e49dece80d8c
Gitweb:        https://git.kernel.org/tip/4e9a5ae8df5b3365183150f6df49e49dece80d8c
Author:        Masami Hiramatsu <mhiramat@kernel.org>
AuthorDate:    Thu, 03 Dec 2020 13:50:37 +09:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Sun, 06 Dec 2020 09:58:13 +01:00

x86/uprobes: Do not use prefixes.nbytes when looping over prefixes.bytes

Since insn.prefixes.nbytes can be bigger than the size of
insn.prefixes.bytes[] when a prefix is repeated, the proper check must
be

  insn.prefixes.bytes[i] != 0 and i < 4

instead of using insn.prefixes.nbytes.

Introduce a for_each_insn_prefix() macro for this purpose. Debugged by
Kees Cook <keescook@chromium.org>.

 [ bp: Massage commit message, sync with the respective header in tools/
   and drop "we". ]

Fixes: 2b1444983508 ("uprobes, mm, x86: Add the ability to install and remove uprobes breakpoints")
Reported-by: syzbot+9b64b619f10f19d19a7c@syzkaller.appspotmail.com
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/160697103739.3146288.7437620795200799020.stgit@devnote2
---
 arch/x86/include/asm/insn.h       | 15 +++++++++++++++
 arch/x86/kernel/uprobes.c         | 10 ++++++----
 tools/arch/x86/include/asm/insn.h | 15 +++++++++++++++
 3 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
index 5c1ae3e..a8c3d28 100644
--- a/arch/x86/include/asm/insn.h
+++ b/arch/x86/include/asm/insn.h
@@ -201,6 +201,21 @@ 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 4 if some prefixes
+ * are repeated, it cannot be used for looping over the prefixes.
+ */
+#define for_each_insn_prefix(insn, idx, prefix)	\
+	for (idx = 0; idx < ARRAY_SIZE(insn->prefixes.bytes) && (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 3fdaa04..138bdb1 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/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
index 568854b..52c6262 100644
--- a/tools/arch/x86/include/asm/insn.h
+++ b/tools/arch/x86/include/asm/insn.h
@@ -201,6 +201,21 @@ 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 4 if some prefixes
+ * are repeated, it cannot be used for looping over the prefixes.
+ */
+#define for_each_insn_prefix(insn, idx, prefix)	\
+	for (idx = 0; idx < ARRAY_SIZE(insn->prefixes.bytes) && (prefix = insn->prefixes.bytes[idx]) != 0; idx++)
+
 #define POP_SS_OPCODE 0x1f
 #define MOV_SREG_OPCODE 0x8e
 

[PATCH v2 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;

[tip: x86/urgent] x86/insn-eval: Use new for_each_insn_prefix() macro to loop over prefixes bytes

[tip-bot2 for Masami Hiramatsu]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     2d7896c24ec977e91af1ff93c823032a27212700
Gitweb:        https://git.kernel.org/tip/2d7896c24ec977e91af1ff93c823032a27212700
Author:        Masami Hiramatsu <mhiramat@kernel.org>
AuthorDate:    Thu, 03 Dec 2020 13:50:50 +09:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Fri, 04 Dec 2020 14:33:51 +01:00

x86/insn-eval: Use new for_each_insn_prefix() macro to loop over prefixes bytes

Since insn.prefixes.nbytes can be bigger than the size of
insn.prefixes.bytes[] when a prefix is repeated, the proper check must
be

  insn.prefixes.bytes[i] != 0 and i < 4

instead of using insn.prefixes.nbytes. Use the new
for_each_insn_prefix() macro which does it correctly.

Debugged by Kees Cook <keescook@chromium.org>.

 [ bp: Massage commit message. ]

Fixes: 32d0b95300db ("x86/insn-eval: Add utility functions to get segment selector")
Reported-by: syzbot+9b64b619f10f19d19a7c@syzkaller.appspotmail.com
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/160697104969.3146288.16329307586428270032.stgit@devnote2
---
 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 58f7fb9..4229950 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;

[tip: x86/urgent] x86/insn-eval: Use new for_each_insn_prefix() macro to loop over prefixes bytes

[tip-bot2 for Masami Hiramatsu]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     12cb908a11b2544b5f53e9af856e6b6a90ed5533
Gitweb:        https://git.kernel.org/tip/12cb908a11b2544b5f53e9af856e6b6a90ed5533
Author:        Masami Hiramatsu <mhiramat@kernel.org>
AuthorDate:    Thu, 03 Dec 2020 13:50:50 +09:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Sun, 06 Dec 2020 10:03:08 +01:00

x86/insn-eval: Use new for_each_insn_prefix() macro to loop over prefixes bytes

Since insn.prefixes.nbytes can be bigger than the size of
insn.prefixes.bytes[] when a prefix is repeated, the proper check must
be

  insn.prefixes.bytes[i] != 0 and i < 4

instead of using insn.prefixes.nbytes. Use the new
for_each_insn_prefix() macro which does it correctly.

Debugged by Kees Cook <keescook@chromium.org>.

 [ bp: Massage commit message. ]

Fixes: 32d0b95300db ("x86/insn-eval: Add utility functions to get segment selector")
Reported-by: syzbot+9b64b619f10f19d19a7c@syzkaller.appspotmail.com
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/160697104969.3146288.16329307586428270032.stgit@devnote2
---
 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 58f7fb9..4229950 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 v2 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;
 	}

[tip: x86/urgent] x86/sev-es: Use new for_each_insn_prefix() macro to loop over prefixes bytes

[tip-bot2 for Masami Hiramatsu]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     46a4ad7814fa39971aa6549b30c1a08d5c2ec65f
Gitweb:        https://git.kernel.org/tip/46a4ad7814fa39971aa6549b30c1a08d5c2ec65f
Author:        Masami Hiramatsu <mhiramat@kernel.org>
AuthorDate:    Thu, 03 Dec 2020 13:51:01 +09:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Fri, 04 Dec 2020 14:45:15 +01:00

x86/sev-es: Use new for_each_insn_prefix() macro to loop over prefixes bytes

Since insn.prefixes.nbytes can be bigger than the size of
insn.prefixes.bytes[] when a prefix is repeated, the proper
check must be:

  insn.prefixes.bytes[i] != 0 and i < 4

instead of using insn.prefixes.nbytes. Use the new
for_each_insn_prefix() macro which does it correctly.

Debugged by Kees Cook <keescook@chromium.org>.

 [ bp: Massage commit message. ]

Fixes: 25189d08e516 ("x86/sev-es: Add support for handling IOIO exceptions")
Reported-by: syzbot+9b64b619f10f19d19a7c@syzkaller.appspotmail.com
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Borislav Petkov <bp@suse.de>
Link: https://lkml.kernel.org/r/160697106089.3146288.2052422845039649176.stgit@devnote2
---
 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 954cb27..27826c2 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;
 	}

[tip: x86/urgent] x86/sev-es: Use new for_each_insn_prefix() macro to loop over prefixes bytes

[tip-bot2 for Masami Hiramatsu]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     84da009f06e60cf59d5e861f8e2101d2d3885517
Gitweb:        https://git.kernel.org/tip/84da009f06e60cf59d5e861f8e2101d2d3885517
Author:        Masami Hiramatsu <mhiramat@kernel.org>
AuthorDate:    Thu, 03 Dec 2020 13:51:01 +09:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Sun, 06 Dec 2020 10:03:08 +01:00

x86/sev-es: Use new for_each_insn_prefix() macro to loop over prefixes bytes

Since insn.prefixes.nbytes can be bigger than the size of
insn.prefixes.bytes[] when a prefix is repeated, the proper
check must be:

  insn.prefixes.bytes[i] != 0 and i < 4

instead of using insn.prefixes.nbytes. Use the new
for_each_insn_prefix() macro which does it correctly.

Debugged by Kees Cook <keescook@chromium.org>.

 [ bp: Massage commit message. ]

Fixes: 25189d08e516 ("x86/sev-es: Add support for handling IOIO exceptions")
Reported-by: syzbot+9b64b619f10f19d19a7c@syzkaller.appspotmail.com
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Borislav Petkov <bp@suse.de>
Link: https://lkml.kernel.org/r/160697106089.3146288.2052422845039649176.stgit@devnote2
---
 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 954cb27..27826c2 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;
 	}