[PATCH 1/2] powerpc/83xx: fix use-after-free in mpc831x_usb_cfg()

[PATCH 1/2] powerpc/83xx: fix use-after-free in mpc831x_usb_cfg()

[Wen Yang]

The immr_node variable is still being used after the of_node_put() call,
which may result in use-after-free.
Fix this issue by calling of_node_put() after the last usage.

Fixes: fd066e850351 ("powerpc/mpc8308: fix USB DR controller initialization")
Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Cc: Scott Wood <oss@buserror.net>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Markus Elfring <Markus.Elfring@web.de>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-kernel@vger.kernel.org
---
 arch/powerpc/platforms/83xx/usb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/83xx/usb.c b/arch/powerpc/platforms/83xx/usb.c
index 3d247d7..19dcef5 100644
--- a/arch/powerpc/platforms/83xx/usb.c
+++ b/arch/powerpc/platforms/83xx/usb.c
@@ -158,11 +158,10 @@ int mpc831x_usb_cfg(void)
 
 	iounmap(immap);
 
-	of_node_put(immr_node);
-
 	/* Map USB SOC space */
 	ret = of_address_to_resource(np, 0, &res);
 	if (ret) {
+		of_node_put(immr_node);
 		of_node_put(np);
 		return ret;
 	}
@@ -203,6 +202,7 @@ int mpc831x_usb_cfg(void)
 
 out:
 	iounmap(usb_regs);
+	of_node_put(immr_node);
 	of_node_put(np);
 	return ret;
 }
-- 
2.9.5

Re: [1/2] powerpc/83xx: fix use-after-free in mpc831x_usb_cfg()

[Markus Elfring]

> 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?

Regards,
Markus

Re: [1/2] powerpc/83xx: fix use-after-free in mpc831x_usb_cfg()

[wen.yang99]

[-- Attachment #1.1: Type: text/plain, Size: 479 bytes --]

> > 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: [1/2] powerpc/83xx: fix use-after-free in mpc831x_usb_cfg()

[Markus Elfring]

> we developed a coccinelle script to detect such problems.

How do you think about to give any attribution to this development software
in your commit descriptions?


> After a period of testing, we will send it to the LMKL mailing list later.

I am also curious then on how this area will evolve further.

Regards,
Markus

Re: Coccinelle: Checking of_node_put() calls with SmPL

[Markus Elfring]

> 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?

Regards,
Markus

Re: Coccinelle: Checking of_node_put() calls with SmPL

[wen.yang99]

[-- Attachment #1.1: Type: text/plain, Size: 1622 bytes --]

> > 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: Coccinelle: Checking of_node_put() calls with SmPL

[Julia Lawall]

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



On Thu, 11 Jul 2019, wen.yang99@zte.com.cn wrote:

> > > 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.

In my experience, when you start looking at these of_node_put things, all
sorts of strange things appear...

julia

Re: Coccinelle: Checking of_node_put() calls with SmPL

[Markus Elfring]

> In my experience, when you start looking at these of_node_put things,
> all sorts of strange things appear...

How much will this situation influence the achievement of further improvements
also for your software?

Regards,
Markus

Re: Coccinelle: Checking of_node_put() calls with SmPL

[Markus Elfring]

> 180                         break;                         --->  Jump out of the loop without releasing it

The device node reference is released behind this for loop.


> 183                 if (!of_device_is_available(state_node)) {
> 184                         of_node_put(state_node);

This function call was added by the commit “cpuidle: dt: Add missing 'of_node_put()'”
on 2017-06-12.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/cpuidle/dt_idle_states.c?id=b2cdd8e1b54849477a32d820acc2e87828a38f3d


> 185                         continue;                    --->  Release the object references within a loop

I became curious on the applicability of an other coding style
(for a software refactoring) at this place.
How do you think about to achieve the same effect by using a goto statement
instead of two statements in such an if branch?


> 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 information points a recurring challenge out for safe source code analysis.
How would you like to exclude the detection of false positives finally?


> This code pattern is very interesting

Thanks that you think also in this direction.


> and the coccinelle software should also recognize this pattern.

There are some open issues to consider for available analysis tools.
How will corresponding details be clarified then?

Regards,
Markus

Re: Coccinelle: Checking of_node_put() calls with SmPL

[Tyrel Datwyler]

On 07/10/2019 11:35 PM, wen.yang99@zte.com.cn wrote:
>>> 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++) {

This loop can only be exited on a break.

> 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

This is required at the end of every loop or continue to free the reference.
Only a break will exit the loop where we hit the below of_node_put().

> 209         }
> 210 
> 211         of_node_put(state_node);       -->    There may be double free here.

None of the break conditions call of_node_put(), so it needs to be called here.

-Tyrel

> 
> This code pattern is very interesting and the coccinelle software should also recognize this pattern.
> 
> Regards,
> Wen
> 

[PATCH 2/2] powerpc/83xx: cleanup error paths in mpc831x_usb_cfg()

[Wen Yang]

Rename the jump labels according to the cleanup they perform,
and move reference handling to simplify cleanup.

Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Cc: Scott Wood <oss@buserror.net>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Markus Elfring <Markus.Elfring@web.de>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-kernel@vger.kernel.org
---
 arch/powerpc/platforms/83xx/usb.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/platforms/83xx/usb.c b/arch/powerpc/platforms/83xx/usb.c
index 19dcef5..56b36fa 100644
--- a/arch/powerpc/platforms/83xx/usb.c
+++ b/arch/powerpc/platforms/83xx/usb.c
@@ -160,11 +160,9 @@ int mpc831x_usb_cfg(void)
 
 	/* Map USB SOC space */
 	ret = of_address_to_resource(np, 0, &res);
-	if (ret) {
-		of_node_put(immr_node);
-		of_node_put(np);
-		return ret;
-	}
+	if (ret)
+		goto out_put_node;
+
 	usb_regs = ioremap(res.start, resource_size(&res));
 
 	/* Using on-chip PHY */
@@ -173,7 +171,7 @@ int mpc831x_usb_cfg(void)
 		u32 refsel;
 
 		if (of_device_is_compatible(immr_node, "fsl,mpc8308-immr"))
-			goto out;
+			goto out_unmap;
 
 		if (of_device_is_compatible(immr_node, "fsl,mpc8315-immr"))
 			refsel = CONTROL_REFSEL_24MHZ;
@@ -200,8 +198,9 @@ int mpc831x_usb_cfg(void)
 		ret = -EINVAL;
 	}
 
-out:
+out_unmap:
 	iounmap(usb_regs);
+out_put_node:
 	of_node_put(immr_node);
 	of_node_put(np);
 	return ret;
-- 
2.9.5

Re: [PATCH 0/2] fix use-after-free in mpc831x_usb_cfg() and do some cleanups

[Markus Elfring]

> According to Markus's suggestion, split it into two small patches:

> https://lkml.org/lkml/2019/7/8/520

Thanks that you picked adjustment possibilities up from my feedback.
https://lore.kernel.org/lkml/99840e11-e0e6-b3f4-e35b-56ef4ec39417@web.de/


Now I wonder why you omitted message recipients from the cover letter.
Please keep the address lists usually complete also here for improvements
on the same source file in subsequent patch series.


Can a subject like “[PATCH 0/2] Fix mpc831x_usb_cfg()” be more succinct?


>  powerpc/83xx: fix use-after-free in mpc831x_usb_cfg()


This update variant is generally fine.
I would prefer to avoid the addition of function calls at two places
when the corresponding exception handling should be specified only once
at the end of such a function implementation.


>  powerpc/83xx: cleanup error paths in mpc831x_usb_cfg()

I would find it clearer to fix the error handling in the first update
step completely.
I guess that a renaming of the label “out” into “out_unmap” (or “unmap_io”?)
would be an auxiliary change for the second update step.


I am curious if different preferences for change combinations will trigger
further collateral evolution.

Regards,
Markus