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

2020-12-10 Thread 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

2020-12-09 Thread 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 -
> "tip-bot2 for Masami Hiramatsu"  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 
> > AuthorDate:Thu, 03 Dec 2020 13:50:37 +09:00
> > Committer: Borislav Petkov 
> > 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 .
> > 
> >  [ 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+9b64b619f10f19d19...@syzkaller.appspotmail.com
> > Signed-off-by: Masami Hiramatsu 
> > Signed-off-by: Borislav Petkov 
> > Reviewed-by: Srikar Dronamraju 
> > Cc: sta...@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 
> 
> 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 

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

2020-12-09 Thread 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  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


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

2020-12-06 Thread 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 
AuthorDate:Thu, 03 Dec 2020 13:50:37 +09:00
Committer: Borislav Petkov 
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 .

 [ 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+9b64b619f10f19d19...@syzkaller.appspotmail.com
Signed-off-by: Masami Hiramatsu 
Signed-off-by: Borislav Petkov 
Reviewed-by: Srikar Dronamraju 
Cc: sta...@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 

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

2020-12-06 Thread 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  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

2020-12-05 Thread Masami Hiramatsu
On Sat, 5 Dec 2020 11:17:04 +0100
Borislav Petkov  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 


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

2020-12-05 Thread 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

2020-12-04 Thread Masami Hiramatsu
On Fri, 04 Dec 2020 15:04:03 -
"tip-bot2 for Masami Hiramatsu"  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 
> AuthorDate:Thu, 03 Dec 2020 13:50:37 +09:00
> Committer: Borislav Petkov 
> 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 .
> 
>  [ 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+9b64b619f10f19d19...@syzkaller.appspotmail.com
> Signed-off-by: Masami Hiramatsu 
> Signed-off-by: Borislav Petkov 
> Reviewed-by: Srikar Dronamraju 
> Cc: sta...@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 

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

Thank you,

-- 
Masami Hiramatsu 


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

2020-12-04 Thread 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 
AuthorDate:Thu, 03 Dec 2020 13:50:37 +09:00
Committer: Borislav Petkov 
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 .

 [ 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+9b64b619f10f19d19...@syzkaller.appspotmail.com
Signed-off-by: Masami Hiramatsu 
Signed-off-by: Borislav Petkov 
Reviewed-by: Srikar Dronamraju 
Cc: sta...@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 
 
 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)\
+