| Message ID | 20211117065454.4142936-1-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | remoteproc: qcom: Don't memcpy_toio more than is provided | expand |
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 >
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
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
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>
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,
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);