linux-kernel - Re: [PATCH v2 2/2] firmware: qcom: scm: fall back to kcalloc() for no SCM device bound

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAMRc=MevCNHSs2jMbMXjoFYY7V8NqKha8jd3aDCgGNvuL3LwEw@mail.gmail.com>
Date: Tue, 17 Sep 2024 12:17:19 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Stephan Gerhold <stephan.gerhold@...aro.org>
Cc: Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio <konradybcio@...nel.org>, 
	Andrew Halaney <ahalaney@...hat.com>, Elliot Berman <quic_eberman@...cinc.com>, 
	Dmitry Baryshkov <dmitry.baryshkov@...aro.org>, Rudraksha Gupta <guptarud@...il.com>, 
	"Linux regression tracking (Thorsten Leemhuis)" <regressions@...mhuis.info>, linux-arm-msm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, 
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH v2 2/2] firmware: qcom: scm: fall back to kcalloc() for no
 SCM device bound

On Mon, Sep 16, 2024 at 6:44 PM Stephan Gerhold
<stephan.gerhold@...aro.org> wrote:
>
> On Wed, Sep 11, 2024 at 11:07:04AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> >
> > Older platforms don't have an actual SCM device tied into the driver
> > model and so there's no struct device which to use with the TZ Mem API.
> > We need to fall-back to kcalloc() when allocating the buffer for
> > additional SMC arguments on such platforms which don't even probe the SCM
> > driver and never create the TZMem pool.
> >
> > Fixes: 449d0d84bcd8 ("firmware: qcom: scm: smc: switch to using the SCM allocator")
> > Reported-by: Rudraksha Gupta <guptarud@...il.com>
> > Closes: https://lore.kernel.org/lkml/692cfe9a-8c05-4ce4-813e-82b3f310019a@gmail.com/
> > Tested-by: Rudraksha Gupta <guptarud@...il.com>
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> > ---
> >  drivers/firmware/qcom/qcom_scm-smc.c | 28 ++++++++++++++++++++++++----
> >  1 file changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/firmware/qcom/qcom_scm-smc.c b/drivers/firmware/qcom/qcom_scm-smc.c
> > index 2b4c2826f572..88652c38c9a0 100644
> > --- a/drivers/firmware/qcom/qcom_scm-smc.c
> > +++ b/drivers/firmware/qcom/qcom_scm-smc.c
> > [...]
> > @@ -173,9 +182,20 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
> >               smc.args[i + SCM_SMC_FIRST_REG_IDX] = desc->args[i];
> >
> >       if (unlikely(arglen > SCM_SMC_N_REG_ARGS)) {
> > -             args_virt = qcom_tzmem_alloc(mempool,
> > -                                          SCM_SMC_N_EXT_ARGS * sizeof(u64),
> > -                                          flag);
> > +             /*
> > +              * Older platforms don't have an entry for SCM in device-tree
> > +              * and so no device is bound to the SCM driver. This means there
> > +              * is no struct device for the TZ Mem API. Fall back to
> > +              * kcalloc() on such platforms.
> > +              */
> > +             if (mempool)
> > +                     args_virt = qcom_tzmem_alloc(
> > +                                     mempool,
> > +                                     SCM_SMC_N_EXT_ARGS * sizeof(u64),
> > +                                     flag);
> > +             else
> > +                     args_virt = kcalloc(SCM_SMC_N_EXT_ARGS, sizeof(u64),
> > +                                         flag);
>
> I'm afraid this won't work. For kcalloc, we would need to flush the
> cache since it returns cached memory. In v6.10 this was done using the
> dma_map_single() call that you removed when moving to the tzmem
> allocator.
>

Indeed, I missed this but it's not very hard to re-add here.

> Actually, taking only the first patch in this series should be enough to
> fix the crash Rudraksha reported. None of the older platforms should
> ever reach into this if statement. I think the rough story is:
>
>  1. The crash Rudraksha reported happens in qcom_scm_set_cold_boot_addr()
>     during SMP CPU core boot-up. That code runs very early, AFAIK even
>     before the device model is initialized. There is no way to get
>     a device pointer at that point. Even if you add the scm node to DT.
>
>  2. AFAIK all the ARM32 platforms without PSCI support implement the
>     legacy calling convention (see qcom_scm-legacy.c). They will only
>     reach qcom_scm-smc.c once during convention detection (see
>     __get_convention()). This is a SCM call with just a single argument
>     that won't go inside the if (unlikely(arglen > SCM_SMC_N_REG_ARGS)).
>     And qcom_scm-legacy.c does not use the tzmem allocator (yet?).
>

No and I didn't plan to add it. Let me know if I should?

>  3. qcom_scm-legacy.c does use the device pointer for dma_map_single(),
>     so it already needs a scm node in the DT. I suspect MSM8960 does not
>     hit an error there only because it does not have enough functionality
>     enabled to actually reach a non-atomic SCM call. This means: Whoever
>     adds that functionality should also add the scm node in the DT.
>
> It would be good to add explicit checks for the device pointer where
> needed, instead of crashing. But other than that I think we should be
> good with just the first patch of this series?
>

Makes sense to me and with the Tested tag from Rudraksha I guess we
can drop this one.

Bart