| Message ID | 1562670768-23178-2-git-send-email-wen.yang99@zte.com.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] powerpc/83xx: fix use-after-free in mpc831x_usb_cfg() | expand |
> 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
> > 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
> 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
> 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
> > 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
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
> 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
> 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
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 --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; }
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(-)