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) <[email protected]>
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: [email protected]
Debugged-by: Kees Cook <[email protected]>
Signed-off-by: Masami Hiramatsu <[email protected]>
Cc: [email protected]
---
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;
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: [email protected]
Debugged-by: Kees Cook <[email protected]>
Reviewed-by: Srikar Dronamraju <[email protected]>
Signed-off-by: Masami Hiramatsu <[email protected]>
Cc: [email protected]
---
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;
}
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: [email protected]
Debugged-by: Kees Cook <[email protected]>
Signed-off-by: Masami Hiramatsu <[email protected]>
---
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;
}
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: [email protected]
> Debugged-by: Kees Cook <[email protected]>
> Reviewed-by: Srikar Dronamraju <[email protected]>
> Signed-off-by: Masami Hiramatsu <[email protected]>
> Cc: [email protected]
> ---
> 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
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
So it ended up like this:
---
From 5014e4e902778d63ce392f864b3654baa4b72384 Mon Sep 17 00:00:00 2001
From: Masami Hiramatsu <[email protected]>
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 <[email protected]>.
[ 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: [email protected]
Signed-off-by: Masami Hiramatsu <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Reviewed-by: Srikar Dronamraju <[email protected]>
Cc: [email protected]
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
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 <[email protected]>
> 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 <[email protected]>.
>
> [ 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: [email protected]
> Signed-off-by: Masami Hiramatsu <[email protected]>
> Signed-off-by: Borislav Petkov <[email protected]>
> Reviewed-by: Srikar Dronamraju <[email protected]>
> Cc: [email protected]
> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.kernel.org%2Fr%2F160697103739.3146288.7437620795200799020.stgit%40devnote2&data=04%7C01%7Cthomas.lendacky%40amd.com%7Ce8ec706c564245542b4408d89789b727%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637425965056484231%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=csaC0C2cszr45mKES42CHeZjC4TnEJtKrMa0gSmHjn8%3D&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
>
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
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
>
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
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
>
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
On Thu, 3 Dec 2020 10:45:48 -0600
Tom Lendacky <[email protected]> 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 <[email protected]>
> > 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 <[email protected]>.
> >
> > [ 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: [email protected]
> > Signed-off-by: Masami Hiramatsu <[email protected]>
> > Signed-off-by: Borislav Petkov <[email protected]>
> > Reviewed-by: Srikar Dronamraju <[email protected]>
> > Cc: [email protected]
> > Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.kernel.org%2Fr%2F160697103739.3146288.7437620795200799020.stgit%40devnote2&data=04%7C01%7Cthomas.lendacky%40amd.com%7Ce8ec706c564245542b4408d89789b727%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637425965056484231%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=csaC0C2cszr45mKES42CHeZjC4TnEJtKrMa0gSmHjn8%3D&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 <[email protected]>
On Thu, 3 Dec 2020 13:37:57 +0100
Borislav Petkov <[email protected]> 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: [email protected]
> > Debugged-by: Kees Cook <[email protected]>
> > Reviewed-by: Srikar Dronamraju <[email protected]>
> > Signed-off-by: Masami Hiramatsu <[email protected]>
> > Cc: [email protected]
> > ---
> > 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 <[email protected]>
On Thu, 3 Dec 2020 12:49:46 -0600
Tom Lendacky <[email protected]> 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 <[email protected]>
On Fri, 4 Dec 2020 09:56:53 +0900
Masami Hiramatsu <[email protected]> wrote:
> On Thu, 3 Dec 2020 12:49:46 -0600
> Tom Lendacky <[email protected]> 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 <[email protected]>
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
On Fri, 4 Dec 2020 12:06:44 +0100
Borislav Petkov <[email protected]> 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 <[email protected]>
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 <[email protected]>
AuthorDate: Thu, 03 Dec 2020 13:51:01 +09:00
Committer: Borislav Petkov <[email protected]>
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 <[email protected]>.
[ bp: Massage commit message. ]
Fixes: 25189d08e516 ("x86/sev-es: Add support for handling IOIO exceptions")
Reported-by: [email protected]
Signed-off-by: Masami Hiramatsu <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
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;
}
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 <[email protected]>
AuthorDate: Thu, 03 Dec 2020 13:50:50 +09:00
Committer: Borislav Petkov <[email protected]>
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 <[email protected]>.
[ bp: Massage commit message. ]
Fixes: 32d0b95300db ("x86/insn-eval: Add utility functions to get segment selector")
Reported-by: [email protected]
Signed-off-by: Masami Hiramatsu <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Cc: [email protected]
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;
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 <[email protected]>
AuthorDate: Thu, 03 Dec 2020 13:50:37 +09:00
Committer: Borislav Petkov <[email protected]>
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 <[email protected]>.
[ 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: [email protected]
Signed-off-by: Masami Hiramatsu <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Reviewed-by: Srikar Dronamraju <[email protected]>
Cc: [email protected]
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
On Fri, 04 Dec 2020 15:04:03 -0000
"tip-bot2 for Masami Hiramatsu" <[email protected]> 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 <[email protected]>
> AuthorDate: Thu, 03 Dec 2020 13:50:37 +09:00
> Committer: Borislav Petkov <[email protected]>
> 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 <[email protected]>.
>
> [ 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: [email protected]
> Signed-off-by: Masami Hiramatsu <[email protected]>
> Signed-off-by: Borislav Petkov <[email protected]>
> Reviewed-by: Srikar Dronamraju <[email protected]>
> Cc: [email protected]
> 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 <[email protected]>
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
On Sat, 5 Dec 2020 11:17:04 +0100
Borislav Petkov <[email protected]> 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 <[email protected]>
( 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 <[email protected]> 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
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 <[email protected]>
AuthorDate: Thu, 03 Dec 2020 13:50:37 +09:00
Committer: Borislav Petkov <[email protected]>
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 <[email protected]>.
[ 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: [email protected]
Signed-off-by: Masami Hiramatsu <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Reviewed-by: Srikar Dronamraju <[email protected]>
Cc: [email protected]
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
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 <[email protected]>
AuthorDate: Thu, 03 Dec 2020 13:51:01 +09:00
Committer: Borislav Petkov <[email protected]>
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 <[email protected]>.
[ bp: Massage commit message. ]
Fixes: 25189d08e516 ("x86/sev-es: Add support for handling IOIO exceptions")
Reported-by: [email protected]
Signed-off-by: Masami Hiramatsu <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
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;
}
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 <[email protected]>
AuthorDate: Thu, 03 Dec 2020 13:50:50 +09:00
Committer: Borislav Petkov <[email protected]>
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 <[email protected]>.
[ bp: Massage commit message. ]
Fixes: 32d0b95300db ("x86/insn-eval: Add utility functions to get segment selector")
Reported-by: [email protected]
Signed-off-by: Masami Hiramatsu <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Cc: [email protected]
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;
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 <[email protected]> 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
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" <[email protected]> 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 <[email protected]>
> > AuthorDate: Thu, 03 Dec 2020 13:50:37 +09:00
> > Committer: Borislav Petkov <[email protected]>
> > 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 <[email protected]>.
> >
> > [ 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: [email protected]
> > Signed-off-by: Masami Hiramatsu <[email protected]>
> > Signed-off-by: Borislav Petkov <[email protected]>
> > Reviewed-by: Srikar Dronamraju <[email protected]>
> > Cc: [email protected]
> > 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
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