Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 18 May 2020 11:20:59 +0900
From:   Masami Hiramatsu <mhiramat@kernel.org>
To:     Kees Cook <keescook@chromium.org>
Cc:     Andrew Jeffery <andrew@aj.id.au>,
        linux-arm-kernel@lists.infradead.org, linux@armlinux.org.uk,
        mhiramat@kernel.org, labbott@redhat.com,
        mathieu.desnoyers@efficios.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] ARM: kprobes: Avoid fortify_panic() when copying
 optprobe template
Message-Id: <20200518112059.c19899ffb17a4843bf4f74ab@kernel.org>
In-Reply-To: <202005171447.00CFE0C@keescook>
References: <20200517153959.293224-1-andrew@aj.id.au>
        <202005171447.00CFE0C@keescook>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, 17 May 2020 14:48:52 -0700
Kees Cook <keescook@chromium.org> wrote:

> On Mon, May 18, 2020 at 01:09:59AM +0930, Andrew Jeffery wrote:
> > As mentioned, a couple of attempts have been made to address the issue
> > by casting a pointer to optprobe_template_entry before providing it to
> > memcpy(), however gccs such as Ubuntu 20.04's arm-linux-gnueabi-gcc
> > 9.3.0 (Ubuntu 9.3.0-10ubuntu1) see through these efforts.
> 
> Ah, dang. :P
> 
> How about converting them all to unsized arrays, which would also allow
> the code to drop the "&" everywhere, I think. This is untested:
> 

This looks good to me since it uses same technique in sections.h.

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you!

> 
> diff --git a/arch/arm/include/asm/kprobes.h b/arch/arm/include/asm/kprobes.h
> index 213607a1f45c..e26a278d301a 100644
> --- a/arch/arm/include/asm/kprobes.h
> +++ b/arch/arm/include/asm/kprobes.h
> @@ -44,20 +44,20 @@ int kprobe_exceptions_notify(struct notifier_block *self,
>  			     unsigned long val, void *data);
>  
>  /* optinsn template addresses */
> -extern __visible kprobe_opcode_t optprobe_template_entry;
> -extern __visible kprobe_opcode_t optprobe_template_val;
> -extern __visible kprobe_opcode_t optprobe_template_call;
> -extern __visible kprobe_opcode_t optprobe_template_end;
> -extern __visible kprobe_opcode_t optprobe_template_sub_sp;
> -extern __visible kprobe_opcode_t optprobe_template_add_sp;
> -extern __visible kprobe_opcode_t optprobe_template_restore_begin;
> -extern __visible kprobe_opcode_t optprobe_template_restore_orig_insn;
> -extern __visible kprobe_opcode_t optprobe_template_restore_end;
> +extern __visible kprobe_opcode_t optprobe_template_entry[];
> +extern __visible kprobe_opcode_t optprobe_template_val[];
> +extern __visible kprobe_opcode_t optprobe_template_call[];
> +extern __visible kprobe_opcode_t optprobe_template_end[];
> +extern __visible kprobe_opcode_t optprobe_template_sub_sp[];
> +extern __visible kprobe_opcode_t optprobe_template_add_sp[];
> +extern __visible kprobe_opcode_t optprobe_template_restore_begin[];
> +extern __visible kprobe_opcode_t optprobe_template_restore_orig_insn[];
> +extern __visible kprobe_opcode_t optprobe_template_restore_end[];
>  
>  #define MAX_OPTIMIZED_LENGTH	4
>  #define MAX_OPTINSN_SIZE				\
> -	((unsigned long)&optprobe_template_end -	\
> -	 (unsigned long)&optprobe_template_entry)
> +	((unsigned long)optprobe_template_end -	\
> +	 (unsigned long)optprobe_template_entry)
>  #define RELATIVEJUMP_SIZE	4
>  
>  struct arch_optimized_insn {
> diff --git a/arch/arm/probes/kprobes/opt-arm.c b/arch/arm/probes/kprobes/opt-arm.c
> index 7a449df0b359..c78180172120 100644
> --- a/arch/arm/probes/kprobes/opt-arm.c
> +++ b/arch/arm/probes/kprobes/opt-arm.c
> @@ -85,21 +85,21 @@ asm (
>  			"optprobe_template_end:\n");
>  
>  #define TMPL_VAL_IDX \
> -	((unsigned long *)&optprobe_template_val - (unsigned long *)&optprobe_template_entry)
> +	((unsigned long *)optprobe_template_val - (unsigned long *)optprobe_template_entry)
>  #define TMPL_CALL_IDX \
> -	((unsigned long *)&optprobe_template_call - (unsigned long *)&optprobe_template_entry)
> +	((unsigned long *)optprobe_template_call - (unsigned long *)optprobe_template_entry)
>  #define TMPL_END_IDX \
> -	((unsigned long *)&optprobe_template_end - (unsigned long *)&optprobe_template_entry)
> +	((unsigned long *)optprobe_template_end - (unsigned long *)optprobe_template_entry)
>  #define TMPL_ADD_SP \
> -	((unsigned long *)&optprobe_template_add_sp - (unsigned long *)&optprobe_template_entry)
> +	((unsigned long *)optprobe_template_add_sp - (unsigned long *)optprobe_template_entry)
>  #define TMPL_SUB_SP \
> -	((unsigned long *)&optprobe_template_sub_sp - (unsigned long *)&optprobe_template_entry)
> +	((unsigned long *)optprobe_template_sub_sp - (unsigned long *)optprobe_template_entry)
>  #define TMPL_RESTORE_BEGIN \
> -	((unsigned long *)&optprobe_template_restore_begin - (unsigned long *)&optprobe_template_entry)
> +	((unsigned long *)optprobe_template_restore_begin - (unsigned long *)optprobe_template_entry)
>  #define TMPL_RESTORE_ORIGN_INSN \
> -	((unsigned long *)&optprobe_template_restore_orig_insn - (unsigned long *)&optprobe_template_entry)
> +	((unsigned long *)optprobe_template_restore_orig_insn - (unsigned long *)optprobe_template_entry)
>  #define TMPL_RESTORE_END \
> -	((unsigned long *)&optprobe_template_restore_end - (unsigned long *)&optprobe_template_entry)
> +	((unsigned long *)optprobe_template_restore_end - (unsigned long *)optprobe_template_entry)
>  
>  /*
>   * ARM can always optimize an instruction when using ARM ISA, except
> @@ -234,7 +234,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *or
>  	}
>  
>  	/* Copy arch-dep-instance from template. */
> -	memcpy(code, (unsigned long *)&optprobe_template_entry,
> +	memcpy(code, (unsigned long *)optprobe_template_entry,
>  			TMPL_END_IDX * sizeof(kprobe_opcode_t));
>  
>  	/* Adjust buffer according to instruction. */
> 
> -- 
> Kees Cook


-- 
Masami Hiramatsu <mhiramat@kernel.org>