Re: [PATCH] remoteproc: qcom: Don't memcpy_toio more than is provided

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Wed 17 Nov 00:54 CST 2021, Stephen Boyd wrote:

> If the string passed into qcom_pil_info_store() isn't as long as
> PIL_RELOC_NAME_LEN we'll try to copy the string assuming the length is
> PIL_RELOC_NAME_LEN to the io space and go beyond the bounds of the
> string. Let's only copy as many byes as the string is long, ignoring the
> NUL terminator.
> 
> This fixes the following KASAN error:
> 
>  BUG: KASAN: global-out-of-bounds in __memcpy_toio+0x124/0x140
>  Read of size 1 at addr ffffffd35086e386 by task rmtfs/2392
> 
>  CPU: 2 PID: 2392 Comm: rmtfs Tainted: G        W         5.16.0-rc1-lockdep+ #10
>  Hardware name: Google Lazor (rev3+) with KB Backlight (DT)
>  Call trace:
>   dump_backtrace+0x0/0x410
>   show_stack+0x24/0x30
>   dump_stack_lvl+0x7c/0xa0
>   print_address_description+0x78/0x2bc
>   kasan_report+0x160/0x1a0
>   __asan_report_load1_noabort+0x44/0x50
>   __memcpy_toio+0x124/0x140
>   qcom_pil_info_store+0x298/0x358 [qcom_pil_info]
>   q6v5_start+0xdf0/0x12e0 [qcom_q6v5_mss]
>   rproc_start+0x178/0x3a0
>   rproc_boot+0x5f0/0xb90
>   state_store+0x78/0x1bc
>   dev_attr_store+0x70/0x90
>   sysfs_kf_write+0xf4/0x118
>   kernfs_fop_write_iter+0x208/0x300
>   vfs_write+0x55c/0x804
>   ksys_pwrite64+0xc8/0x134
>   __arm64_compat_sys_aarch32_pwrite64+0xc4/0xdc
>   invoke_syscall+0x78/0x20c
>   el0_svc_common+0x11c/0x1f0
>   do_el0_svc_compat+0x50/0x60
>   el0_svc_compat+0x5c/0xec
>   el0t_32_sync_handler+0xc0/0xf0
>   el0t_32_sync+0x1a4/0x1a8
> 
>  The buggy address belongs to the variable:
>   .str.59+0x6/0xffffffffffffec80 [qcom_q6v5_mss]
> 
>  Memory state around the buggy address:
>   ffffffd35086e280: 00 00 00 00 02 f9 f9 f9 f9 f9 f9 f9 00 00 00 00
>   ffffffd35086e300: 00 02 f9 f9 f9 f9 f9 f9 00 00 00 06 f9 f9 f9 f9
>  >ffffffd35086e380: 06 f9 f9 f9 05 f9 f9 f9 00 00 00 00 00 06 f9 f9
>                     ^
>   ffffffd35086e400: f9 f9 f9 f9 01 f9 f9 f9 04 f9 f9 f9 00 00 01 f9
>   ffffffd35086e480: f9 f9 f9 f9 00 00 00 00 00 00 00 01 f9 f9 f9 f9
> 
> Fixes: 549b67da660d ("remoteproc: qcom: Introduce helper to store pil info in IMEM")
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Thanks,
Bjorn

> ---
>  drivers/remoteproc/qcom_pil_info.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/qcom_pil_info.c b/drivers/remoteproc/qcom_pil_info.c
> index 7c007dd7b200..aca21560e20b 100644
> --- a/drivers/remoteproc/qcom_pil_info.c
> +++ b/drivers/remoteproc/qcom_pil_info.c
> @@ -104,7 +104,7 @@ int qcom_pil_info_store(const char *image, phys_addr_t base, size_t size)
>  	return -ENOMEM;
>  
>  found_unused:
> -	memcpy_toio(entry, image, PIL_RELOC_NAME_LEN);
> +	memcpy_toio(entry, image, strnlen(image, PIL_RELOC_NAME_LEN));
>  found_existing:
>  	/* Use two writel() as base is only aligned to 4 bytes on odd entries */
>  	writel(base, entry + PIL_RELOC_NAME_LEN);
> 
> base-commit: fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf
> -- 
> https://chromeos.dev
>