* [PATCH] remoteproc: qcom: Don't memcpy_toio more than is provided
@ 2021-11-17 6:54 Stephen Boyd
2021-11-17 14:46 ` Bjorn Andersson
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Stephen Boyd @ 2021-11-17 6:54 UTC (permalink / raw)
To: Bjorn Andersson; +Cc: linux-kernel, linux-arm-msm
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>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] remoteproc: qcom: Don't memcpy_toio more than is provided
2021-11-17 6:54 [PATCH] remoteproc: qcom: Don't memcpy_toio more than is provided Stephen Boyd
@ 2021-11-17 14:46 ` Bjorn Andersson
2021-12-07 0:01 ` Doug Anderson
2021-12-15 22:44 ` Bjorn Andersson
2 siblings, 0 replies; 6+ messages in thread
From: Bjorn Andersson @ 2021-11-17 14:46 UTC (permalink / raw)
To: Stephen Boyd; +Cc: linux-kernel, linux-arm-msm
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] remoteproc: qcom: Don't memcpy_toio more than is provided
2021-11-17 6:54 [PATCH] remoteproc: qcom: Don't memcpy_toio more than is provided Stephen Boyd
2021-11-17 14:46 ` Bjorn Andersson
@ 2021-12-07 0:01 ` Doug Anderson
2021-12-07 0:24 ` Bjorn Andersson
2021-12-15 22:44 ` Bjorn Andersson
2 siblings, 1 reply; 6+ messages in thread
From: Doug Anderson @ 2021-12-07 0:01 UTC (permalink / raw)
To: Stephen Boyd; +Cc: Bjorn Andersson, linux-kernel, linux-arm-msm
Hi,
On Tue, Nov 16, 2021 at 10:55 PM Stephen Boyd <swboyd@chromium.org> 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>
> ---
> 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));
The above seems slightly sketchy...
Let's say:
image = "modem" (5 characters plus null termination)
PIL_RELOC_NAME_LEN = 8
...so strnlen(image, 8) = 5, right?
...so we'll copy characters _not_ including the NULL termination.
I guess that's OK as long as we're certain that the destination was
zero-initted, but maybe it would be better/safer to write:
memcpy_toio(entry, image, strnlen(image, PIL_RELOC_NAME_LEN - 1) + 1);
-Doug
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] remoteproc: qcom: Don't memcpy_toio more than is provided
2021-12-07 0:01 ` Doug Anderson
@ 2021-12-07 0:24 ` Bjorn Andersson
2021-12-07 0:34 ` Doug Anderson
0 siblings, 1 reply; 6+ messages in thread
From: Bjorn Andersson @ 2021-12-07 0:24 UTC (permalink / raw)
To: Doug Anderson; +Cc: Stephen Boyd, linux-kernel, linux-arm-msm
On Mon 06 Dec 16:01 PST 2021, Doug Anderson wrote:
> Hi,
>
> On Tue, Nov 16, 2021 at 10:55 PM Stephen Boyd <swboyd@chromium.org> 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>
> > ---
> > 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));
>
> The above seems slightly sketchy...
>
> Let's say:
>
> image = "modem" (5 characters plus null termination)
> PIL_RELOC_NAME_LEN = 8
>
> ...so strnlen(image, 8) = 5, right?
> ...so we'll copy characters _not_ including the NULL termination.
>
> I guess that's OK as long as we're certain that the destination was
> zero-initted, but maybe it would be better/safer to write:
>
> memcpy_toio(entry, image, strnlen(image, PIL_RELOC_NAME_LEN - 1) + 1);
>
Yes, I agree that it looks a little bit sketchy.
But we have to assume that either there's remnants from the boot stages
or perhaps remnants of stale data from before a reboot. Therefor I
concluded as I wrote this that I had to memset() the entire region in
qcom_pil_info_init(), so this is taken care of.
The region being zero-initialized is also a requirement for the
conditional jump to "found_unused" based on !buf[0]...
Regards,
Bjorn
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] remoteproc: qcom: Don't memcpy_toio more than is provided
2021-12-07 0:24 ` Bjorn Andersson
@ 2021-12-07 0:34 ` Doug Anderson
0 siblings, 0 replies; 6+ messages in thread
From: Doug Anderson @ 2021-12-07 0:34 UTC (permalink / raw)
To: Bjorn Andersson; +Cc: Stephen Boyd, linux-kernel, linux-arm-msm
Hi,
On Mon, Dec 6, 2021 at 4:23 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Mon 06 Dec 16:01 PST 2021, Doug Anderson wrote:
>
> > Hi,
> >
> > On Tue, Nov 16, 2021 at 10:55 PM Stephen Boyd <swboyd@chromium.org> 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>
> > > ---
> > > 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));
> >
> > The above seems slightly sketchy...
> >
> > Let's say:
> >
> > image = "modem" (5 characters plus null termination)
> > PIL_RELOC_NAME_LEN = 8
> >
> > ...so strnlen(image, 8) = 5, right?
> > ...so we'll copy characters _not_ including the NULL termination.
> >
> > I guess that's OK as long as we're certain that the destination was
> > zero-initted, but maybe it would be better/safer to write:
> >
> > memcpy_toio(entry, image, strnlen(image, PIL_RELOC_NAME_LEN - 1) + 1);
> >
>
> Yes, I agree that it looks a little bit sketchy.
>
> But we have to assume that either there's remnants from the boot stages
> or perhaps remnants of stale data from before a reboot. Therefor I
> concluded as I wrote this that I had to memset() the entire region in
> qcom_pil_info_init(), so this is taken care of.
Ah, right! I had missed the memset_io() in qcom_pil_info_init()
> The region being zero-initialized is also a requirement for the
> conditional jump to "found_unused" based on !buf[0]...
OK, makes sense. Even though that's only checking the first character
we know that the whole thing must be zero-initted because we memset it
and never write it more than once. OK, you have me convinced.
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] remoteproc: qcom: Don't memcpy_toio more than is provided
2021-11-17 6:54 [PATCH] remoteproc: qcom: Don't memcpy_toio more than is provided Stephen Boyd
2021-11-17 14:46 ` Bjorn Andersson
2021-12-07 0:01 ` Doug Anderson
@ 2021-12-15 22:44 ` Bjorn Andersson
2 siblings, 0 replies; 6+ messages in thread
From: Bjorn Andersson @ 2021-12-15 22:44 UTC (permalink / raw)
To: Stephen Boyd; +Cc: linux-kernel, linux-arm-msm
On Tue, 16 Nov 2021 22:54:54 -0800, 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:
>
> [...]
Applied, thanks!
[1/1] remoteproc: qcom: Don't memcpy_toio more than is provided
commit: fdc12231d885119cc2e2b4f3e0fbba3155f37a56
Best regards,
--
Bjorn Andersson <bjorn.andersson@linaro.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-12-15 22:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-17 6:54 [PATCH] remoteproc: qcom: Don't memcpy_toio more than is provided Stephen Boyd
2021-11-17 14:46 ` Bjorn Andersson
2021-12-07 0:01 ` Doug Anderson
2021-12-07 0:24 ` Bjorn Andersson
2021-12-07 0:34 ` Doug Anderson
2021-12-15 22:44 ` Bjorn Andersson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).