* [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
* 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 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 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
* [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
* 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 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
* 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 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
* [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
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.