linux-kernel - Re: [PATCH] remoteproc: qcom: Don't memcpy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD=FV=UWT-snUqHjtj11VdSh+ekbzzSjeHmGRGV2LkhUr5iDmA@mail.gmail.com>
Date:   Mon, 6 Dec 2021 16:34:27 -0800
From:   Doug Anderson <dianders@...omium.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     Stephen Boyd <swboyd@...omium.org>, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH] remoteproc: qcom: Don't memcpy_toio more than is provided

Hi,

On Mon, Dec 6, 2021 at 4:23 PM Bjorn Andersson
<bjorn.andersson@...aro.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@...omium.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@...omium.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@...omium.org>
Tested-by: Douglas Anderson <dianders@...omium.org>