diff mbox series

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

Message ID 1562670768-23178-2-git-send-email-wen.yang99@zte.com.cn
State New
Headers show
Series [1/2] powerpc/83xx: fix use-after-free in mpc831x_usb_cfg() | expand

Commit Message

Wen Yang July 9, 2019, 11:12 a.m. UTC
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(-)

Comments

Markus Elfring July 10, 2019, 7:19 a.m. UTC | #1
> 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
Wen Yang July 10, 2019, 7:33 a.m. UTC | #2
> > 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
Markus Elfring July 10, 2019, 9:24 a.m. UTC | #3
> 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
Markus Elfring July 10, 2019, 3:15 p.m. UTC | #4
> 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
Wen Yang July 11, 2019, 6:35 a.m. UTC | #5
> > 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
Julia Lawall July 11, 2019, 6:46 a.m. UTC | #6
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
Markus Elfring July 11, 2019, 9:04 a.m. UTC | #7
> 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
Markus Elfring July 11, 2019, 9:33 a.m. UTC | #8
> 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
Tyrel Datwyler July 11, 2019, 2:41 p.m. UTC | #9
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
>
diff mbox series

Patch

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;
 }