There are functions which increment a reference counter for a device node.
These functions belong to a programming interface for the management
of information from device trees.
The counter must be decremented after the last usage of a device node.
We find these functions by using the following script:
<SmPL>
@initialize:ocaml@
@@
let relevant_str = "use of_node_put() on it when done"
let contains s1 s2 =
let re = Str.regexp_string s2
in
try ignore (Str.search_forward re s1 0); true
with Not_found -> false
let relevant_functions = ref []
let add_function f c =
if not (List.mem f !relevant_functions)
then
begin
let s = String.concat " "
(
(List.map String.lowercase_ascii
(List.filter
(function x ->
Str.string_match
(Str.regexp "[a-zA-Z_\\(\\)][-a-zA-Z0-9_\\(\\)]*$")
x 0) (Str.split (Str.regexp "[ .;\t\n]+") c)))) in
if contains s relevant_str
then
Printf.printf "Found relevant function: %s\n" f;
relevant_functions := f :: !relevant_functions;
end
@r@
identifier fn;
comments c;
type T = struct device_node *;
@@
T@c fn(...) {
...
}
@script:ocaml@
f << r.fn;
c << r.c;
@@
let (cb,cm,ca) = List.hd c in
let c = String.concat " " cb in
add_function f c
</SmPL>
Then copy the function names found by the above script to the r_miss_put
rule. This rule checks for missing of_node_put.
And this patch also looks for places where an of_node_put() call is on some
paths but not on others (implemented by the r_miss_put_ext rule).
Finally, this patch finds use-after-free issues for a node.
(implemented by the r_use_after_put rule)
Suggested-by: Julia Lawall <[email protected]>
Signed-off-by: Wen Yang <[email protected]>
Cc: Julia Lawall <[email protected]>
Cc: Gilles Muller <[email protected]>
Cc: Nicolas Palix <[email protected]>
Cc: Michal Marek <[email protected]>
Cc: Masahiro Yamada <[email protected]>
Cc: Wen Yang <[email protected]>
Cc: Markus Elfring <[email protected]>
Cc: [email protected]
---
v3: delete the global set, add a rule that checks for use-after-free.
v2: improve the commit description and delete duplicate code.
scripts/coccinelle/free/of_node_put.cocci | 192 ++++++++++++++++++++++++++++++
1 file changed, 192 insertions(+)
create mode 100644 scripts/coccinelle/free/of_node_put.cocci
diff --git a/scripts/coccinelle/free/of_node_put.cocci b/scripts/coccinelle/free/of_node_put.cocci
new file mode 100644
index 0000000..cda43fa
--- /dev/null
+++ b/scripts/coccinelle/free/of_node_put.cocci
@@ -0,0 +1,192 @@
+// SPDX-License-Identifier: GPL-2.0
+/// Find missing of_node_put
+///
+// Confidence: Moderate
+// Copyright: (C) 2018-2019 Wen Yang, ZTE.
+// Comments:
+// Options: --no-includes --include-headers
+
+virtual report
+virtual org
+
+@initialize:python@
+@@
+
+report_miss_prefix = "ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line "
+report_miss_suffix = ", but without a corresponding object release within this function."
+org_miss_main = "acquired a node pointer with refcount incremented"
+org_miss_sec = "needed of_node_put"
+report_use_after_put = "ERROR: use-after-free; reference preceded by of_node_put on line "
+org_use_after_put_main = "of_node_put"
+org_use_after_put_sec = "reference"
+
+@r_miss_put exists@
+local idexpression struct device_node *x;
+expression e, e1;
+position p1, p2;
+statement S;
+type T, T1;
+@@
+
+* x = @p1\(of_find_all_nodes\|
+ of_get_cpu_node\|
+ of_get_parent\|
+ of_get_next_parent\|
+ of_get_next_child\|
+ of_get_next_cpu_node\|
+ of_get_compatible_child\|
+ of_get_child_by_name\|
+ of_find_node_opts_by_path\|
+ of_find_node_by_name\|
+ of_find_node_by_type\|
+ of_find_compatible_node\|
+ of_find_node_with_property\|
+ of_find_matching_node_and_match\|
+ of_find_node_by_phandle\|
+ of_parse_phandle\|
+ of_find_next_cache_node\|
+ of_get_next_available_child\)(...);
+...
+if (x == NULL || ...) S
+... when != e = (T)x
+ when != of_node_put(x)
+ when != of_get_next_parent(x)
+ when != of_find_matching_node(x, ...)
+ when != if (x) { ... return x; }
+ when != v4l2_async_notifier_add_fwnode_subdev(..., <+...x...+>, ...)
+ when != e1 = of_fwnode_handle(x)
+(
+ if (x) { ... when forall
+ of_node_put(x) ... }
+|
+ return (T1)x;
+|
+ return of_fwnode_handle(x);
+|
+* return@p2 ...;
+)
+
+@script:python depends on report && r_miss_put@
+p1 << r_miss_put.p1;
+p2 << r_miss_put.p2;
+@@
+
+coccilib.report.print_report(p2[0], report_miss_prefix + p1[0].line + report_miss_suffix)
+
+@script:python depends on org && r_miss_put@
+p1 << r_miss_put.p1;
+p2 << r_miss_put.p2;
+@@
+
+cocci.print_main(org_miss_main, p1)
+cocci.print_secs(org_miss_sec, p2)
+
+@r_miss_put_ext exists@
+local idexpression struct device_node *x;
+expression e, e1;
+position p1 != r_miss_put.p1, p2 != r_miss_put.p2;
+identifier f;
+statement S;
+type T, T1;
+@@
+
+(
+* x = f@p1(...);
+... when != e = (T)x
+ when != true x == NULL
+ when != of_node_put(x)
+ when != of_get_next_parent(x)
+ when != of_find_matching_node(x, ...)
+ when != if (x) { ... return x; }
+ when != v4l2_async_notifier_add_fwnode_subdev(..., <+...x...+>, ...)
+ when != e1 = of_fwnode_handle(x)
+(
+ if (x) { ... when forall
+ of_node_put(x) ... }
+|
+ return (T1)x;
+|
+ return of_fwnode_handle(x);
+|
+* return@p2 ...;
+)
+&
+x = f(...)
+...
+if (<+...x...+>) S
+...
+of_node_put(x);
+)
+
+@script:python depends on report && r_miss_put_ext@
+p1 << r_miss_put_ext.p1;
+p2 << r_miss_put_ext.p2;
+@@
+
+coccilib.report.print_report(p2[0], report_miss_prefix + p1[0].line + report_miss_suffix)
+
+@script:python depends on org && r_miss_put_ext@
+p1 << r_miss_put_ext.p1;
+p2 << r_miss_put_ext.p2;
+@@
+cocci.print_main(org_miss_main, p1)
+cocci.print_secs(org_miss_sec, p2)
+
+@r_put@
+expression E;
+position p1;
+@@
+
+* of_node_put@p1(E)
+
+@r_use_after_put exists@
+expression r_put.E, subE<=r_put.E;
+constant char [] c;
+expression E1;
+iterator iter;
+identifier f;
+statement S;
+type T;
+position r_put.p1, p2;
+@@
+
+* of_node_put@p1(E,...)
+...
+(
+ iter(...,subE,...) S
+|
+ subE = (T)E1
+|
+ &(T)subE
+|
+ f(...,c,...,(T)E,...)
+|
+ E == (T)E1
+|
+ E != (T)E1
+|
+ E1 == (T)E
+|
+ E1 != (T)E
+|
+ !E
+|
+ (T)E || ...
+|
+* (T)E@p2
+)
+
+@script:python depends on r_use_after_put && report@
+p1 << r_put.p1;
+p2 << r_use_after_put.p2;
+@@
+
+coccilib.report.print_report(p2[0], report_use_after_put + p1[0].line)
+
+@script:python depends on r_use_after_put && org@
+p1 << r_put.p1;
+p2 << r_use_after_put.p2;
+@@
+
+cocci.print_main(org_use_after_put_main, p1)
+cocci.print_secs(org_use_after_put_sec, p2)
--
2.9.5
> We find these functions by using the following script:
Why would you like to keep this SmPL code in the commit description?
I would prefer software evolution in an other direction.
https://lore.kernel.org/lkml/[email protected]/
https://lkml.org/lkml/2019/7/4/21
> @initialize:ocaml@
> @@
>
> let relevant_str = "use of_node_put() on it when done"
I see further possibilities to improve this data processing approach.
https://lore.kernel.org/lkml/[email protected]/
https://lore.kernel.org/patchwork/patch/1095169/#1291378
https://lkml.org/lkml/2019/6/28/326
I am missing more constructive answers for mentioned development concerns.
> And this patch also looks for places …
Does a SmPL script perform an action?
> Finally, this patch finds use-after-free issues for a node.
> (implemented by the r_use_after_put rule)
This software extension is another interesting contribution.
But I imagine that a separate SmPL script can be more helpful for
this source code search pattern.
> v3: delete the global set, …
To which previous implementation detail do you refer here?
> +virtual report
> +virtual org
> +
> +@initialize:python@
> +@@
> +
> +report_miss_prefix = "ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line "
> +report_miss_suffix = ", but without a corresponding object release within this function."
> +org_miss_main = "acquired a node pointer with refcount incremented"
> +org_miss_sec = "needed of_node_put"
> +report_use_after_put = "ERROR: use-after-free; reference preceded by of_node_put on line "
> +org_use_after_put_main = "of_node_put"
> +org_use_after_put_sec = "reference"
If you would insist on the usage of these variables, they should be applied
only for the selected analysis operation mode.
I would expect corresponding SmPL dependency specifications.
https://github.com/coccinelle/coccinelle/blob/b4509f6e7fb06d5616bb19dd5a110b203fd0e566/docs/manual/cocci_syntax.tex#L559
> +@r_miss_put exists@
> +local idexpression struct device_node *x;
> +expression e, e1;
> +position p1, p2;
> +statement S;
> +type T, T1;
> +@@
> +
> +* x = @p1\(of_find_all_nodes\|
The usage of the SmPL asterisk functionality can fit to the operation mode “context”.
https://bottest.wiki.kernel.org/coccicheck#modes
Would you like to add any corresponding SmPL details?
Under which circumstances will remaining programming concerns be clarified
for such SmPL disjunctions?
> +... when != e = (T)x
> + when != true x == NULL
Will assignment exclusions get any more software development attention?
https://lore.kernel.org/lkml/[email protected]/
https://lore.kernel.org/patchwork/patch/1095169/#1291892
https://lkml.org/lkml/2019/6/29/193
> + when != of_node_put(x)
…
> +)
> +&
> +x = f(...)
> +...
> +if (<+...x...+>) S
> +...
> +of_node_put(x);
> +)
You propose once more to use a SmPL conjunction in the rule “r_miss_put_ext”.
I am also still waiting for a definitive explanation on the applicability
of this combination.
> +@r_put@
> +expression E;
> +position p1;
> +@@
> +
> +* of_node_put@p1(E)
I guess that this SmPL code will need further adjustments.
> +@r_use_after_put exists@
> +expression r_put.E, subE<=r_put.E;
I have got an understanding difficulty around the interpretation
of the shown SmPL constraint.
How will the clarification be continued?
Regards,
Markus
On Tue, 16 Jul 2019, Markus Elfring wrote:
> > We find these functions by using the following script:
>
> Why would you like to keep this SmPL code in the commit description?
I don't know indetail what you are proposing, but I would prefer not to
put semantic patches that involve iteration into the kernel, for
simplicity.
julia
>
> I would prefer software evolution in an other direction.
> https://lore.kernel.org/lkml/[email protected]/
> https://lkml.org/lkml/2019/7/4/21
>
>
> > @initialize:ocaml@
> > @@
> >
> > let relevant_str = "use of_node_put() on it when done"
>
> I see further possibilities to improve this data processing approach.
> https://lore.kernel.org/lkml/[email protected]/
> https://lore.kernel.org/patchwork/patch/1095169/#1291378
> https://lkml.org/lkml/2019/6/28/326
>
>
> I am missing more constructive answers for mentioned development concerns.
>
>
> > And this patch also looks for places …
>
> Does a SmPL script perform an action?
>
>
> > Finally, this patch finds use-after-free issues for a node.
> > (implemented by the r_use_after_put rule)
>
> This software extension is another interesting contribution.
> But I imagine that a separate SmPL script can be more helpful for
> this source code search pattern.
>
>
> > v3: delete the global set, …
>
> To which previous implementation detail do you refer here?
>
>
> > +virtual report
> > +virtual org
> > +
> > +@initialize:python@
> > +@@
> > +
> > +report_miss_prefix = "ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line "
> > +report_miss_suffix = ", but without a corresponding object release within this function."
> > +org_miss_main = "acquired a node pointer with refcount incremented"
> > +org_miss_sec = "needed of_node_put"
> > +report_use_after_put = "ERROR: use-after-free; reference preceded by of_node_put on line "
> > +org_use_after_put_main = "of_node_put"
> > +org_use_after_put_sec = "reference"
>
> If you would insist on the usage of these variables, they should be applied
> only for the selected analysis operation mode.
> I would expect corresponding SmPL dependency specifications.
> https://github.com/coccinelle/coccinelle/blob/b4509f6e7fb06d5616bb19dd5a110b203fd0e566/docs/manual/cocci_syntax.tex#L559
>
>
> > +@r_miss_put exists@
> > +local idexpression struct device_node *x;
> > +expression e, e1;
> > +position p1, p2;
> > +statement S;
> > +type T, T1;
> > +@@
> > +
> > +* x = @p1\(of_find_all_nodes\|
>
> The usage of the SmPL asterisk functionality can fit to the operation mode “context”.
> https://bottest.wiki.kernel.org/coccicheck#modes
> Would you like to add any corresponding SmPL details?
>
> Under which circumstances will remaining programming concerns be clarified
> for such SmPL disjunctions?
>
>
> > +... when != e = (T)x
> > + when != true x == NULL
>
> Will assignment exclusions get any more software development attention?
> https://lore.kernel.org/lkml/[email protected]/
> https://lore.kernel.org/patchwork/patch/1095169/#1291892
> https://lkml.org/lkml/2019/6/29/193
>
>
> > + when != of_node_put(x)
> …
> > +)
> > +&
> > +x = f(...)
> > +...
> > +if (<+...x...+>) S
> > +...
> > +of_node_put(x);
> > +)
>
> You propose once more to use a SmPL conjunction in the rule “r_miss_put_ext”.
> I am also still waiting for a definitive explanation on the applicability
> of this combination.
>
>
> > +@r_put@
> > +expression E;
> > +position p1;
> > +@@
> > +
> > +* of_node_put@p1(E)
>
> I guess that this SmPL code will need further adjustments.
>
>
> > +@r_use_after_put exists@
> > +expression r_put.E, subE<=r_put.E;
>
> I have got an understanding difficulty around the interpretation
> of the shown SmPL constraint.
> How will the clarification be continued?
>
> Regards,
> Markus
>
>> Why would you like to keep this SmPL code in the commit description?
>
> I don't know indetail what you are proposing,
I imagine that you can get more interesting software development ideas
from links to previous messages.
I hope that the desired clarification can become more constructive.
How are the chances to move such code into SmPL script files?
> but I would prefer not to put semantic patches that involve iteration
> into the kernel, for simplicity.
This view is also interesting.
But I hope that this functionality will become more helpful
if we can agree on value combinations which should be iterated
for powerful source code analysis.
>> I would prefer software evolution in an other direction.
>> https://lore.kernel.org/lkml/[email protected]/
>> https://lkml.org/lkml/2019/7/4/21
Would you like to add any more advices for affected software components?
Regards,
Markus
>> Why would you like to keep this SmPL code in the commit description?
>
> I don't know indetail what you are proposing,
I imagine that you can get more interesting software development ideas
from links to previous messages.
I hope that the desired clarification can become more constructive.
How are the chances to move such code into SmPL script files?
> but I would prefer not to put semantic patches that involve iteration
> into the kernel, for simplicity.
This view is also interesting.
But I hope that this functionality will become more helpful
if we can agree on value combinations which should be iterated
for powerful source code analysis.
>> I would prefer software evolution in an other direction.
>> https://lore.kernel.org/lkml/[email protected]/
>> https://lkml.org/lkml/2019/7/4/21
Would you like to add any more advices for affected software components?
Regards,
Markus
> Finally, this patch finds use-after-free issues for a node.
> (implemented by the r_use_after_put rule)
I suggest to take another look also at information from a clarification attempt
on a topic like “Checking statement order for patch generation with SmPL support”.
https://systeme.lip6.fr/pipermail/cocci/2017-September/004483.html
https://lore.kernel.org/cocci/alpine.DEB.2.20.1709071519240.3168@hadrien/
Under which circumstances will it become safer to develop SmPL script variants
for such source code search patterns?
Regards,
Markus
> Finally, this patch finds use-after-free issues for a node.
> (implemented by the r_use_after_put rule)
I suggest to take another look also at information from a clarification attempt
on a topic like “Checking statement order for patch generation with SmPL support”.
https://systeme.lip6.fr/pipermail/cocci/2017-September/004483.html
https://lore.kernel.org/cocci/alpine.DEB.2.20.1709071519240.3168@hadrien/
Under which circumstances will it become safer to develop SmPL script variants
for such source code search patterns?
Regards,
Markus