Re: [PATCH] soc: fsl: qe: Fix potential NULL pointer dereference inqe_reset()

[未君 <1742789905@qq.com>]

[-- Attachment #1: Type: text/plain, Size: 5244 bytes --]

Hi,


Thank you for the detailed review. You are completely right.


My commit message was confusing, and returning early in qe_reset() just shifts the NULL pointer dereference to the dependent drivers later on, without actually fixing the root cause.


To achieve what you suggested ("if qe_immr remap fails, all drivers depending on it don't get probed"), I plan to do the following in the v2 patch:


1. Change the return type of qe_reset() from `void` to `int`.
2. Return `-ENOMEM` if the ioremap() fails.
3. Update the callers of qe_reset() (e.g., qe_probe() and other board-specific setup functions) to check this return value. If qe_reset() fails, the callers will abort their initialization/probing, which will properly prevent the child devices from being probed.


Does this approach sound correct to you? If so, I will prepare and submit the v2 patch accordingly.


Best regards,
Wang Jun


未君
1742789905@qq.com



        



         Original
         
       
From: Christophe Leroy (CS GROUP) <chleroy@kernel.org&gt;
Date: 2026-03-13 17:48
To: Wang Jun <1742789905@qq.com&gt;, Qiang Zhao <qiang.zhao@nxp.com&gt;, linuxppc-dev <linuxppc-dev@lists.ozlabs.org&gt;, linux-arm-kernel <linux-arm-kernel@lists.infradead.org&gt;
Cc: linux-kernel <linux-kernel@vger.kernel.org&gt;, gszhai <gszhai@bjtu.edu.cn&gt;, 25125332 <25125332@bjtu.edu.cn&gt;, 25125283 <25125283@bjtu.edu.cn&gt;, 23120469 <23120469@bjtu.edu.cn&gt;
Subject: Re: [PATCH] soc: fsl: qe: Fix potential NULL pointer dereference inqe_reset()





Le&nbsp;10/03/2026&nbsp;à&nbsp;13:11,&nbsp;Wang&nbsp;Jun&nbsp;a&nbsp;écrit&nbsp;:
&gt;&nbsp;[Vous&nbsp;ne&nbsp;recevez&nbsp;pas&nbsp;souvent&nbsp;de&nbsp;courriers&nbsp;de&nbsp;1742789905@qq.com.&nbsp;Découvrez&nbsp;pourquoi&nbsp;ceci&nbsp;est&nbsp;important&nbsp;à&nbsp;https://aka.ms/LearnAboutSenderIdentification&nbsp;]
&gt;&nbsp;
&gt;&nbsp;The&nbsp;function&nbsp;qe_reset()&nbsp;uses&nbsp;qe_immr&nbsp;without&nbsp;checking&nbsp;if&nbsp;it&nbsp;is&nbsp;NULL,
&gt;&nbsp;which&nbsp;could&nbsp;happen&nbsp;if&nbsp;ioremap()&nbsp;failed&nbsp;earlier.&nbsp;Add&nbsp;a&nbsp;NULL&nbsp;check&nbsp;and
&gt;&nbsp;perform&nbsp;ioremap()&nbsp;if&nbsp;needed;&nbsp;if&nbsp;it&nbsp;still&nbsp;fails,&nbsp;print&nbsp;an&nbsp;error&nbsp;and
&gt;&nbsp;return&nbsp;to&nbsp;avoid&nbsp;crashing&nbsp;the&nbsp;system.

I&nbsp;don't&nbsp;understand&nbsp;what&nbsp;you&nbsp;are&nbsp;trying&nbsp;to&nbsp;say&nbsp;here.&nbsp;What&nbsp;you&nbsp;say&nbsp;is&nbsp;
already&nbsp;what&nbsp;qe_reset()&nbsp;does:&nbsp;it&nbsp;does&nbsp;a&nbsp;NULL&nbsp;check&nbsp;and&nbsp;performs&nbsp;
ioremap()&nbsp;when&nbsp;it&nbsp;is&nbsp;NULL:

	if&nbsp;(qe_immr&nbsp;==&nbsp;NULL)
		qe_immr&nbsp;=&nbsp;ioremap(get_qe_base(),&nbsp;QE_IMMAP_SIZE);

You&nbsp;are&nbsp;adding&nbsp;a&nbsp;second&nbsp;NULL&nbsp;check&nbsp;and&nbsp;return&nbsp;early&nbsp;from&nbsp;qe_reset().&nbsp;But&nbsp;
it&nbsp;doesn't&nbsp;really&nbsp;fix&nbsp;the&nbsp;problem&nbsp;because&nbsp;qe_immr&nbsp;is&nbsp;used&nbsp;in&nbsp;many&nbsp;other&nbsp;
places&nbsp;so&nbsp;you&nbsp;are&nbsp;just&nbsp;delaying&nbsp;the&nbsp;problem.

What&nbsp;needs&nbsp;to&nbsp;be&nbsp;done&nbsp;is&nbsp;that&nbsp;if&nbsp;qe_immr&nbsp;remap&nbsp;fails,&nbsp;all&nbsp;drivers&nbsp;
depending&nbsp;on&nbsp;it&nbsp;don't&nbsp;get&nbsp;probed.

&gt;&nbsp;
&gt;&nbsp;Signed-off-by:&nbsp;Wang&nbsp;Jun&nbsp;<1742789905@qq.com&gt;
&gt;&nbsp;---
&gt;&nbsp;&nbsp;&nbsp;drivers/soc/fsl/qe/qe.c&nbsp;|&nbsp;7&nbsp;++++++-
&gt;&nbsp;&nbsp;&nbsp;1&nbsp;file&nbsp;changed,&nbsp;6&nbsp;insertions(+),&nbsp;1&nbsp;deletion(-)
&gt;&nbsp;
&gt;&nbsp;diff&nbsp;--git&nbsp;a/drivers/soc/fsl/qe/qe.c&nbsp;b/drivers/soc/fsl/qe/qe.c
&gt;&nbsp;index&nbsp;70b6eddb867b..6dcfa340970a&nbsp;100644
&gt;&nbsp;---&nbsp;a/drivers/soc/fsl/qe/qe.c
&gt;&nbsp;+++&nbsp;b/drivers/soc/fsl/qe/qe.c
&gt;&nbsp;@@&nbsp;-86,8&nbsp;+86,13&nbsp;@@&nbsp;static&nbsp;phys_addr_t&nbsp;get_qe_base(void)
&gt;&nbsp;
&gt;&nbsp;&nbsp;&nbsp;void&nbsp;qe_reset(void)
&gt;&nbsp;&nbsp;&nbsp;{
&gt;&nbsp;-&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;if&nbsp;(qe_immr&nbsp;==&nbsp;NULL)
&gt;&nbsp;+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;if&nbsp;(qe_immr&nbsp;==&nbsp;NULL)&nbsp;{
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;qe_immr&nbsp;=&nbsp;ioremap(get_qe_base(),&nbsp;QE_IMMAP_SIZE);
&gt;&nbsp;+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;if&nbsp;(qe_immr&nbsp;==&nbsp;NULL)&nbsp;{
&gt;&nbsp;+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;pr_err("QE:&nbsp;cannot&nbsp;remap&nbsp;IMMR\n");
&gt;&nbsp;+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;return;
&gt;&nbsp;+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;}
&gt;&nbsp;+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;}
&gt;&nbsp;
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;qe_snums_init();
&gt;&nbsp;
&gt;&nbsp;--
&gt;&nbsp;2.43.0
&gt;&nbsp;

[-- Attachment #2: Type: text/html, Size: 11091 bytes --]