Re:[PATCH v7] cpufreq/pasemi: fix an use-after-freeinpas_cpufreq_cpu_init()
>>> Hello Wen, >>> >>> Thanks for your patch! >>> >>> Did you test your patch with a P.A. Semi board? >>> >> Hello Christian, thank you. >> We don't have a P.A. Semi board yet, so we didn't test it. >> If you have such a board, could you please kindly help to test it? >> >> -- >> Thanks and regards, >> Wen > > Hello Wen, > > I successfully tested your pasemi cpufreq modifications with my P.A. > Semi board [1] today. > > First I patched the latest Git kernel with Viresh Kumar's patch [2]. > After that I was able to patch the latest Git kernel with your v7 patch [3]. > > Then the kernel compiled without any errors. > > Afterwards I successfully tested the new Git kernel with some cpufreq > governors on openSUSE Tumbleweed 20190521 PowerPC64 [4] and on ubuntu > MATE 16.04.6 LTS PowerPC32. > > Thanks a lot for your work! > > Tested-by: Christian Zigotzky > > Cheers, > Christian Thank you very much! -- Cheers, Wen > [1] https://en.wikipedia.org/wiki/AmigaOne_X1000 > [2] > https://lore.kernel.org/lkml/ee8cf5fb4b4a01fdf9199037ff6d835b935cfd13.1562902877.git.viresh.ku...@linaro.org/#Z30drivers:cpufreq:pasemi-cpufreq.c > [3] https://lists.ozlabs.org/pipermail/linuxppc-dev/2019-July/193735.html > [4] Screenshots: > https://i.pinimg.com/originals/37/66/93/37669306cbc909a9d79270a849d18aa6.png > and > https://i.pinimg.com/originals/fe/f8/bf/fef8bfc90d95b5ae9cf31e175e8ba2da.png
Re: [PATCH v6] cpufreq/pasemi: fix an use-after-free inpas_cpufreq_cpu_init()
> > The cpu variable is still being used in the of_get_property() call
> > after the of_node_put() call, which may result in use-after-free.
> >
> > Fixes: a9acc26b75f6 ("cpufreq/pasemi: fix possible object reference leak")
> > Signed-off-by: Wen Yang
> > Cc: "Rafael J. Wysocki"
> > Cc: Viresh Kumar
> > Cc: Michael Ellerman
> > Cc: linuxppc-dev@lists.ozlabs.org
> > Cc: linux...@vger.kernel.org
> > Cc: linux-ker...@vger.kernel.org
> > ---
> > v6: keep the blank line and fix warning: label 'out_unmap_sdcpwr' defined
> > but not used.
> > v5: put together the code to get, use, and release cpu device_node.
> > v4: restore the blank line.
> > v3: fix a leaked reference.
> > v2: clean up the code according to the advice of viresh.
> >
> > drivers/cpufreq/pasemi-cpufreq.c | 26 ++
> > 1 file changed, 14 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/cpufreq/pasemi-cpufreq.c
> > b/drivers/cpufreq/pasemi-cpufreq.c
> > index 6b1e4ab..7d557f9 100644
> > --- a/drivers/cpufreq/pasemi-cpufreq.c
> > +++ b/drivers/cpufreq/pasemi-cpufreq.c
> > @@ -131,10 +131,18 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy
> > *policy)
> > int err = -ENODEV;
> >
> > cpu = of_get_cpu_node(policy->cpu, NULL);
> > +if (!cpu)
> > +goto out;
> >
> > +max_freqp = of_get_property(cpu, "clock-frequency", NULL);
> > of_node_put(cpu);
> > -if (!cpu)
> > +if (!max_freqp) {
> > +err = -EINVAL;
> > goto out;
> > +}
> > +
> > +/* we need the freq in kHz */
> > +max_freq = *max_freqp / 1000;
> >
> > dn = of_find_compatible_node(NULL, NULL, "1682m-sdc");
> > if (!dn)
> > @@ -171,16 +179,6 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy
> > *policy)
> > }
> >
> > pr_debug("init cpufreq on CPU %d\n", policy->cpu);
> > -
> > -max_freqp = of_get_property(cpu, "clock-frequency", NULL);
> > -if (!max_freqp) {
> > -err = -EINVAL;
> > -goto out_unmap_sdcpwr;
> > -}
> > -
> > -/* we need the freq in kHz */
> > -max_freq = *max_freqp / 1000;
> > -
> > pr_debug("max clock-frequency is at %u kHz\n", max_freq);
> > pr_debug("initializing frequency table\n");
> >
> > @@ -196,7 +194,11 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy
> > *policy)
> > policy->cur = pas_freqs[cur_astate].frequency;
> > ppc_proc_freq = policy->cur * 1000ul;
> >
> > -return cpufreq_generic_init(policy, pas_freqs, get_gizmo_latency());
> > +err = cpufreq_generic_init(policy, pas_freqs, get_gizmo_latency());
>
> So you are trying to fix an earlier issue here with this. Should have
> been a separate patch. Over that I have just sent a patch now to make
> this routine return void.
>
> https://lore.kernel.org/lkml/ee8cf5fb4b4a01fdf9199037ff6d835b935cfd13.1562902877.git.viresh.ku...@linaro.org/
>
> So all you need to do is to remove the label out_unmap_sdcpwr instead.
Okay thank you.
Now this patch
(https://lore.kernel.org/lkml/ee8cf5fb4b4a01fdf9199037ff6d835b935cfd13.1562902877.git.viresh.ku...@linaro.org/)
seems to have not been merged into the linux-next.
In order to avoid code conflicts, we will wait until this patch is merged in
and then send v7.
--
Thanks and regards,
Wen
> > +if (err)
> > +goto out_unmap_sdcpwr;
> > +
> > +return 0;
> >
> > out_unmap_sdcpwr:
> > iounmap(sdcpwr_mapbase);
> > --
> > 2.9.5
Re: [PATCH v6] cpufreq/pasemi: fix an use-after-free inpas_cpufreq_cpu_init()
> > The cpu variable is still being used in the of_get_property() call
> > after the of_node_put() call, which may result in use-after-free.
> >
> > Fixes: a9acc26b75f6 ("cpufreq/pasemi: fix possible object reference leak")
> > Signed-off-by: Wen Yang
> > Cc: "Rafael J. Wysocki"
> > Cc: Viresh Kumar
> > Cc: Michael Ellerman
> > Cc: linuxppc-dev@lists.ozlabs.org
> > Cc: linux...@vger.kernel.org
> > Cc: linux-ker...@vger.kernel.org
> > ---
> > v6: keep the blank line and fix warning: label 'out_unmap_sdcpwr' defined
> > but not used.
> > v5: put together the code to get, use, and release cpu device_node.
> > v4: restore the blank line.
> > v3: fix a leaked reference.
> > v2: clean up the code according to the advice of viresh.
> >
> > drivers/cpufreq/pasemi-cpufreq.c | 26 ++
> > 1 file changed, 14 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/cpufreq/pasemi-cpufreq.c
> > b/drivers/cpufreq/pasemi-cpufreq.c
> > index 6b1e4ab..7d557f9 100644
> > --- a/drivers/cpufreq/pasemi-cpufreq.c
> > +++ b/drivers/cpufreq/pasemi-cpufreq.c
> > @@ -131,10 +131,18 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy
> > *policy)
> > int err = -ENODEV;
> >
> > cpu = of_get_cpu_node(policy->cpu, NULL);
> > +if (!cpu)
> > +goto out;
> >
> > +max_freqp = of_get_property(cpu, "clock-frequency", NULL);
> > of_node_put(cpu);
> > -if (!cpu)
> > +if (!max_freqp) {
> > +err = -EINVAL;
> > goto out;
> > +}
> > +
> > +/* we need the freq in kHz */
> > +max_freq = *max_freqp / 1000;
> >
> > dn = of_find_compatible_node(NULL, NULL, "1682m-sdc");
> > if (!dn)
> > @@ -171,16 +179,6 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy
> > *policy)
> > }
> >
> > pr_debug("init cpufreq on CPU %d\n", policy->cpu);
> > -
> > -max_freqp = of_get_property(cpu, "clock-frequency", NULL);
> > -if (!max_freqp) {
> > -err = -EINVAL;
> > -goto out_unmap_sdcpwr;
> > -}
> > -
> > -/* we need the freq in kHz */
> > -max_freq = *max_freqp / 1000;
> > -
> > pr_debug("max clock-frequency is at %u kHz\n", max_freq);
> > pr_debug("initializing frequency table\n");
> >
> > @@ -196,7 +194,11 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy
> > *policy)
> > policy->cur = pas_freqs[cur_astate].frequency;
> > ppc_proc_freq = policy->cur * 1000ul;
> >
> > -return cpufreq_generic_init(policy, pas_freqs, get_gizmo_latency());
> > +err = cpufreq_generic_init(policy, pas_freqs, get_gizmo_latency());
>
> So you are trying to fix an earlier issue here with this. Should have
> been a separate patch. Over that I have just sent a patch now to make
> this routine return void.
>
> https://lore.kernel.org/lkml/ee8cf5fb4b4a01fdf9199037ff6d835b935cfd13.1562902877.git.viresh.ku...@linaro.org/
>
> So all you need to do is to remove the label out_unmap_sdcpwr instead.
Ok, thanks.
--
Cheers,
Wen
Re: Coccinelle: Checking of_node_put() calls with SmPL
> > we developed a coccinelle script to detect such problems.
>
> Would you find the implementation of the function “dt_init_idle_driver”
> suspicious according to discussed source code search patterns?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/cpuidle/dt_idle_states.c?id=e9a83bd2322035ed9d7dcf35753d3f984d76c6a5#n208
> https://elixir.bootlin.com/linux/v5.2/source/drivers/cpuidle/dt_idle_states.c#L208
>
>
> > This script is still being improved.
>
> Will corresponding software development challenges become more interesting?
Hello Markus,
This is the simplified code pattern for it:
172 for (i = 0; ; i++) {
173 state_node = of_parse_phandle(...); ---> Obtain here
...
177 match_id = of_match_node(matches, state_node);
178 if (!match_id) {
179 err = -ENODEV;
180 break; ---> Jump out of
the loop without releasing it
181 }
182
183 if (!of_device_is_available(state_node)) {
184 of_node_put(state_node);
185 continue;---> Release the
object references within a loop
186 }
...
208 of_node_put(state_node); --> Release the object
references within a loop
209 }
210
211 of_node_put(state_node); -->There may be double free here.
This code pattern is very interesting and the coccinelle software should also
recognize this pattern.
Regards,
Wen
Re: [1/2] powerpc/83xx: fix use-after-free in mpc831x_usb_cfg()
> > The immr_node variable is still being used after the of_node_put() call, > > which may result in use-after-free. > > Was any known source code analysis tool involved to point such > a questionable implementation detail out for further software > development considerations? Hi Markus, we developed a coccinelle script to detect such problems. This script is still being improved. After a period of testing, we will send it to the LMKL mailing list later. -- Regards, Wen
Re: powerpc/83xx: fix use-after-free on mpc831x_usb_cfg()
> > The np variable is still being used after the of_node_put() call, > > > which may result in use-after-free. > > > We fix this issue by calling of_node_put() after the last usage. > > > I imagine that this commit description can be improved a bit more > (by mentioning the influence of “immr_node”?). > How do you think about to omit the word “We” here? > > > > This patatch also do some cleanup. > > > Should the renaming of a jump label be contributed in a separate update > step of a small patch series besides a wording without a typo? Thank you for your comments. We will improve the commit description and send v2 as soon as possible. -- Regards, Wen
Re:[PATCH v3] cpufreq/pasemi: fix an use-after-free inpas_cpufreq_cpu_init()
> Hello Wen, > > Thanks for your patch! > > Did you test your patch with a P.A. Semi board? > Hello Christian, thank you. We don't have a P.A. Semi board yet, so we didn't test it. If you have such a board, could you please kindly help to test it? -- Thanks and regards, Wen
Re: [PATCH v2] cpufreq/pasemi: fix an use-after-free inpas_cpufreq_cpu_init()
> > The cpu variable is still being used in the of_get_property() call
> > after the of_node_put() call, which may result in use-after-free.
> >
> > Fixes: a9acc26b75f ("cpufreq/pasemi: fix possible object reference leak")
> > Signed-off-by: Wen Yang
> > Cc: "Rafael J. Wysocki"
> > Cc: Viresh Kumar
> > Cc: linuxppc-dev@lists.ozlabs.org
> > Cc: linux...@vger.kernel.org
> > Cc: linux-ker...@vger.kernel.org
> > ---
> > v2: clean up the code according to the advice of viresh.
> >
> > drivers/cpufreq/pasemi-cpufreq.c | 10 +-
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/cpufreq/pasemi-cpufreq.c
> > b/drivers/cpufreq/pasemi-cpufreq.c
> > index 6b1e4ab..c6d464b 100644
> > --- a/drivers/cpufreq/pasemi-cpufreq.c
> > +++ b/drivers/cpufreq/pasemi-cpufreq.c
> > @@ -128,20 +128,18 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy
> > *policy)
> > int cur_astate, idx;
> > struct resource res;
> > struct device_node *cpu, *dn;
> > -int err = -ENODEV;
> > +int err;
> >
> > cpu = of_get_cpu_node(policy->cpu, NULL);
> > -
> > -of_node_put(cpu);
> > if (!cpu)
> > -goto out;
> > +return -ENODEV;
> >
>
>
> > dn = of_find_compatible_node(NULL, NULL, "1682m-sdc");
> > if (!dn)
> > dn = of_find_compatible_node(NULL, NULL,
> > "pasemi,pwrficient-sdc");
> > if (!dn)
> > -goto out;
> > +return -ENODEV;
>
> This change looks incorrect. You still need to drop reference to cpu ?
>
Thanks!
We will fix it immediately.
--
Thanks and regards,
Wen
Re: [PATCH] cpufreq/pasemi: fix an use-after-free inpas_cpufreq_cpu_init()
> > The cpu variable is still being used in the of_get_property() call
> > after the of_node_put() call, which may result in use-after-free.
> >
> > Fixes: a9acc26b75f ("cpufreq/pasemi: fix possible object reference leak")
> > Signed-off-by: Wen Yang
> > Cc: "Rafael J. Wysocki"
> > Cc: Viresh Kumar
> > Cc: linuxppc-dev@lists.ozlabs.org
> > Cc: linux...@vger.kernel.org
> > Cc: linux-ker...@vger.kernel.org
> > ---
> > drivers/cpufreq/pasemi-cpufreq.c | 10 ++
> > 1 file changed, 6 insertions(+), 4 deletions(-)
>
> I will suggest some changes here.
Hello viresh, thank you for your comments.
We will make changes as soon as possible.
--
Thanks and regards,
Wen
答复: Re: [PATCH v2] powerpc/8xx: fix possible object reference leak
>> The call to of_find_compatible_node returns a node pointer with refcount
>> incremented thus it must be explicitly decremented after the last
>> usage.
>> irq_domain_add_linear also calls of_node_get to increase refcount,
>> so irq_domain will not be affected when it is released.
>>
>> Detected by coccinelle with the following warnings:
>> ./arch/powerpc/platforms/8xx/pic.c:158:1-7: ERROR: missing of_node_put;
>> acquired a node pointer with refcount incremented on line 136, but without a
>> corresponding object release within this function.
>>
>> Fixes: a8db8cf0d894 ("irq_domain: Replace irq_alloc_host() with
>> revmap-specific initializers")
>> Signed-off-by: Wen Yang
>> Suggested-by: Christophe Leroy
>> Reviewed-by: Peng Hao
>> Cc: Vitaly Bordug
>> Cc: Benjamin Herrenschmidt
>> Cc: Paul Mackerras
>> Cc: Michael Ellerman
>> Cc: linuxppc-dev@lists.ozlabs.org
>> Cc: linux-ker...@vger.kernel.org
>> ---
>> arch/powerpc/platforms/8xx/pic.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/8xx/pic.c
>> b/arch/powerpc/platforms/8xx/pic.c
>> index 8d5a25d..4453df6 100644
>> --- a/arch/powerpc/platforms/8xx/pic.c
>> +++ b/arch/powerpc/platforms/8xx/pic.c
>> @@ -153,9 +153,7 @@ int mpc8xx_pic_init(void)
>> if (mpc8xx_pic_host == NULL) {
>> printk(KERN_ERR "MPC8xx PIC: failed to allocate irq host!\n");
>> ret = -ENOMEM;
>> -goto out;
>> }
>> -return 0;
>
> It's fragile to rely on ret being equal to zero here. If the code
> further up is changed it's easy enough to miss that ret is expected to
> be zero.
>
> Better to set it to zero here explicitly, this is the success path after
> all, eg:
>
> ret = 0;
>
Thank you for your comments.
I'll fix it soon.
Thanks and regards,
Wen
Re: Re: [PATCH 5/5] powerpc/8xx: fix possible object reference leak
Hi, Christophe,
>> The call to of_find_compatible_node returns a node pointer with refcount
>> incremented thus it must be explicitly decremented after the last
>> usage.
>> irq_domain_add_linear also calls of_node_get to increase refcount,
>> so irq_domain will not be affected when it is released.
>
>
> Should you have a:
>
> Fixes: a8db8cf0d894 ("irq_domain: Replace irq_alloc_host() with
> revmap-specific initializers")
>
> If not, it means your change is in contradiction with commit
> b1725c9319aa ("[POWERPC] arch/powerpc/sysdev: Add missing of_node_put")
Thank you very much.
This problem existed before this commit (a8db8cf0d894).
and the SmPL we used is:
https://lkml.org/lkml/2019/3/14/880
We are still improving this SmPL, and it is somewhat different from the script
used by b1725c9319aa ("[POWERPC] arch/powerpc/sysdev: Add missing of_node_put").
>> Detected by coccinelle with the following warnings:
>> ./arch/powerpc/platforms/8xx/pic.c:158:1-7: ERROR: missing of_node_put;
>> acquired a node pointer with refcount incremented on line 136, but without a
>> corresponding object release within this function.
>>
>> arch/powerpc/platforms/8xx/pic.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/powerpc/platforms/8xx/pic.c
>> b/arch/powerpc/platforms/8xx/pic.c
>> index 8d5a25d..13d880b 100644
>> --- a/arch/powerpc/platforms/8xx/pic.c
>> +++ b/arch/powerpc/platforms/8xx/pic.c
>> @@ -155,6 +155,7 @@ int mpc8xx_pic_init(void)
>> ret = -ENOMEM;
>> goto out;
>> }
>> +of_node_put(np);
>> return 0;
>>
>> out:
>>
>
> I guess it would be better as follows:
>
> --- a/arch/powerpc/platforms/8xx/pic.c
> +++ b/arch/powerpc/platforms/8xx/pic.c
> @@ -153,9 +153,7 @@ int mpc8xx_pic_init(void)
> if (mpc8xx_pic_host == NULL) {
> printk(KERN_ERR "MPC8xx PIC: failed to allocate irq
> host!\n");
> ret = -ENOMEM;
> - goto out;
> }
> - return 0;
>
> out:
> of_node_put(np);
OK.
Thank you.
We will fix it soon.
Thanks and regards,
Wen
Re: [PATCH v3] soc/fsl/qe: fix err handling of ucc_of_parse_tdm
Hi David，
Thank you, we'll fix it soon.
Best wishes,
Wen
--Original Mail--
Sender: DavidMiller
To: peng hao10096742;
CC: qiang.z...@nxp.com leoyang...@nxp.com
linux-ker...@vger.kernel.org
wen yang10156314;julia.law...@lip6.fr
net...@vger.kernel.org
linuxppc-dev@lists.ozlabs.org
Date: 2018/12/29 13:11
Subject: Re: [PATCH v3] soc/fsl/qe: fix err handling of ucc_of_parse_tdm
From: Peng Hao
Date: Wed, 26 Dec 2018 16:26:29 +0800
> diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
> index 839fa77..8ce4921f 100644
> --- a/drivers/net/wan/fsl_ucc_hdlc.c
> +++ b/drivers/net/wan/fsl_ucc_hdlc.c
> @@ -1057,6 +1057,27 @@ static const struct net_device_ops uhdlc_ops = {
> .ndo_tx_timeout= uhdlc_tx_timeout,
> };
>
> +static int ucc_get_resource_by_nodename(char *name, struct resource *res)
> +{
...
> +res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +return 0;
This assignment to 'res' doesn't do what you think it does. The caller never
sees the value you compute.
Re:RE: [PATCH] soc/fsl/qe: fix err handling of ucc_of_parse_tdm
Hi Qiang,
Thank you, we'll send a new version to fix this.
Best regards,
Wen
--Original Mail--
Sender: QiangZhao
To: wang yi10129963;
CC: zhong weidong10001088;lkml
julia.law...@lip6.fr
linuxppc-dev wen
yang10156314;moderatedlist:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
Leo Li
Date: 2018/12/25 09:58
Subject: RE: [PATCH] soc/fsl/qe: fix err handling of ucc_of_parse_tdm
Hi Wen,
Will you send another version to resolve the issue described in the comments?
BR
Qiang
> -Original Message-
> From: Li Yang
> Sent: 2018年12月6日 4:10
> To: wang.y...@zte.com.cn
> Cc: Qiang Zhao ; zhong.weid...@zte.com.cn; lkml
> ; julia.law...@lip6.fr; linuxppc-dev
> ; wen.yan...@zte.com.cn; moderated
> list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
>
> Subject: Re: [PATCH] soc/fsl/qe: fix err handling of ucc_of_parse_tdm
>
> On Thu, Nov 22, 2018 at 2:42 PM Yi Wang wrote:
> >
> > From: Wen Yang
> >
> > Currently there are 2 problems with the ucc_of_parse_tdm function:
> > 1,a possible null pointer dereference in ucc_of_parse_tdm, detected by
> > the semantic patch deref_null.cocci, with the following warning:
> > drivers/soc/fsl/qe/qe_tdm.c:177:21-24: ERROR: pdev is NULL but
> dereferenced.
> > 2,dev gets modified, so in any case that devm_iounmap() will fail even
> > when the new pdev is valid, because the iomap was done with a different
> pdev.
> > This patch fixes them.
>
> While we are at this, I think this logic need more serious fixing. I see
> there is
> no driver bind with the "fsl,t1040-qe-si" or "fsl,t1040-qe-siram" device. So
> allocating resources using devm_*() with these devices won't provide a
> cleanup path for these resources when the caller fails. I think we should
> probably allocate resource under device of caller (e.g. ucc-hdlc), so that
> when
> caller probe fails or is removed it will trigger the cleanup.
>
> >
> > Suggested-by: Christophe LEROY
> > Signed-off-by: Wen Yang
> > CC: Julia Lawall
> > CC: Zhao Qiang
> > ---
> > drivers/soc/fsl/qe/qe_tdm.c | 20 ++--
> > 1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/soc/fsl/qe/qe_tdm.c b/drivers/soc/fsl/qe/qe_tdm.c
> > index f78c346..9a29f0b 100644
> > --- a/drivers/soc/fsl/qe/qe_tdm.c
> > +++ b/drivers/soc/fsl/qe/qe_tdm.c
> > @@ -47,7 +47,7 @@ int ucc_of_parse_tdm(struct device_node *np, struct
> ucc_tdm *utdm,
> > struct resource *res;
> > struct device_node *np2;
> > static int siram_init_flag;
> > - struct platform_device *pdev;
> > + struct platform_device *pdev_si, *pdev_siram;
> >
> > sprop = of_get_property(np, "fsl,rx-sync-clock", NULL);
> > if (sprop) {
> > @@ -129,16 +129,16 @@ int ucc_of_parse_tdm(struct device_node *np,
> struct ucc_tdm *utdm,
> > if (!np2)
> > return -EINVAL;
> >
> > - pdev = of_find_device_by_node(np2);
> > - if (!pdev) {
> > + pdev_si = of_find_device_by_node(np2);
> > + if (!pdev_si) {
> > pr_err("%pOFn: failed to lookup pdev\n", np2);
> > of_node_put(np2);
> > return -EINVAL;
> > }
> >
> > of_node_put(np2);
> > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > - utdm->si_regs = devm_ioremap_resource(>dev, res);
> > + res = platform_get_resource(pdev_si, IORESOURCE_MEM, 0);
> > + utdm->si_regs = devm_ioremap_resource(_si->dev, res);
> > if (IS_ERR(utdm->si_regs)) {
> > ret = PTR_ERR(utdm->si_regs);
> > goto err_miss_siram_property; @@ -150,8 +150,8 @@
> int
> > ucc_of_parse_tdm(struct device_node *np, struct ucc_tdm *utdm,
> > goto err_miss_siram_property;
> > }
> >
> > - pdev = of_find_device_by_node(np2);
> > - if (!pdev) {
> > + pdev_siram = of_find_device_by_node(np2);
> > + if (!pdev_siram) {
> > ret = -EINVAL;
> > pr_err("%pOFn: failed to lookup pdev\n", np2);
> > of_node_put(np2);
> > @@ -159,8 +159,8 @@ int ucc_of_parse_tdm(struct device_node *np, struct
> ucc_tdm *utdm,
> > }
> >
> > of_node_put(np2);
> > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > - utdm->siram = devm_ioremap_resource(>dev, res);
> > + res = platform_get_resource(pdev_siram, IORESOURCE_MEM, 0);
> > + utdm->siram = devm_ioremap_resource(_siram->dev, res);
> > if (IS_ERR(utdm->siram)) {
> > ret = PTR_ERR(utdm->siram);
> > goto err_miss_siram_property; @@ -174,7 +174,7 @@
> int
> > ucc_of_parse_tdm(struct device_node *np, struct ucc_tdm *utdm,
> > return ret;
> >
> > err_miss_siram_property:
> > - devm_iounmap(>dev, utdm->si_regs);
> > + devm_iounmap(_si->dev, utdm->si_regs);
> > return ret;
> > }
> > EXPORT_SYMBOL(ucc_of_parse_tdm);
> > --
> > 2.9.5
> >
答复: Re: [PATCH 2/4] soc/fsl/qe: fix potential NULL pointer dereference inucc_of_parse_tdm
Hi Christophe, thank you for your review.
There are two problems in the ucc_of_parse_tdm function.
1, NULL pointer reference
2, pdev gets modified, iomap was done with a different pdev.
We will submit a patch to repair it later.
Thanks.
132 pdev = of_find_device_by_node(np2);
..
141 utdm->si_regs = devm_ioremap_resource(>dev, res); ---> use
pdev to call devm_ioremap
..
153 pdev = of_find_device_by_node(np2); --->This line, pdev gets
modified
154 if (!pdev) {
155 ret = -EINVAL;
156 pr_err("%pOFn: failed to lookup pdev\n", np2);
157 of_node_put(np2);
158 goto err_miss_siram_property;
--> This line, if pdev is NULL, will jump to label err_miss_siram_property
159 }
..
163 utdm->siram = devm_ioremap_resource(>dev, res); --> use
changed pdev to call devm_ioremap
..
175
176 err_miss_siram_property:
177 devm_iounmap(>dev, utdm->si_regs);--> Within the code
block, if pdev is NULL, kernel panic will be triggered; devm_iounmap() will
alse fail even when the new pdev is valid.
178 return ret;
179 }
--原始邮件--
发件人：ChristopheLEROY
收件人：文洋10156314;qiang.z...@nxp.com
抄送人：汪翼10129963;钟卫东10001088;linux-ker...@vger.kernel.org
leoyang...@nxp.com Julia
Lawall linuxppc-dev@lists.ozlabs.org
linux-arm-ker...@lists.infradead.org
日 期 ：2018年11月21日 18:46
主 题 ：Re: [PATCH 2/4] soc/fsl/qe: fix potential NULL pointer dereference
inucc_of_parse_tdm
Le 21/11/2018 à 08:41, Wen Yang a écrit :
> This patch fixes a possible null pointer dereference in
> ucc_of_parse_tdm, detected by the semantic patch
> deref_null.cocci, with the following warning:
>
> ./drivers/soc/fsl/qe/qe_tdm.c:177:21-24: ERROR: pdev is NULL but dereferenced.
>
> The following code has potential null pointer references:
> pdev = of_find_device_by_node(np2);
> if (!pdev) {
> ret = -EINVAL;
> pr_err("%pOFn: failed to lookup pdev\n", np2);
> of_node_put(np2);
> goto err_miss_siram_property;
> }
> ...
> err_miss_siram_property:
> devm_iounmap(>dev, utdm->si_regs);
>
> Signed-off-by: Wen Yang
> Reviewed-by: Tan Hu
> CC: Julia Lawall
> ---
> drivers/soc/fsl/qe/qe_tdm.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soc/fsl/qe/qe_tdm.c b/drivers/soc/fsl/qe/qe_tdm.c
> index f78c346..166378b 100644
> --- a/drivers/soc/fsl/qe/qe_tdm.c
> +++ b/drivers/soc/fsl/qe/qe_tdm.c
> @@ -174,7 +174,8 @@ int ucc_of_parse_tdm(struct device_node *np, struct
> ucc_tdm *utdm,
> return ret;
>
> err_miss_siram_property:
> -devm_iounmap(>dev, utdm->si_regs);
> +if (pdev)
> +devm_iounmap(>dev, utdm->si_regs);
You are just hiding the issue, not fixing it.
The problem is that pdev gets modified, so in any case that
devm_iounmap() will fail even when the new pdev is valid, because the
iomap was done with a different pdev.
Christophe
> return ret;
> }
> EXPORT_SYMBOL(ucc_of_parse_tdm);
>
