ACK/cmnt: [PATCH] x86/kprobes: Fix optprobe to detect INT3 padding correctly

[Kleber Souza]

On 08.03.21 21:49, Tim Gardner wrote:
> From: Masami Hiramatsu <mhiramat at 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 at pi3.com.pl>
> Signed-off-by: Masami Hiramatsu <mhiramat at kernel.org>
> Signed-off-by: Borislav Petkov <bp at suse.de>
> Reviewed-by: Steven Rostedt (VMware) <rostedt at goodmis.org>
> Reviewed-by: Kees Cook <keescook at chromium.org>
> Cc: stable at vger.kernel.org
> Link: https://lkml.kernel.org/r/160767025681.3880685.16021570341428835411.stgit@devnote2
> Signed-off-by: Greg Kroah-Hartman <gregkh at 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 at 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 at 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);
>