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:   Sun, 17 May 2020 17:02:27 +0100
From:   Russell King - ARM Linux admin <linux@armlinux.org.uk>
To:     Andrew Jeffery <andrew@aj.id.au>
Cc:     linux-arm-kernel@lists.infradead.org, keescook@chromium.org,
        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: <20200517160227.GU1551@shell.armlinux.org.uk>
References: <20200517153959.293224-1-andrew@aj.id.au>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200517153959.293224-1-andrew@aj.id.au>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, May 18, 2020 at 01:09:59AM +0930, Andrew Jeffery wrote:
> Setting both CONFIG_KPROBES=y and CONFIG_FORTIFY_SOURCE=y on ARM leads
> to a panic in memcpy() when injecting a kprobe despite the fixes found
> in commit e46daee53bb5 ("ARM: 8806/1: kprobes: Fix false positive with
> FORTIFY_SOURCE") and commit 0ac569bf6a79 ("ARM: 8834/1: Fix: kprobes:
> optimized kprobes illegal instruction").
> 
> arch/arm/include/asm/kprobes.h effectively declares
> the target type of the optprobe_template_entry assembly label as a u32,
> which leads memcpy()'s __builtin_object_size() call to determine that
> the pointed-to object is of size four. In practical terms the symbol is
> used as a handle for the optimised probe assembly template that is at
> least 96 bytes in size. The symbol's use despite its type blows up the
> memcpy() in ARM's arch_prepare_optimized_kprobe() with a false-positive
> fortify_panic() when it should instead copy the optimised probe template
> into place.
> 
> 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.
> 
> Squash the false-positive by aliasing the template assembly with a new
> symbol 'arm_optprobe_template'; declare it as a function object and
> pass the function object as the argument to memcpy() such that
> __builtin_object_size() cannot immediately determine the object size.
> 
> Fixes: e46daee53bb5 ("ARM: 8806/1: kprobes: Fix false positive with FORTIFY_SOURCE")
> Fixes: 0ac569bf6a79 ("ARM: 8834/1: Fix: kprobes: optimized kprobes illegal instruction")
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  arch/arm/include/asm/kprobes.h    | 7 +++++++
>  arch/arm/probes/kprobes/opt-arm.c | 4 +++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/include/asm/kprobes.h b/arch/arm/include/asm/kprobes.h
> index 213607a1f45c..94db8bf25f9c 100644
> --- a/arch/arm/include/asm/kprobes.h
> +++ b/arch/arm/include/asm/kprobes.h
> @@ -43,6 +43,13 @@ int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr);
>  int kprobe_exceptions_notify(struct notifier_block *self,
>  			     unsigned long val, void *data);
>  
> +/*
> + * The optprobe template buffer is not anything that should be called directly,
> + * however describe it as a function to give ourselves a handle to it that
> + * bypasses CONFIG_FORTIFY_SOURCE=y sanity checks in memcpy().
> + */
> +extern __visible void arm_optprobe_template(void);

Does this really need to be globally visible to anything that happens
to include this header?

While we may abhor "extern" declarations and prototypes in .c files, it
seems to me to be entirely reasonable for this to live in opt-arm.c and
remove the .global for this symbol, thereby making this symbol local to
opt-arm.c

> +
>  /* optinsn template addresses */
>  extern __visible kprobe_opcode_t optprobe_template_entry;
>  extern __visible kprobe_opcode_t optprobe_template_val;
> diff --git a/arch/arm/probes/kprobes/opt-arm.c b/arch/arm/probes/kprobes/opt-arm.c
> index 7a449df0b359..59133d59616a 100644
> --- a/arch/arm/probes/kprobes/opt-arm.c
> +++ b/arch/arm/probes/kprobes/opt-arm.c
> @@ -31,6 +31,8 @@
>   * to the stack cost of the instruction.
>   */
>  asm (
> +			".global arm_optprobe_template\n"
> +			"arm_optprobe_template:\n"
>  			".global optprobe_template_entry\n"
>  			"optprobe_template_entry:\n"
>  			".global optprobe_template_sub_sp\n"
> @@ -234,7 +236,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, arm_optprobe_template,
>  			TMPL_END_IDX * sizeof(kprobe_opcode_t));
>  
>  	/* Adjust buffer according to instruction. */
> -- 
> 2.25.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up