* [PATCH 0/3] x86/insn: Fix not using prefixes.nbytes for loop over prefixes.bytes
@ 2020-12-02 8:50 Masami Hiramatsu
2020-12-02 8:51 ` [PATCH 1/3] x86/sev-es: " Masami Hiramatsu
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Masami Hiramatsu @ 2020-12-02 8:50 UTC (permalink / raw)
To: x86, Thomas Gleixner, Ingo Molnar, Borislav Petkov
Cc: Kees Cook, Masami Hiramatsu, H . Peter Anvin, Joerg Roedel,
Tom Lendacky, Gustavo A . R . Silva, Jann Horn,
Srikar Dronamraju, Ricardo Neri, linux-kernel
Hi,
Here are the patches to fix the wrong loop boundary check
on insn.prefixes.bytes[] array.
Kees Cook reported that this issue that there are 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/sev-es: Fix not using prefixes.nbytes for loop over prefixes.bytes
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
arch/x86/boot/compressed/sev-es.c | 2 +-
arch/x86/kernel/uprobes.c | 4 ++--
arch/x86/lib/insn-eval.c | 4 ++--
3 files changed, 5 insertions(+), 5 deletions(-)
--
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] x86/sev-es: Fix not using prefixes.nbytes for loop over prefixes.bytes
2020-12-02 8:50 [PATCH 0/3] x86/insn: Fix not using prefixes.nbytes for loop over prefixes.bytes Masami Hiramatsu
@ 2020-12-02 8:51 ` Masami Hiramatsu
2020-12-02 15:31 ` Tom Lendacky
2020-12-02 8:51 ` [PATCH 2/3] x86/uprobes: " Masami Hiramatsu
2020-12-02 8:51 ` [PATCH 3/3] x86/insn-eval: " Masami Hiramatsu
2 siblings, 1 reply; 11+ messages in thread
From: Masami Hiramatsu @ 2020-12-02 8:51 UTC (permalink / raw)
To: x86, Thomas Gleixner, Ingo Molnar, Borislav Petkov
Cc: Kees Cook, Masami Hiramatsu, H . Peter Anvin, Joerg Roedel,
Tom Lendacky, Gustavo A . R . Silva, Jann Horn,
Srikar Dronamraju, Ricardo Neri, linux-kernel
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: 25189d08e516 ("x86/sev-es: Add support for handling IOIO exceptions")
Reported-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
arch/x86/boot/compressed/sev-es.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/boot/compressed/sev-es.c b/arch/x86/boot/compressed/sev-es.c
index 954cb2702e23..6a7a3027c9ac 100644
--- a/arch/x86/boot/compressed/sev-es.c
+++ b/arch/x86/boot/compressed/sev-es.c
@@ -36,7 +36,7 @@ static bool insn_has_rep_prefix(struct insn *insn)
insn_get_prefixes(insn);
- for (i = 0; i < insn->prefixes.nbytes; i++) {
+ for (i = 0; insn->prefixes.bytes[i] && i < 4; i++) {
insn_byte_t p = insn->prefixes.bytes[i];
if (p == 0xf2 || p == 0xf3)
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes
2020-12-02 8:50 [PATCH 0/3] x86/insn: Fix not using prefixes.nbytes for loop over prefixes.bytes Masami Hiramatsu
2020-12-02 8:51 ` [PATCH 1/3] x86/sev-es: " Masami Hiramatsu
@ 2020-12-02 8:51 ` Masami Hiramatsu
2020-12-02 14:51 ` Srikar Dronamraju
2020-12-02 19:04 ` Kees Cook
2020-12-02 8:51 ` [PATCH 3/3] x86/insn-eval: " Masami Hiramatsu
2 siblings, 2 replies; 11+ messages in thread
From: Masami Hiramatsu @ 2020-12-02 8:51 UTC (permalink / raw)
To: x86, Thomas Gleixner, Ingo Molnar, Borislav Petkov
Cc: Kees Cook, Masami Hiramatsu, H . Peter Anvin, Joerg Roedel,
Tom Lendacky, Gustavo A . R . Silva, Jann Horn,
Srikar Dronamraju, Ricardo Neri, linux-kernel
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: 2b1444983508 ("uprobes, mm, x86: Add the ability to install and remove uprobes breakpoints")
Cc: stable@vger.kernel.org
Reported-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
arch/x86/kernel/uprobes.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 3fdaa042823d..bb3ea3705b99 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -257,7 +257,7 @@ static bool is_prefix_bad(struct insn *insn)
{
int i;
- for (i = 0; i < insn->prefixes.nbytes; i++) {
+ for (i = 0; insn->prefixes.bytes[i] && i < 4; i++) {
insn_attr_t attr;
attr = inat_get_opcode_attribute(insn->prefixes.bytes[i]);
@@ -746,7 +746,7 @@ 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++) {
+ for (i = 0; insn->prefixes.bytes[i] && i < 4; i++) {
if (insn->prefixes.bytes[i] == 0x66)
return -ENOTSUPP;
}
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] x86/insn-eval: Fix not using prefixes.nbytes for loop over prefixes.bytes
2020-12-02 8:50 [PATCH 0/3] x86/insn: Fix not using prefixes.nbytes for loop over prefixes.bytes Masami Hiramatsu
2020-12-02 8:51 ` [PATCH 1/3] x86/sev-es: " Masami Hiramatsu
2020-12-02 8:51 ` [PATCH 2/3] x86/uprobes: " Masami Hiramatsu
@ 2020-12-02 8:51 ` Masami Hiramatsu
2 siblings, 0 replies; 11+ messages in thread
From: Masami Hiramatsu @ 2020-12-02 8:51 UTC (permalink / raw)
To: x86, Thomas Gleixner, Ingo Molnar, Borislav Petkov
Cc: Kees Cook, Masami Hiramatsu, H . Peter Anvin, Joerg Roedel,
Tom Lendacky, Gustavo A . R . Silva, Jann Horn,
Srikar Dronamraju, Ricardo Neri, linux-kernel
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")
Cc: stable@vger.kernel.org
Reported-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
arch/x86/lib/insn-eval.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
index 58f7fb95c7f4..c52c91461f52 100644
--- a/arch/x86/lib/insn-eval.c
+++ b/arch/x86/lib/insn-eval.c
@@ -67,7 +67,7 @@ bool insn_has_rep_prefix(struct insn *insn)
insn_get_prefixes(insn);
- for (i = 0; i < insn->prefixes.nbytes; i++) {
+ for (i = 0; insn->prefixes.bytes[i] && i < 4; i++) {
insn_byte_t p = insn->prefixes.bytes[i];
if (p == 0xf2 || p == 0xf3)
@@ -99,7 +99,7 @@ static int get_seg_reg_override_idx(struct insn *insn)
insn_get_prefixes(insn);
/* Look for any segment override prefixes. */
- for (i = 0; i < insn->prefixes.nbytes; i++) {
+ for (i = 0; insn->prefixes.bytes[i] && i < 4; i++) {
insn_attr_t attr;
attr = inat_get_opcode_attribute(insn->prefixes.bytes[i]);
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes
2020-12-02 8:51 ` [PATCH 2/3] x86/uprobes: " Masami Hiramatsu
@ 2020-12-02 14:51 ` Srikar Dronamraju
2020-12-03 4:20 ` Masami Hiramatsu
2020-12-02 19:04 ` Kees Cook
1 sibling, 1 reply; 11+ messages in thread
From: Srikar Dronamraju @ 2020-12-02 14:51 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: x86, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Kees Cook,
H . Peter Anvin, Joerg Roedel, Tom Lendacky,
Gustavo A . R . Silva, Jann Horn, Ricardo Neri, linux-kernel
* Masami Hiramatsu <mhiramat@kernel.org> [2020-12-02 17:51:16]:
> 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: 2b1444983508 ("uprobes, mm, x86: Add the ability to install and remove uprobes breakpoints")
> Cc: stable@vger.kernel.org
> Reported-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Looks good to me.
Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
> arch/x86/kernel/uprobes.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 3fdaa042823d..bb3ea3705b99 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -257,7 +257,7 @@ static bool is_prefix_bad(struct insn *insn)
> {
> int i;
>
> - for (i = 0; i < insn->prefixes.nbytes; i++) {
> + for (i = 0; insn->prefixes.bytes[i] && i < 4; i++) {
> insn_attr_t attr;
>
> attr = inat_get_opcode_attribute(insn->prefixes.bytes[i]);
> @@ -746,7 +746,7 @@ 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++) {
> + for (i = 0; insn->prefixes.bytes[i] && i < 4; i++) {
> if (insn->prefixes.bytes[i] == 0x66)
> return -ENOTSUPP;
> }
>
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] x86/sev-es: Fix not using prefixes.nbytes for loop over prefixes.bytes
2020-12-02 8:51 ` [PATCH 1/3] x86/sev-es: " Masami Hiramatsu
@ 2020-12-02 15:31 ` Tom Lendacky
2020-12-02 19:07 ` Kees Cook
0 siblings, 1 reply; 11+ messages in thread
From: Tom Lendacky @ 2020-12-02 15:31 UTC (permalink / raw)
To: Masami Hiramatsu, x86, Thomas Gleixner, Ingo Molnar, Borislav Petkov
Cc: Kees Cook, H . Peter Anvin, Joerg Roedel, Gustavo A . R . Silva,
Jann Horn, Srikar Dronamraju, Ricardo Neri, linux-kernel
On 12/2/20 2:51 AM, 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.
>
> Fixes: 25189d08e516 ("x86/sev-es: Add support for handling IOIO exceptions")
> Reported-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
> arch/x86/boot/compressed/sev-es.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/boot/compressed/sev-es.c b/arch/x86/boot/compressed/sev-es.c
> index 954cb2702e23..6a7a3027c9ac 100644
> --- a/arch/x86/boot/compressed/sev-es.c
> +++ b/arch/x86/boot/compressed/sev-es.c
> @@ -36,7 +36,7 @@ static bool insn_has_rep_prefix(struct insn *insn)
>
> insn_get_prefixes(insn);
>
> - for (i = 0; i < insn->prefixes.nbytes; i++) {
> + for (i = 0; insn->prefixes.bytes[i] && i < 4; i++) {
Wouldn't it be better to create a #define for the size rather than hard
coding 4 in the various files? That would protect everything should the
bytes array size ever change in the future.
Thanks,
Tom
> insn_byte_t p = insn->prefixes.bytes[i];
>
> if (p == 0xf2 || p == 0xf3)
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes
2020-12-02 8:51 ` [PATCH 2/3] x86/uprobes: " Masami Hiramatsu
2020-12-02 14:51 ` Srikar Dronamraju
@ 2020-12-02 19:04 ` Kees Cook
2020-12-03 2:00 ` Masami Hiramatsu
1 sibling, 1 reply; 11+ messages in thread
From: Kees Cook @ 2020-12-02 19:04 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: x86, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H . Peter Anvin, Joerg Roedel, Tom Lendacky,
Gustavo A . R . Silva, Jann Horn, Srikar Dronamraju,
Ricardo Neri, linux-kernel
On Wed, Dec 02, 2020 at 05:51:16PM +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.
>
> Fixes: 2b1444983508 ("uprobes, mm, x86: Add the ability to install and remove uprobes breakpoints")
> Cc: stable@vger.kernel.org
> Reported-by: Kees Cook <keescook@chromium.org>
This should probably be:
Reported-by: syzbot+9b64b619f10f19d19a7c@syzkaller.appspotmail.com
Debugged-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
> arch/x86/kernel/uprobes.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 3fdaa042823d..bb3ea3705b99 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -257,7 +257,7 @@ static bool is_prefix_bad(struct insn *insn)
> {
> int i;
>
> - for (i = 0; i < insn->prefixes.nbytes; i++) {
> + for (i = 0; insn->prefixes.bytes[i] && i < 4; i++) {
> insn_attr_t attr;
>
> attr = inat_get_opcode_attribute(insn->prefixes.bytes[i]);
> @@ -746,7 +746,7 @@ 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++) {
> + for (i = 0; insn->prefixes.bytes[i] && i < 4; i++) {
> if (insn->prefixes.bytes[i] == 0x66)
> return -ENOTSUPP;
> }
>
--
Kees Cook
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] x86/sev-es: Fix not using prefixes.nbytes for loop over prefixes.bytes
2020-12-02 15:31 ` Tom Lendacky
@ 2020-12-02 19:07 ` Kees Cook
2020-12-03 2:03 ` Masami Hiramatsu
0 siblings, 1 reply; 11+ messages in thread
From: Kees Cook @ 2020-12-02 19:07 UTC (permalink / raw)
To: Tom Lendacky
Cc: Masami Hiramatsu, x86, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H . Peter Anvin, Joerg Roedel,
Gustavo A . R . Silva, Jann Horn, Srikar Dronamraju,
Ricardo Neri, linux-kernel
On Wed, Dec 02, 2020 at 09:31:57AM -0600, Tom Lendacky wrote:
> On 12/2/20 2:51 AM, 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.
> >
> > Fixes: 25189d08e516 ("x86/sev-es: Add support for handling IOIO exceptions")
> > Reported-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> > arch/x86/boot/compressed/sev-es.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/boot/compressed/sev-es.c b/arch/x86/boot/compressed/sev-es.c
> > index 954cb2702e23..6a7a3027c9ac 100644
> > --- a/arch/x86/boot/compressed/sev-es.c
> > +++ b/arch/x86/boot/compressed/sev-es.c
> > @@ -36,7 +36,7 @@ static bool insn_has_rep_prefix(struct insn *insn)
> > insn_get_prefixes(insn);
> > - for (i = 0; i < insn->prefixes.nbytes; i++) {
> > + for (i = 0; insn->prefixes.bytes[i] && i < 4; i++) {
You must test "i" before bytes[i] or you still do the out-of-bounds-read.
>
> Wouldn't it be better to create a #define for the size rather than hard
> coding 4 in the various files? That would protect everything should the
> bytes array size ever change in the future.
Agreed, and perhaps instead of repeating the idiom in the for loop, add
a helper like:
#define insn_prefix_valid(prefixes, i) (i >=0 && i < 4 && prefixes->bytes[i])
to be used like:
for (i = 0; insn_prefix_valid(&insn->prefixes, i); i++) {
>
> Thanks,
> Tom
>
> > insn_byte_t p = insn->prefixes.bytes[i];
> > if (p == 0xf2 || p == 0xf3)
> >
--
Kees Cook
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes
2020-12-02 19:04 ` Kees Cook
@ 2020-12-03 2:00 ` Masami Hiramatsu
0 siblings, 0 replies; 11+ messages in thread
From: Masami Hiramatsu @ 2020-12-03 2:00 UTC (permalink / raw)
To: Kees Cook
Cc: x86, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H . Peter Anvin, Joerg Roedel, Tom Lendacky,
Gustavo A . R . Silva, Jann Horn, Srikar Dronamraju,
Ricardo Neri, linux-kernel
On Wed, 2 Dec 2020 11:04:41 -0800
Kees Cook <keescook@chromium.org> wrote:
> On Wed, Dec 02, 2020 at 05:51:16PM +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.
> >
> > Fixes: 2b1444983508 ("uprobes, mm, x86: Add the ability to install and remove uprobes breakpoints")
> > Cc: stable@vger.kernel.org
> > Reported-by: Kees Cook <keescook@chromium.org>
>
> This should probably be:
>
> Reported-by: syzbot+9b64b619f10f19d19a7c@syzkaller.appspotmail.com
> Debugged-by: Kees Cook <keescook@chromium.org>
OK, let me fix it.
Thank you,
>
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> > arch/x86/kernel/uprobes.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> > index 3fdaa042823d..bb3ea3705b99 100644
> > --- a/arch/x86/kernel/uprobes.c
> > +++ b/arch/x86/kernel/uprobes.c
> > @@ -257,7 +257,7 @@ static bool is_prefix_bad(struct insn *insn)
> > {
> > int i;
> >
> > - for (i = 0; i < insn->prefixes.nbytes; i++) {
> > + for (i = 0; insn->prefixes.bytes[i] && i < 4; i++) {
> > insn_attr_t attr;
> >
> > attr = inat_get_opcode_attribute(insn->prefixes.bytes[i]);
> > @@ -746,7 +746,7 @@ 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++) {
> > + for (i = 0; insn->prefixes.bytes[i] && i < 4; i++) {
> > if (insn->prefixes.bytes[i] == 0x66)
> > return -ENOTSUPP;
> > }
> >
>
>
> --
> Kees Cook
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] x86/sev-es: Fix not using prefixes.nbytes for loop over prefixes.bytes
2020-12-02 19:07 ` Kees Cook
@ 2020-12-03 2:03 ` Masami Hiramatsu
0 siblings, 0 replies; 11+ messages in thread
From: Masami Hiramatsu @ 2020-12-03 2:03 UTC (permalink / raw)
To: Kees Cook
Cc: Tom Lendacky, Masami Hiramatsu, x86, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H . Peter Anvin, Joerg Roedel,
Gustavo A . R . Silva, Jann Horn, Srikar Dronamraju,
Ricardo Neri, linux-kernel
On Wed, 2 Dec 2020 11:07:26 -0800
Kees Cook <keescook@chromium.org> wrote:
> On Wed, Dec 02, 2020 at 09:31:57AM -0600, Tom Lendacky wrote:
> > On 12/2/20 2:51 AM, 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.
> > >
> > > Fixes: 25189d08e516 ("x86/sev-es: Add support for handling IOIO exceptions")
> > > Reported-by: Kees Cook <keescook@chromium.org>
> > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > ---
> > > arch/x86/boot/compressed/sev-es.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/boot/compressed/sev-es.c b/arch/x86/boot/compressed/sev-es.c
> > > index 954cb2702e23..6a7a3027c9ac 100644
> > > --- a/arch/x86/boot/compressed/sev-es.c
> > > +++ b/arch/x86/boot/compressed/sev-es.c
> > > @@ -36,7 +36,7 @@ static bool insn_has_rep_prefix(struct insn *insn)
> > > insn_get_prefixes(insn);
> > > - for (i = 0; i < insn->prefixes.nbytes; i++) {
> > > + for (i = 0; insn->prefixes.bytes[i] && i < 4; i++) {
>
> You must test "i" before bytes[i] or you still do the out-of-bounds-read.
Oops, thanks.
>
> >
> > Wouldn't it be better to create a #define for the size rather than hard
> > coding 4 in the various files? That would protect everything should the
> > bytes array size ever change in the future.
>
> Agreed, and perhaps instead of repeating the idiom in the for loop, add
> a helper like:
>
> #define insn_prefix_valid(prefixes, i) (i >=0 && i < 4 && prefixes->bytes[i])
>
> to be used like:
>
> for (i = 0; insn_prefix_valid(&insn->prefixes, i); i++) {
Hm, for all of these usage, they are looping on the prefixes, so
for_each_insn_prefix(insn, idx, prefix) {
...
}
will be simpler.
Thank you,
>
> >
> > Thanks,
> > Tom
> >
> > > insn_byte_t p = insn->prefixes.bytes[i];
> > > if (p == 0xf2 || p == 0xf3)
> > >
>
> --
> Kees Cook
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes
2020-12-02 14:51 ` Srikar Dronamraju
@ 2020-12-03 4:20 ` Masami Hiramatsu
0 siblings, 0 replies; 11+ messages in thread
From: Masami Hiramatsu @ 2020-12-03 4:20 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: x86, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Kees Cook,
H . Peter Anvin, Joerg Roedel, Tom Lendacky,
Gustavo A . R . Silva, Jann Horn, Ricardo Neri, linux-kernel
On Wed, 2 Dec 2020 20:21:35 +0530
Srikar Dronamraju <srikar@linux.vnet.ibm.com> wrote:
> * Masami Hiramatsu <mhiramat@kernel.org> [2020-12-02 17:51:16]:
>
> > 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: 2b1444983508 ("uprobes, mm, x86: Add the ability to install and remove uprobes breakpoints")
> > Cc: stable@vger.kernel.org
> > Reported-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
>
> Looks good to me.
>
> Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Thanks Srikar!
>
> > ---
> > arch/x86/kernel/uprobes.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> > index 3fdaa042823d..bb3ea3705b99 100644
> > --- a/arch/x86/kernel/uprobes.c
> > +++ b/arch/x86/kernel/uprobes.c
> > @@ -257,7 +257,7 @@ static bool is_prefix_bad(struct insn *insn)
> > {
> > int i;
> >
> > - for (i = 0; i < insn->prefixes.nbytes; i++) {
> > + for (i = 0; insn->prefixes.bytes[i] && i < 4; i++) {
> > insn_attr_t attr;
> >
> > attr = inat_get_opcode_attribute(insn->prefixes.bytes[i]);
> > @@ -746,7 +746,7 @@ 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++) {
> > + for (i = 0; insn->prefixes.bytes[i] && i < 4; i++) {
> > if (insn->prefixes.bytes[i] == 0x66)
> > return -ENOTSUPP;
> > }
> >
>
> --
> Thanks and Regards
> Srikar Dronamraju
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-12-03 4:21 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-02 8:50 [PATCH 0/3] x86/insn: Fix not using prefixes.nbytes for loop over prefixes.bytes Masami Hiramatsu
2020-12-02 8:51 ` [PATCH 1/3] x86/sev-es: " Masami Hiramatsu
2020-12-02 15:31 ` Tom Lendacky
2020-12-02 19:07 ` Kees Cook
2020-12-03 2:03 ` Masami Hiramatsu
2020-12-02 8:51 ` [PATCH 2/3] x86/uprobes: " Masami Hiramatsu
2020-12-02 14:51 ` Srikar Dronamraju
2020-12-03 4:20 ` Masami Hiramatsu
2020-12-02 19:04 ` Kees Cook
2020-12-03 2:00 ` Masami Hiramatsu
2020-12-02 8:51 ` [PATCH 3/3] x86/insn-eval: " Masami Hiramatsu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.