linux-kernel - Re: [PATCH 1/4] crypto: caam - Prevent crash on suspend with iMX8QM / iMX8ULP

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: <aDCK00bzc6RDKu5s@lizhi-Precision-Tower-5810>
Date: Fri, 23 May 2025 10:48:51 -0400
From: Frank Li <Frank.li@....com>
To: John Ernberg <john.ernberg@...ia.se>
Cc: Horia Geantă <horia.geanta@....com>,
	Pankaj Gupta <pankaj.gupta@....com>,
	Gaurav Jain <gaurav.jain@....com>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	"David S . Miller" <davem@...emloft.net>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>,
	Thomas Richard <thomas.richard@...tlin.com>,
	"linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>,
	"stable@...nel.org" <stable@...nel.org>
Subject: Re: [PATCH 1/4] crypto: caam - Prevent crash on suspend with iMX8QM
 / iMX8ULP

On Fri, May 23, 2025 at 02:19:38PM +0000, John Ernberg wrote:
> Hi Frank,
>
> On 5/23/25 3:53 PM, Frank Li wrote:
> > On Fri, May 23, 2025 at 01:18:32PM +0000, John Ernberg wrote:
> >> Since the CAAM on these SoCs is managed by another ARM core, called the
> >> SECO (Security Controller) on iMX8QM and Secure Enclave on iMX8ULP, which
> >> also reserves access to register page 0 suspend operations cannot touch
> >> this page.
> >>
> >> Introduce a variable to track this situation. Since this is synonymous
> >> with the optee case in suspend/resume the optee check is replaced with
> >> this new check.
> >>
> >> Fixes the following splat at suspend:
> >>
> >>      Internal error: synchronous external abort: 0000000096000010 [#1] SMP
> >>      Hardware name: Freescale i.MX8QXP ACU6C (DT)
> >>      pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> >>      pc : readl+0x0/0x18
> >>      lr : rd_reg32+0x18/0x3c
> >>      sp : ffffffc08192ba20
> >>      x29: ffffffc08192ba20 x28: ffffff8025190000 x27: 0000000000000000
> >>      x26: ffffffc0808ae808 x25: ffffffc080922338 x24: ffffff8020e89090
> >>      x23: 0000000000000000 x22: ffffffc080922000 x21: ffffff8020e89010
> >>      x20: ffffffc080387ef8 x19: ffffff8020e89010 x18: 000000005d8000d5
> >>      x17: 0000000030f35963 x16: 000000008f785f3f x15: 000000003b8ef57c
> >>      x14: 00000000c418aef8 x13: 00000000f5fea526 x12: 0000000000000001
> >>      x11: 0000000000000002 x10: 0000000000000001 x9 : 0000000000000000
> >>      x8 : ffffff8025190870 x7 : ffffff8021726880 x6 : 0000000000000002
> >>      x5 : ffffff80217268f0 x4 : ffffff8021726880 x3 : ffffffc081200000
> >>      x2 : 0000000000000001 x1 : ffffff8020e89010 x0 : ffffffc081200004
> >>      Call trace:
> >>       readl+0x0/0x18
> >>       caam_ctrl_suspend+0x30/0xdc
> >>       dpm_run_callback.constprop.0+0x24/0x5c
> >>       device_suspend+0x170/0x2e8
> >>       dpm_suspend+0xa0/0x104
> >>       dpm_suspend_start+0x48/0x50
> >>       suspend_devices_and_enter+0x7c/0x45c
> >>       pm_suspend+0x148/0x160
> >>       state_store+0xb4/0xf8
> >>       kobj_attr_store+0x14/0x24
> >>       sysfs_kf_write+0x38/0x48
> >>       kernfs_fop_write_iter+0xb4/0x178
> >>       vfs_write+0x118/0x178
> >>       ksys_write+0x6c/0xd0
> >>       __arm64_sys_write+0x14/0x1c
> >>       invoke_syscall.constprop.0+0x64/0xb0
> >>       do_el0_svc+0x90/0xb0
> >>       el0_svc+0x18/0x44
> >>       el0t_64_sync_handler+0x88/0x124
> >>       el0t_64_sync+0x150/0x154
> >>      Code: 88dffc21 88dffc21 5ac00800 d65f03c0 (b9400000)
> >>
> >> Fixes: d2835701d93c ("crypto: caam - i.MX8ULP donot have CAAM page0 access")
> >> Fixes: 61bb8db6f682 ("crypto: caam - Add support for i.MX8QM")
> >> Cc: stable@...nel.org # v6.10+
> >> Signed-off-by: John Ernberg <john.ernberg@...ia.se>
> >>
> >> ---
> >>
> >> I noticed this when enabling the iMX8QXP support (next patch), hence the
> >> iMX8QXP backtrace, but the iMX8QM CAAM integration works exactly the same
> >> and according to the NXP tree [1] the iMX8ULP suffers the same issue.
> >>
> >> [1]: https://github.com/nxp-imx/linux-imx/commit/653712ffe52dd59f407af1b781ce318f3d9e17bb
> >> ---
> >>   drivers/crypto/caam/ctrl.c   | 5 +++--
> >>   drivers/crypto/caam/intern.h | 1 +
> >>   2 files changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> >> index 38ff931059b4..766c447c9cfb 100644
> >> --- a/drivers/crypto/caam/ctrl.c
> >> +++ b/drivers/crypto/caam/ctrl.c
> >> @@ -831,7 +831,7 @@ static int caam_ctrl_suspend(struct device *dev)
> >>   {
> >>        const struct caam_drv_private *ctrlpriv = dev_get_drvdata(dev);
> >>
> >> -     if (ctrlpriv->caam_off_during_pm && !ctrlpriv->optee_en)
> >> +     if (ctrlpriv->caam_off_during_pm && !ctrlpriv->no_page0)
> >>                caam_state_save(dev);
> >>
> >>        return 0;
> >> @@ -842,7 +842,7 @@ static int caam_ctrl_resume(struct device *dev)
> >>        struct caam_drv_private *ctrlpriv = dev_get_drvdata(dev);
> >>        int ret = 0;
> >>
> >> -     if (ctrlpriv->caam_off_during_pm && !ctrlpriv->optee_en) {
> >> +     if (ctrlpriv->caam_off_during_pm && !ctrlpriv->no_page0) {
> >>                caam_state_restore(dev);
> >>
> >>                /* HW and rng will be reset so deinstantiation can be removed */
> >> @@ -908,6 +908,7 @@ static int caam_probe(struct platform_device *pdev)
> >>
> >>                imx_soc_data = imx_soc_match->data;
> >>                reg_access = reg_access && imx_soc_data->page0_access;
> >> +             ctrlpriv->no_page0 = !reg_access;
> >
> > If you want to use no_page0 to control if call caam_state_save(), you'd
> > better set ctrlpriv->no_page0 also after ctrlpriv->optee_en = !!np;
> >
> > Frank
>
> I'm not sure I understand, I cannot see a code path where no_page0 will
> be (un)set incorrectly.
>
> optee disables the page0 access, so reg_access is already the inverse of
> optee_en. reg_access == false when optee_en == true.
>
> Thus, if optee is loaded on a SoC that normally has page0_access the
> `reg_access = reg_access && imx_soc_data->page0_access;` statement on
> the line above setting no_page0 already takes care of it, so:
> reg_access = false && true -> false.
>
> Similarly if both reg_access == false and page0_access == false,
> reg_access will still be false.

Okay, I check original code. You are right. You'd better to add descripton
in commit message about no_page0 is true when optee_en is true.

Frank
>
> Thanks! // John Ernberg