| Message ID | 20210308204945.25624-2-tim.gardner@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | x86/kprobes: Fix optprobe to detect INT3 padding correctly | expand |
On 08.03.21 21:49, Tim Gardner wrote: > From: Masami Hiramatsu <mhiramat@kernel.org> > > CVE-2021-3411 > > commit 0d07c0ec4381f630c801539c79ad8dcc627f6e4a upstream. > > Commit > > 7705dc855797 ("x86/vmlinux: Use INT3 instead of NOP for linker fill bytes") > > changed the padding bytes between functions from NOP to INT3. However, > when optprobe decodes a target function it finds INT3 and gives up the > jump optimization. > > Instead of giving up any INT3 detection, check whether the rest of the > bytes to the end of the function are INT3. If all of them are INT3, > those come from the linker. In that case, continue the optprobe jump > optimization. > > [ bp: Massage commit message. ] > > Fixes: 7705dc855797 ("x86/vmlinux: Use INT3 instead of NOP for linker fill bytes") > Reported-by: Adam Zabrocki <pi3@pi3.com.pl> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > Signed-off-by: Borislav Petkov <bp@suse.de> > Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > Reviewed-by: Kees Cook <keescook@chromium.org> > Cc: stable@vger.kernel.org > Link: https://lkml.kernel.org/r/160767025681.3880685.16021570341428835411.stgit@devnote2 > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > (backported from commit d4f949439d2748209b004b4003e21285e580909d) This sha1 seems to come from the linux-5.9.y linux-stable branch, so we should add the "linux-5.9.y" prefix to the line above. > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > > [ commit fe557319aa06c23cffc9346000f119547e0f289a "maccess: rename probe_kernel_{read,write} > to copy_{from,to}_kernel_nofault" renamed probe_kernel_read() to copy_from_kernel_nofault(). > commit 25f12ae45fc1931a1dce3cc59f9989a9d87834b0 "maccess: rename probe_kernel_address to get_kernel_nofault" > added get_kernel_nofault() in include/linux/uaccess.h. However, that commit is too invasive to > backport intact. ] Apart from the small comment above, which we can fix when applying, the backport looks good. Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> > --- > arch/x86/kernel/kprobes/opt.c | 25 +++++++++++++++++++++++-- > 1 file changed, 23 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c > index 3f45b5c43a71..21acfc823a91 100644 > --- a/arch/x86/kernel/kprobes/opt.c > +++ b/arch/x86/kernel/kprobes/opt.c > @@ -247,6 +247,22 @@ static int insn_is_indirect_jump(struct insn *insn) > return ret; > } > > +#define get_kernel_nofault(val, ptr) \ > + probe_kernel_read(&(val), (ptr), sizeof(val)) > + > +static bool is_padding_int3(unsigned long addr, unsigned long eaddr) > +{ > + unsigned char ops; > + > + for (; addr < eaddr; addr++) { > + if (get_kernel_nofault(ops, (void *)addr) < 0 || > + ops != INT3_INSN_OPCODE) > + return false; > + } > + > + return true; > +} > + > /* Decode whole function to ensure any instructions don't jump into target */ > static int can_optimize(unsigned long paddr) > { > @@ -287,9 +303,14 @@ static int can_optimize(unsigned long paddr) > return 0; > kernel_insn_init(&insn, (void *)recovered_insn, MAX_INSN_SIZE); > insn_get_length(&insn); > - /* Another subsystem puts a breakpoint */ > + /* > + * In the case of detecting unknown breakpoint, this could be > + * a padding INT3 between functions. Let's check that all the > + * rest of the bytes are also INT3. > + */ > if (insn.opcode.bytes[0] == INT3_INSN_OPCODE) > - return 0; > + return is_padding_int3(addr, paddr - offset + size) ? 1 : 0; > + > /* Recover address */ > insn.kaddr = (void *)addr; > insn.next_byte = (void *)(addr + insn.length); >
On 3/10/21 4:25 AM, Kleber Souza wrote: > On 08.03.21 21:49, Tim Gardner wrote: >> From: Masami Hiramatsu <mhiramat@kernel.org> >> >> CVE-2021-3411 >> >> commit 0d07c0ec4381f630c801539c79ad8dcc627f6e4a upstream. >> >> Commit >> >> 7705dc855797 ("x86/vmlinux: Use INT3 instead of NOP for linker fill >> bytes") >> >> changed the padding bytes between functions from NOP to INT3. However, >> when optprobe decodes a target function it finds INT3 and gives up the >> jump optimization. >> >> Instead of giving up any INT3 detection, check whether the rest of the >> bytes to the end of the function are INT3. If all of them are INT3, >> those come from the linker. In that case, continue the optprobe jump >> optimization. >> >> [ bp: Massage commit message. ] >> >> Fixes: 7705dc855797 ("x86/vmlinux: Use INT3 instead of NOP for linker >> fill bytes") >> Reported-by: Adam Zabrocki <pi3@pi3.com.pl> >> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> >> Signed-off-by: Borislav Petkov <bp@suse.de> >> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org> >> Reviewed-by: Kees Cook <keescook@chromium.org> >> Cc: stable@vger.kernel.org >> Link: >> https://lkml.kernel.org/r/160767025681.3880685.16021570341428835411.stgit@devnote2 >> >> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> >> (backported from commit d4f949439d2748209b004b4003e21285e580909d) > > This sha1 seems to come from the linux-5.9.y linux-stable branch, so we > should add the > "linux-5.9.y" prefix to the line above. > Whoops. Indeed it came from v5.9.14. ----------- Tim Gardner Canonical, Inc
LGTM. As Kleber mentioned, backported line should be fixed which can be done when applying. Acked-by: Kelsey Skunberg <kelsey.skunberg@canonical.com> On 2021-03-08 13:49:45 , Tim Gardner wrote: > From: Masami Hiramatsu <mhiramat@kernel.org> > > CVE-2021-3411 > > commit 0d07c0ec4381f630c801539c79ad8dcc627f6e4a upstream. > > Commit > > 7705dc855797 ("x86/vmlinux: Use INT3 instead of NOP for linker fill bytes") > > changed the padding bytes between functions from NOP to INT3. However, > when optprobe decodes a target function it finds INT3 and gives up the > jump optimization. > > Instead of giving up any INT3 detection, check whether the rest of the > bytes to the end of the function are INT3. If all of them are INT3, > those come from the linker. In that case, continue the optprobe jump > optimization. > > [ bp: Massage commit message. ] > > Fixes: 7705dc855797 ("x86/vmlinux: Use INT3 instead of NOP for linker fill bytes") > Reported-by: Adam Zabrocki <pi3@pi3.com.pl> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > Signed-off-by: Borislav Petkov <bp@suse.de> > Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > Reviewed-by: Kees Cook <keescook@chromium.org> > Cc: stable@vger.kernel.org > Link: https://lkml.kernel.org/r/160767025681.3880685.16021570341428835411.stgit@devnote2 > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > (backported from commit d4f949439d2748209b004b4003e21285e580909d) > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > > [ commit fe557319aa06c23cffc9346000f119547e0f289a "maccess: rename probe_kernel_{read,write} > to copy_{from,to}_kernel_nofault" renamed probe_kernel_read() to copy_from_kernel_nofault(). > commit 25f12ae45fc1931a1dce3cc59f9989a9d87834b0 "maccess: rename probe_kernel_address to get_kernel_nofault" > added get_kernel_nofault() in include/linux/uaccess.h. However, that commit is too invasive to > backport intact. ] > --- > arch/x86/kernel/kprobes/opt.c | 25 +++++++++++++++++++++++-- > 1 file changed, 23 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c > index 3f45b5c43a71..21acfc823a91 100644 > --- a/arch/x86/kernel/kprobes/opt.c > +++ b/arch/x86/kernel/kprobes/opt.c > @@ -247,6 +247,22 @@ static int insn_is_indirect_jump(struct insn *insn) > return ret; > } > > +#define get_kernel_nofault(val, ptr) \ > + probe_kernel_read(&(val), (ptr), sizeof(val)) > + > +static bool is_padding_int3(unsigned long addr, unsigned long eaddr) > +{ > + unsigned char ops; > + > + for (; addr < eaddr; addr++) { > + if (get_kernel_nofault(ops, (void *)addr) < 0 || > + ops != INT3_INSN_OPCODE) > + return false; > + } > + > + return true; > +} > + > /* Decode whole function to ensure any instructions don't jump into target */ > static int can_optimize(unsigned long paddr) > { > @@ -287,9 +303,14 @@ static int can_optimize(unsigned long paddr) > return 0; > kernel_insn_init(&insn, (void *)recovered_insn, MAX_INSN_SIZE); > insn_get_length(&insn); > - /* Another subsystem puts a breakpoint */ > + /* > + * In the case of detecting unknown breakpoint, this could be > + * a padding INT3 between functions. Let's check that all the > + * rest of the bytes are also INT3. > + */ > if (insn.opcode.bytes[0] == INT3_INSN_OPCODE) > - return 0; > + return is_padding_int3(addr, paddr - offset + size) ? 1 : 0; > + > /* Recover address */ > insn.kaddr = (void *)addr; > insn.next_byte = (void *)(addr + insn.length); > -- > 2.17.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c index 3f45b5c43a71..21acfc823a91 100644 --- a/arch/x86/kernel/kprobes/opt.c +++ b/arch/x86/kernel/kprobes/opt.c @@ -247,6 +247,22 @@ static int insn_is_indirect_jump(struct insn *insn) return ret; } +#define get_kernel_nofault(val, ptr) \ + probe_kernel_read(&(val), (ptr), sizeof(val)) + +static bool is_padding_int3(unsigned long addr, unsigned long eaddr) +{ + unsigned char ops; + + for (; addr < eaddr; addr++) { + if (get_kernel_nofault(ops, (void *)addr) < 0 || + ops != INT3_INSN_OPCODE) + return false; + } + + return true; +} + /* Decode whole function to ensure any instructions don't jump into target */ static int can_optimize(unsigned long paddr) { @@ -287,9 +303,14 @@ static int can_optimize(unsigned long paddr) return 0; kernel_insn_init(&insn, (void *)recovered_insn, MAX_INSN_SIZE); insn_get_length(&insn); - /* Another subsystem puts a breakpoint */ + /* + * In the case of detecting unknown breakpoint, this could be + * a padding INT3 between functions. Let's check that all the + * rest of the bytes are also INT3. + */ if (insn.opcode.bytes[0] == INT3_INSN_OPCODE) - return 0; + return is_padding_int3(addr, paddr - offset + size) ? 1 : 0; + /* Recover address */ insn.kaddr = (void *)addr; insn.next_byte = (void *)(addr + insn.length);