LinuxLists.cc - [PATCH] coccinelle: api: add kzfree script

[permalink] [raw]

Subject: Re: [PATCH] coccinelle: api: add kzfree script

On Thu, 4 Jun 2020, Denis Efremov wrote:

> Check for memset() with 0 followed by kfree().
>
> Signed-off-by: Denis Efremov <[email protected]>
> ---
> Patches:
> 1. kzfree in drivers/w1 https://lkml.org/lkml/2020/6/4/438
> 2. kzfree in drivers/iommu/ https://lkml.org/lkml/2020/6/4/421
> 3. kzfree in drivers/scsi/ https://lkml.org/lkml/2020/6/4/442
>
> scripts/coccinelle/api/kzfree.cocci | 53 +++++++++++++++++++++++++++++
> 1 file changed, 53 insertions(+)
> create mode 100644 scripts/coccinelle/api/kzfree.cocci
>
> diff --git a/scripts/coccinelle/api/kzfree.cocci b/scripts/coccinelle/api/kzfree.cocci
> new file mode 100644
> index 000000000000..c6b8f7676af4
> --- /dev/null
> +++ b/scripts/coccinelle/api/kzfree.cocci
> @@ -0,0 +1,53 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +///
> +/// Use kzfree rather than memset with 0 followed by kfree
> +///
> +// Confidence: High
> +// Copyright: (C) 2020 Denis Efremov ISPRAS
> +// Options: --no-includes --include-headers
> +//
> +// Keywords: kzfree
> +//
> +
> +virtual context
> +virtual org
> +virtual report
> +virtual patch
> +
> +// Ignore kzfree definition
> +// Ignore kasan test
> +@r depends on !patch && !(file in "lib/test_kasan.c") && !(file in "mm/slab_common.c")@
> +expression E;
> +position p;
> +@@
> +
> +(
> +* memset(E, 0, ...);
> +|
> +* memset(E, '\0', ...);
> +)

You shouldn't need both cases. 0 should be enough.

Did you try ... here but find that some subexpressions of E could be
modified in between?

julia

> +* kfree(E)@p;
> +
> +@r1 depends on patch && !(file in "lib/test_kasan.c") && !(file in "mm/slab_common.c")@
> +expression E;
> +@@
> +
> +(
> +- memset(E, 0, ...);
> +|
> +- memset(E, '\0', ...);
> +)
> +- kfree(E);
> ++ kzfree(E);
> +
> +@script:python depends on report@
> +p << r.p;
> +@@
> +
> +coccilib.report.print_report(p[0], "WARNING opportunity for kzfree")
> +
> +@script:python depends on org@
> +p << r.p;
> +@@
> +
> +coccilib.org.print_todo(p[0], "WARNING opportunity for kzfree")
> --
> 2.26.2
>
>

2020-06-04 15:26:36

[permalink] [raw]

Subject: Re: [PATCH] coccinelle: api: add kzfree script

> Check for memset() with 0 followed by kfree().

I suggest to simplify the SmPL code a bit like the following.

> +virtual context
> +virtual org
> +virtual report
> +virtual patch

+virtual context, org, report, patch

…
> +@@
> +
> +(
> +* memset(E, 0, ...);
> +|
> +* memset(E, '\0', ...);
> +)
> +* kfree(E)@p;

+@@
+*memset(E, 0, ...);
+*kfree(E)@p;

How does the SmPL asterisk functionality fit to the operation
modes “org” and “report”?

> +@@
> +
> +(
> +- memset(E, 0, ...);
> +|
> +- memset(E, '\0', ...);
> +)
> +- kfree(E);
> ++ kzfree(E);

+@@
+-memset(E, 0, ...);
+-kfree
++kzfree
+ (E);

I got the impression that the specification of a SmPL disjunction
could be omitted because of the technical detail that the isomorphism
“zero_multiple_format” should handle such an use case already.

Would you like to tolerate any extra source code between these function calls?

Regards,
Markus

2020-06-04 15:56:24

[permalink] [raw]

Subject: Re: [PATCH] coccinelle: api: add kzfree script

On Thu, 4 Jun 2020, Denis Efremov wrote:

>
>
> On 6/4/20 5:15 PM, Julia Lawall wrote:
> > Did you try ... here but find that some subexpressions of E could be
> > modified in between?
>
> Yes, I tried to use "... when != E = E1 when != &E" and results were bad.
> Now, I've tried forall and when strict. Here are examples:
>
> // forall added
> // Works well, suitable for v2. One additional catch in w1 driver.
> @r depends on !patch && !(file in "lib/test_kasan.c") && !(file in "mm/slab_common.c") forall@
> expression *E; // pointer. Results are equal as if we use E.
> position p;
> @@
>
> * memset(E, 0, ...);
> ... when != E // Is it enough to match &E, E = E1?

Yes.

> * kfree(E)@p;
>
> //no forall, when strict
> //results are bad, too many false positives
> @r depends on !patch && !(file in "lib/test_kasan.c") && !(file in "mm/slab_common.c")@
> expression *E;
> position p;
> @@
>
> * memset(E, 0, ...);
> ... when != E // E is not enough here
> when strict

OK, it's reassonable.

> * kfree(E)@p;
>
> I guess that the difference is that "forall" requires that whole pattern should occur on
> every path, "when strict" states that kfree should be called on every path after memset.
> This results in missed uses of E in loops and under conditions. How can I state in this
> case that E should not occur at all (in all paths) in between memset, kfree even as a
> subexpression?
>
> // Doesn't work well
> ... when != E
> when != if (...) { ... E ... }
> when != for(...;...;...) { ... E ... }

Could you send an example of some C code on which the result is not
suitable?

julia

2020-06-04 16:01:43

[permalink] [raw]

Subject: Re: [PATCH] coccinelle: api: add kzfree script

On Thu, 4 Jun 2020, Markus Elfring wrote:

> > Check for memset() with 0 followed by kfree().
>
> I suggest to simplify the SmPL code a bit like the following.
>
>
> > +virtual context
> > +virtual org
> > +virtual report
> > +virtual patch
>
> +virtual context, org, report, patch

This is pointless.

>
>
> …
> > +@@
> > +
> > +(
> > +* memset(E, 0, ...);
> > +|
> > +* memset(E, '\0', ...);
> > +)
> > +* kfree(E)@p;
>
> +@@
> +*memset(E, 0, ...);
> +*kfree(E)@p;
>
>
> How does the SmPL asterisk functionality fit to the operation
> modes “org” and “report”?

make coccicheck uses the option --no-show-diff for the org and report
modes.

>
> > +@@
> > +
> > +(
> > +- memset(E, 0, ...);
> > +|
> > +- memset(E, '\0', ...);
> > +)
> > +- kfree(E);
> > ++ kzfree(E);
>
> +@@
> +-memset(E, 0, ...);
> +-kfree
> ++kzfree
> + (E);
>
> I got the impression that the specification of a SmPL disjunction
> could be omitted because of the technical detail that the isomorphism
> “zero_multiple_format” should handle such an use case already.
>
> Would you like to tolerate any extra source code between these function calls?

I already addressed these issues.

julia

2020-06-04 16:11:04

[permalink] [raw]

Subject: Re: coccinelle: api: add kzfree script

>>> +virtual context
>>> +virtual org
>>> +virtual report
>>> +virtual patch
>>
>> +virtual context, org, report, patch
>
> This is pointless.

I find the proposed SmPL code variant more succinct.

>> How does the SmPL asterisk functionality fit to the operation
>> modes “org” and “report”?
>
> make coccicheck uses the option --no-show-diff for the org and report modes.

Can it be nicer to avoid the usage of specific functionality
in the SmPL script directly?
Should the corresponding SmPL rule dependencies become clearer?

Regards,
Markus

2020-06-04 17:33:21

[permalink] [raw]

Subject: Re: [PATCH] coccinelle: api: add kzfree script

On 6/4/20 7:27 PM, Joe Perches wrote:
> On Thu, 2020-06-04 at 17:08 +0300, Denis Efremov wrote:
>> Check for memset() with 0 followed by kfree().
>
> Perhaps those uses should be memzero_explicit or kvfree_sensitive.

Thanks, I will add memzero_explicit(). However, I can't find kvfree_sensitive().
Is it in next already?

Thanks,
Denis

2020-06-04 18:59:34

[permalink] [raw]

Subject: Re: [PATCH] coccinelle: api: add kzfree script

> Could you send an example of some C code on which the result is not
> suitable?

I've updated the pattern to handle false positives:

@ifok@
position p;
expression *E;
@@

(
if (...) {
...
memset(E, 0, ...)@p;
...
}
|
if (...) {
...
} else {
...
memset(E, 0, ...)@p;
...
}
)

// Ignore kzfree definition
// Ignore kasan test
@r depends on !patch && !(file in "lib/test_kasan.c") && !(file in "mm/slab_common.c")@
expression *E;
position p != ifok.p;
@@

* memset(E, 0, ...)@p;
... when != E
when != if (...) { ... E ... }
when != for (...;...;...) { ... E ... }
when != while (...) { ... E ... }
when strict
* kfree(E);

Example of false positives:

void test_memset_under_if(void)
{
char *p = malloc(10, GFP_KERNEL);
if (p % 5) {
p[5] = 1;
} else {
memset(p, 0, 10);
}
kfree(p);
}

void test_memset_under_if(void)
{
int i;
char *p = malloc(10, GFP_KERNEL);
for (i = 0; i < 10; ++i) {
memset(p, 0, 10);
}
kfree(p);
}

void test_E_in_if(void)
{
char *p = malloc(10, GFP_KERNEL);
memset(p, 0, 10); // when != E is not enough
if (10) { // when != if (...) { ... E ... } is required
p[5] = 1;
}
kfree(p);
}

void test_E_in_for(void)
{
char *p = malloc(10, GFP_KERNEL);
memset(p, 0, 10);
for(;;) {
p[5] = 1;
}
kfree(p);
}

void test_E_in_while(void)
{
char *p = malloc(10, GFP_KERNEL);
memset(p, 0, 10);
while(1) {
p[6] = 2;
}
kfree(p);
}

void test_E_in_struct(void)
{
struct t { int a[3]; };
struct t *p = malloc(10 * sizeof(struct(struct t)), GFP_KERNEL);
memset(p, 0, 10);
for(;;) {
if (1) {
p->a[2] = 1; // I give up on this
p->a[0] = 10;
}
}
kfree(p);
}

After all it seems reasonable to me to add forall and memset_explicit rather
than handle all these false positives. Something like this for v2?

@r depends on !patch && !(file in "lib/test_kasan.c") && !(file in "mm/slab_common.c") forall@
expression *E;
position p;
@@

* $memset\|memset_explicit$(E, 0, ...);
... when != E
* kfree(E)@p;

Do I need to add "when strict" with forall or it's already enabled in this case?
Do I need to enable forall for pathing "-/+"?

Thanks,
Denis

2020-06-04 19:54:01

[permalink] [raw]

Subject: Re: [PATCH] coccinelle: api: add kzfree script

On 6/4/20 5:15 PM, Julia Lawall wrote:
> Did you try ... here but find that some subexpressions of E could be
> modified in between?

Yes, I tried to use "... when != E = E1 when != &E" and results were bad.
Now, I've tried forall and when strict. Here are examples:

// forall added
// Works well, suitable for v2. One additional catch in w1 driver.
@r depends on !patch && !(file in "lib/test_kasan.c") && !(file in "mm/slab_common.c") forall@
expression *E; // pointer. Results are equal as if we use E.
position p;
@@

* memset(E, 0, ...);
... when != E // Is it enough to match &E, E = E1?
* kfree(E)@p;

//no forall, when strict
//results are bad, too many false positives
@r depends on !patch && !(file in "lib/test_kasan.c") && !(file in "mm/slab_common.c")@
expression *E;
position p;
@@

* memset(E, 0, ...);
... when != E // E is not enough here
when strict
* kfree(E)@p;

I guess that the difference is that "forall" requires that whole pattern should occur on
every path, "when strict" states that kfree should be called on every path after memset.
This results in missed uses of E in loops and under conditions. How can I state in this
case that E should not occur at all (in all paths) in between memset, kfree even as a
subexpression?

// Doesn't work well
... when != E
when != if (...) { ... E ... }
when != for(...;...;...) { ... E ... }

Regards,
Denis

2020-06-04 19:58:12

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH] coccinelle: api: add kzfree script

On Thu, 2020-06-04 at 17:08 +0300, Denis Efremov wrote:
> Check for memset() with 0 followed by kfree().

Perhaps those uses should be memzero_explicit or kvfree_sensitive.

2020-06-04 20:02:13

[permalink] [raw]

Subject: Re: [PATCH] coccinelle: api: add kzfree script

> After all it seems reasonable to me to add forall and memset_explicit rather
> than handle all these false positives. Something like this for v2?
>
> @r depends on !patch && !(file in "lib/test_kasan.c") && !(file in "mm/slab_common.c") forall@
> expression *E;
> position p;
> @@
>
> * $memset\|memset_explicit$(E, 0, ...);
> ... when != E
> * kfree(E)@p;
>
> Do I need to add "when strict" with forall or it's already enabled in this case?
> Do I need to enable forall for pathing "-/+"?

forall seems entirely reasonable. You don't need it in the -/+ case. I
would put when strict in both cases.

julia

2020-06-04 20:51:34

[permalink] [raw]

Subject: [PATCH v2] coccinelle: api: add kzfree script

Check for memset()/memset_explicit() with 0 followed by
kfree()/vfree()/kvfree().

Signed-off-by: Denis Efremov <[email protected]>
---
Changes in v2:
- memset_explicit() added
- kvfree_sensitive() added
- forall added to r1
- ... between memset and kfree added
Unfortunately, it doesn't work as I would expect it to in "patch"
mode. I've added my comment about it in the rule. It can be safely
removed from the patch if I misunderstood something.

Another "strange" behaviour that I faced that r2 rule works only if I
write 2 expression lines:
expression *E;
expression size;
If I try to use a single line "expression *E, size;" then r2 matches nothing.

scripts/coccinelle/api/kzfree.cocci | 65 +++++++++++++++++++++++++++++
1 file changed, 65 insertions(+)
create mode 100644 scripts/coccinelle/api/kzfree.cocci

diff --git a/scripts/coccinelle/api/kzfree.cocci b/scripts/coccinelle/api/kzfree.cocci
new file mode 100644
index 000000000000..5c7e4bb13bb7
--- /dev/null
+++ b/scripts/coccinelle/api/kzfree.cocci
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Use kzfree, kvfree_sensitive rather than memset or
+/// memset_explicit with 0 followed by kfree
+///
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+// Keywords: kzfree, kvfree_sensitive
+//
+
+virtual context
+virtual patch
+virtual org
+virtual report
+
+
+// Ignore kzfree definition
+// Ignore kasan test
+@r depends on !patch && !(file in "lib/test_kasan.c") && !(file in "mm/slab_common.c") forall@
+expression *E;
+position p;
+@@
+
+* $memset\|memset_explicit$(E, 0, ...);
+ ... when != E
+ when strict
+* $kfree\|vfree\|kvfree$(E)@p;
+
+@r1 depends on patch && !(file in "lib/test_kasan.c") && !(file in "mm/slab_common.c")@
+expression *E;
+expression size;
+@@
+
+- $memset\|memset_explicit$(E, 0, size);
+/// Unfortunately, it doesn't work as in !patch mode.
+/// spatch (v1.0.8) should patch 4 functions in linux 5.7 with this rule
+/// and uncommented "when" lines. With only "... when != E" line 2 functions
+/// are patched, none with "when strict". 3 functions patch is produced by the
+/// rule with "when" lines commented out.
+// ... when != E
+// when strict
+(
+- kfree(E);
++ kzfree(E);
+|
+- vfree(E);
++ kvfree_sensitive(E, size);
+|
+- kvfree(E);
++ kvfree_sensitive(E, size);
+)
+
+@script:python depends on report@
+p << r.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for kzfree/kvfree_sensitive")
+
+@script:python depends on org@
+p << r.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING opportunity for kzfree/kvfree_sensitive")
--
2.26.2

2020-06-04 21:00:55

[permalink] [raw]

Subject: Re: [Cocci] [PATCH v2] coccinelle: api: add kzfree script

On Thu, 4 Jun 2020, Denis Efremov wrote:

> Check for memset()/memset_explicit() with 0 followed by
> kfree()/vfree()/kvfree().
>
> Signed-off-by: Denis Efremov <[email protected]>
> ---
> Changes in v2:
> - memset_explicit() added
> - kvfree_sensitive() added
> - forall added to r1
> - ... between memset and kfree added
> Unfortunately, it doesn't work as I would expect it to in "patch"
> mode. I've added my comment about it in the rule. It can be safely
> removed from the patch if I misunderstood something.
>
> Another "strange" behaviour that I faced that r2 rule works only if I
> write 2 expression lines:
> expression *E;
> expression size;
> If I try to use a single line "expression *E, size;" then r2 matches nothing.

The parser for metavariables is not so smart. Everything to the left of
the first metavariable name is the type. Everything after is the list of
metavariables of that type. So if you put them together you require size
to be a pointer.

On the other hand, do you really require E to be a pointer? If you do
that, it will have to find the type of E. If E refers to a structure
field, then the type might not be available in the current function, and
you may need command line argments like --all-includes or
--recursive-includes. Is avoiding transforming the case where E is not
verified to be a pointer a concern?

julia

>
> scripts/coccinelle/api/kzfree.cocci | 65 +++++++++++++++++++++++++++++
> 1 file changed, 65 insertions(+)
> create mode 100644 scripts/coccinelle/api/kzfree.cocci
>
> diff --git a/scripts/coccinelle/api/kzfree.cocci b/scripts/coccinelle/api/kzfree.cocci
> new file mode 100644
> index 000000000000..5c7e4bb13bb7
> --- /dev/null
> +++ b/scripts/coccinelle/api/kzfree.cocci
> @@ -0,0 +1,65 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +///
> +/// Use kzfree, kvfree_sensitive rather than memset or
> +/// memset_explicit with 0 followed by kfree
> +///
> +// Confidence: High
> +// Copyright: (C) 2020 Denis Efremov ISPRAS
> +// Options: --no-includes --include-headers
> +//
> +// Keywords: kzfree, kvfree_sensitive
> +//
> +
> +virtual context
> +virtual patch
> +virtual org
> +virtual report
> +
> +
> +// Ignore kzfree definition
> +// Ignore kasan test
> +@r depends on !patch && !(file in "lib/test_kasan.c") && !(file in "mm/slab_common.c") forall@
> +expression *E;
> +position p;
> +@@
> +
> +* $memset\|memset_explicit$(E, 0, ...);
> + ... when != E
> + when strict
> +* $kfree\|vfree\|kvfree$(E)@p;
> +
> +@r1 depends on patch && !(file in "lib/test_kasan.c") && !(file in "mm/slab_common.c")@
> +expression *E;
> +expression size;
> +@@
> +
> +- $memset\|memset_explicit$(E, 0, size);
> +/// Unfortunately, it doesn't work as in !patch mode.
> +/// spatch (v1.0.8) should patch 4 functions in linux 5.7 with this rule
> +/// and uncommented "when" lines. With only "... when != E" line 2 functions
> +/// are patched, none with "when strict". 3 functions patch is produced by the
> +/// rule with "when" lines commented out.
> +// ... when != E
> +// when strict
> +(
> +- kfree(E);
> ++ kzfree(E);
> +|
> +- vfree(E);
> ++ kvfree_sensitive(E, size);
> +|
> +- kvfree(E);
> ++ kvfree_sensitive(E, size);
> +)
> +
> +@script:python depends on report@
> +p << r.p;
> +@@
> +
> +coccilib.report.print_report(p[0], "WARNING opportunity for kzfree/kvfree_sensitive")
> +
> +@script:python depends on org@
> +p << r.p;
> +@@
> +
> +coccilib.org.print_todo(p[0], "WARNING opportunity for kzfree/kvfree_sensitive")
> --
> 2.26.2
>
> _______________________________________________
> Cocci mailing list
> [email protected]
> https://systeme.lip6.fr/mailman/listinfo/cocci
>

2020-06-04 21:07:00

[permalink] [raw]

Subject: Re: [Cocci] [PATCH v2] coccinelle: api: add kzfree script

On 6/4/20 11:57 PM, Julia Lawall wrote:
> On the other hand, do you really require E to be a pointer? If you do
> that, it will have to find the type of E. If E refers to a structure
> field, then the type might not be available in the current function, and
> you may need command line argments like --all-includes or
> --recursive-includes. Is avoiding transforming the case where E is not
> verified to be a pointer a concern?

It's save to use "expression E;" in this rule. Do you want me to send v3?

Any thoughts about commented lines in "patch"? Maybe I do something wrong
trying to use "..." it in r2?

Thanks,
Denis

2020-06-04 21:27:19

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH] coccinelle: api: add kzfree script

On Thu, 2020-06-04 at 20:30 +0300, Denis Efremov wrote:
>
> On 6/4/20 7:27 PM, Joe Perches wrote:
> > On Thu, 2020-06-04 at 17:08 +0300, Denis Efremov wrote:
> > > Check for memset() with 0 followed by kfree().
> >
> > Perhaps those uses should be memzero_explicit or kvfree_sensitive.
>
> Thanks, I will add memzero_explicit(). However, I can't find kvfree_sensitive().
> Is it in next already?

Yes

$ git grep kvfree_sensitive
include/linux/mm.h:extern void kvfree_sensitive(const void *addr, size_t len);
mm/util.c: * kvfree_sensitive - Free a data object containing sensitive information.
mm/util.c:void kvfree_sensitive(const void *addr, size_t len)
mm/util.c:EXPORT_SYMBOL(kvfree_sensitive);
security/keys/keyctl.c: kvfree_sensitive(payload, plen);
security/keys/keyctl.c: kvfree_sensitive(payload, plen);
security/keys/keyctl.c: kvfree_sensitive(key_data, key_data_len);
security/keys/keyctl.c: kvfree_sensitive(key_data, key_data_len);
security/keys/keyctl.c: kvfree_sensitive(payload, plen);

2020-06-04 22:35:28

[permalink] [raw]

Subject: Re: [Cocci] [PATCH v2] coccinelle: api: add kzfree script

It looks like a good idea to add type cast, like:

+// Ignore kzfree definition
+// Ignore kasan test
+@r depends on !patch && !(file in "lib/test_kasan.c") && !(file in "mm/slab_common.c") forall@
+expression *E;
+position p;
+type T;
+@@
+
+* $memset\|memset_explicit$((T)E, 0, ...);
+ ... when != E
+ when strict
+* $kfree\|vfree\|kvfree$(E)@p;
+

and to exclude file "mm/util.c" because it will contain the definition of
kvfree_sensitive().

I will wait for your recommendation about commented lines and will send v3 after.

Thanks,
Denis

2020-06-05 05:52:56

[permalink] [raw]

Subject: Re: [PATCH v2] coccinelle: api: add kzfree script

> On the other hand, do you really require E to be a pointer?
> If you do that, it will have to find the type of E.

I suggest to reconsider this information.

> If E refers to a structure field, then the type might not be available
> in the current function, and you may need command line argments like
> --all-includes or --recursive-includes.

Will the software documentation need corresponding extensions for
the safe application of the semantic patch language?

Will the used data structure access operator like arrow or dot
influence the interpretation of the software situation?

> Is avoiding transforming the case where E is not verified
> to be a pointer a concern?

I would find it desirable to express constraints for pointer data types
according to the applied programming interfaces.

Regards,
Markus

2020-06-05 06:46:00

[permalink] [raw]

Subject: Re: [PATCH v2] coccinelle: api: add kzfree script

> Check for memset()/memset_explicit() with 0 followed by
> kfree()/vfree()/kvfree().

Another software evolution will become interesting here.

> +/// memset_explicit with 0 followed by kfree

How do you think about to extend this comment?

Would you like to take the following SmPL code variants into account?

> +virtual context
> +virtual org
> +virtual report
> +virtual patch

+virtual context, org, report, patch

> +(
> +- kfree(E);
> ++ kzfree(E);
> +|
> +- vfree(E);
> ++ kvfree_sensitive(E, size);
> +|
> +- kvfree(E);
> ++ kvfree_sensitive(E, size);
> +)

+(
+-kfree
++kzfree
+ (E);
+|
+-vfree
++kvfree_sensitive
+ (E
++ , size
+ );
+|
+-kvfree
++kvfree_sensitive
+ (E
++ , size
+ );
+)

Regards,
Markus

2020-06-06 08:19:18

[permalink] [raw]

Subject: Re: [Cocci] [PATCH v2] coccinelle: api: add kzfree script

On Fri, 5 Jun 2020, Denis Efremov wrote:

> It looks like a good idea to add type cast, like:
>
> +// Ignore kzfree definition
> +// Ignore kasan test
> +@r depends on !patch && !(file in "lib/test_kasan.c") && !(file in "mm/slab_common.c") forall@
> +expression *E;
> +position p;
> +type T;
> +@@
> +
> +* $memset\|memset_explicit$((T)E, 0, ...);
> + ... when != E
> + when strict
> +* $kfree\|vfree\|kvfree$(E)@p;
> +
>
> and to exclude file "mm/util.c" because it will contain the definition of
> kvfree_sensitive().
>
> I will wait for your recommendation about commented lines and will send v3 after.

Instead of the file in things, maybe it would be simpler to say:

position p : script:ocaml() { not (List.mem (List.hd p).current_element ["kzfree";"..."]) };

Or:

@initialize:ocaml@
@@

let relevant p =
not (List.mem (List.hd p).current_element ["kzfree";"..."])

and then

position p : script:ocaml() { relevant p };

Or the python counterpart. It's true that the script is probably not
relevant to those files at all, but listing the specific functions would
avoid the need for the comments and make the issue more clear.

It's just a suggestion. If you prefer the file in solution, that's ok
too.

julia

2020-06-14 19:46:56

[permalink] [raw]

Subject: Re: [PATCH] coccinelle: api: add kzfree script

On 6/4/20 7:27 PM, Joe Perches wrote:
> On Thu, 2020-06-04 at 17:08 +0300, Denis Efremov wrote:
>> Check for memset() with 0 followed by kfree().
>
> Perhaps those uses should be memzero_explicit or kvfree_sensitive.
>

Is it safe to suggest to use kzfree instead of memzero_explicit && kfree?
Or it would be better to use kvfree_sensitive in this case.

kzfree uses memset(0) with no barrier_data.

For example:
diff -u -p a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c
--- a/drivers/crypto/inside-secure/safexcel_hash.c
+++ b/drivers/crypto/inside-secure/safexcel_hash.c
@@ -1081,8 +1081,7 @@ static int safexcel_hmac_init_pad(struct
}

/* Avoid leaking */
- memzero_explicit(keydup, keylen);
- kfree(keydup);
+ kzfree(keydup);

if (ret)
return ret;

Thanks,
Denis

2020-06-14 20:06:34

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH] coccinelle: api: add kzfree script

On Sun, 2020-06-14 at 22:42 +0300, Denis Efremov wrote:
> On 6/4/20 7:27 PM, Joe Perches wrote:
> > On Thu, 2020-06-04 at 17:08 +0300, Denis Efremov wrote:
> > > Check for memset() with 0 followed by kfree().
> >
> > Perhaps those uses should be memzero_explicit or kvfree_sensitive.
> >
> Is it safe to suggest to use kzfree instead of memzero_explicit && kfree?
> Or it would be better to use kvfree_sensitive in this case.
> kzfree uses memset(0) with no barrier_data.
>
> For example:
> diff -u -p a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c
[]
> @@ -1081,8 +1081,7 @@ static int safexcel_hmac_init_pad(struct
> }
>
> /* Avoid leaking */
> - memzero_explicit(keydup, keylen);
> - kfree(keydup);
> + kzfree(keydup);

It would be better to use kvfree_sensitive()

2020-06-14 22:00:03

[permalink] [raw]

Subject: [PATCH v3] coccinelle: api: add kzfree script

Check for memset()/memzero_explicit() followed by kfree()/vfree()/kvfree().

Signed-off-by: Denis Efremov <[email protected]>
---
Changes in v2:
- memset_explicit() added
- kvfree_sensitive() added
- forall added to r1
- ... between memset and kfree added
Changes in v3:
- Explicit filter for definitions instead of !(file in "...") conditions
- type T added to match casts
- memzero_explicit() patterns fixed
- additional rule "cond" added to filter false-positives

scripts/coccinelle/api/kzfree.cocci | 90 +++++++++++++++++++++++++++++
1 file changed, 90 insertions(+)
create mode 100644 scripts/coccinelle/api/kzfree.cocci

diff --git a/scripts/coccinelle/api/kzfree.cocci b/scripts/coccinelle/api/kzfree.cocci
new file mode 100644
index 000000000000..4758ca5a781e
--- /dev/null
+++ b/scripts/coccinelle/api/kzfree.cocci
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Use kzfree, kvfree_sensitive rather than memset or
+/// memzero_explicit followed by kfree
+///
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+// Keywords: kzfree, kvfree_sensitive
+//
+
+virtual context
+virtual patch
+virtual org
+virtual report
+
+@initialize:python@
+@@
+# kmalloc_oob_in_memset uses memset to explicitly trigger out-of-bounds access
+filter = frozenset(['kmalloc_oob_in_memset', 'kzfree', 'kvfree_sensitive'])
+
+def relevant(p):
+ return not (filter & {el.current_element for el in p})
+
+@cond@
+position ok;
+@@
+
+if (...)
+ $memset@ok\|memzero_explicit@ok$(...);
+
+@r depends on !patch forall@
+expression E;
+position p : script:python() { relevant(p) };
+position m != cond.ok;
+type T;
+@@
+
+(
+* memset@m((T)E, 0, ...);
+|
+* memzero_explicit@m((T)E, ...);
+)
+ ... when != E
+ when strict
+* $kfree\|vfree\|kvfree$(E)@p;
+
+@rp_memzero depends on patch@
+expression E, size;
+position p : script:python() { relevant(p) };
+type T;
+@@
+
+- memzero_explicit((T)E, size)@p;
+ ... when != E
+ when strict
+- $kfree\|vfree\|kvfree$(E);
++ kvfree_sensitive(E, size);
+
+@rp_memset depends on patch@
+expression E, size;
+position p : script:python() { relevant(p) };
+type T;
+@@
+
+- memset((T)E, size)@p;
+ ... when != E
+ when strict
+(
+- kfree(E);
++ kzfree(E);
+|
+- $vfree\|kvfree$(E);
++ kvfree_sensitive(E, size);
+)
+
+@script:python depends on report@
+p << r.p;
+@@
+
+coccilib.report.print_report(p[0],
+ "WARNING: opportunity for kzfree/kvfree_sensitive")
+
+@script:python depends on org@
+p << r.p;
+@@
+
+coccilib.org.print_todo(p[0],
+ "WARNING: opportunity for kzfree/kvfree_sensitive")
--
2.26.2

2020-06-15 12:08:04

by Dan Carpenter

[permalink] [raw]

Subject: Re: [PATCH] coccinelle: api: add kzfree script

On Sun, Jun 14, 2020 at 10:42:54PM +0300, Denis Efremov wrote:
> On 6/4/20 7:27 PM, Joe Perches wrote:
> > On Thu, 2020-06-04 at 17:08 +0300, Denis Efremov wrote:
> >> Check for memset() with 0 followed by kfree().
> >
> > Perhaps those uses should be memzero_explicit or kvfree_sensitive.
> >
>
> Is it safe to suggest to use kzfree instead of memzero_explicit && kfree?
> Or it would be better to use kvfree_sensitive in this case.
>
> kzfree uses memset(0) with no barrier_data.

Yeah. That seems buggy. It should have a barrier. Also I thought I
saw somewhere that Linus doesn't like the name and so that's why we have
the _sensitive() name?

regards,
dan carpenter

2020-06-15 13:31:13

[permalink] [raw]

Subject: Re: [PATCH v3] coccinelle: api: add kzfree script

…
> +virtual context
> +virtual patch
> +virtual org
> +virtual report

+virtual context, patch, org, report

Is such a SmPL code variant more succinct?

…
> +if (...)
> + $memset@ok\|memzero_explicit@ok$(...);

Would you like to tolerate any extra source code around such a function call
in an if branch?

…
> +(
> +* memset@m((T)E, 0, ...);
> +|
> +* memzero_explicit@m((T)E, ...);
> +)
…

I suggest to move a semicolon.

+(
+*memset@m((T)E, 0, ...)
+|
+*memzero_explicit@m((T)E, ...)
+);

…
> +- $kfree\|vfree\|kvfree$(E);
> ++ kvfree_sensitive(E, size);
…

Would you like to increase the precision a bit for the change specification?

+-$kfree\|vfree\|kvfree$
++kvfree_sensitive
+ (E
++ , size
+ );

…
> +(
> +- kfree(E);
> ++ kzfree(E);
> +|
> +- $vfree\|kvfree$(E);
> ++ kvfree_sensitive(E, size);
> +)
…

+(
+-kfree
++kzfree
+ (E)
+|
+-$vfree\|kvfree$
++kvfree_sensitive
+ (E
++ , size
+ )
+);

…
> +coccilib.org.print_todo(p[0],
> + "WARNING: opportunity for kzfree/kvfree_sensitive")

I suggest to align the second function parameter.

+coccilib.org.print_todo(p[0],
+ "WARNING: opportunity for kzfree/kvfree_sensitive")

Regards,
Markus

2020-06-15 13:55:53

[permalink] [raw]

Subject: Re: [PATCH] coccinelle: api: add kzfree script

On 6/15/20 3:03 PM, Dan Carpenter wrote:
> On Sun, Jun 14, 2020 at 10:42:54PM +0300, Denis Efremov wrote:
>> On 6/4/20 7:27 PM, Joe Perches wrote:
>>> On Thu, 2020-06-04 at 17:08 +0300, Denis Efremov wrote:
>>>> Check for memset() with 0 followed by kfree().
>>>
>>> Perhaps those uses should be memzero_explicit or kvfree_sensitive.
>>>
>>
>> Is it safe to suggest to use kzfree instead of memzero_explicit && kfree?
>> Or it would be better to use kvfree_sensitive in this case.
>>
>> kzfree uses memset(0) with no barrier_data.
>
> Yeah. That seems buggy. It should have a barrier. Also I thought I
> saw somewhere that Linus doesn't like the name and so that's why we have
> the _sensitive() name?
>

Oh, there are already patches for renaming kzfree to kfree_sensitive.

https://lkml.org/lkml/2020/4/13/729

It seems they are not accepted despite multiple acks, through.

Thanks,
Denis

2020-06-17 20:46:09

[permalink] [raw]

Subject: Re: [Cocci] [PATCH v3] coccinelle: api: add kzfree script

On Mon, 15 Jun 2020, Denis Efremov wrote:

> Check for memset()/memzero_explicit() followed by kfree()/vfree()/kvfree().
>
> Signed-off-by: Denis Efremov <[email protected]>
> ---
> Changes in v2:
> - memset_explicit() added
> - kvfree_sensitive() added
> - forall added to r1
> - ... between memset and kfree added
> Changes in v3:
> - Explicit filter for definitions instead of !(file in "...") conditions
> - type T added to match casts
> - memzero_explicit() patterns fixed
> - additional rule "cond" added to filter false-positives
>
> scripts/coccinelle/api/kzfree.cocci | 90 +++++++++++++++++++++++++++++
> 1 file changed, 90 insertions(+)
> create mode 100644 scripts/coccinelle/api/kzfree.cocci
>
> diff --git a/scripts/coccinelle/api/kzfree.cocci b/scripts/coccinelle/api/kzfree.cocci
> new file mode 100644
> index 000000000000..4758ca5a781e
> --- /dev/null
> +++ b/scripts/coccinelle/api/kzfree.cocci
> @@ -0,0 +1,90 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +///
> +/// Use kzfree, kvfree_sensitive rather than memset or
> +/// memzero_explicit followed by kfree
> +///
> +// Confidence: High
> +// Copyright: (C) 2020 Denis Efremov ISPRAS
> +// Options: --no-includes --include-headers
> +//
> +// Keywords: kzfree, kvfree_sensitive
> +//
> +
> +virtual context
> +virtual patch
> +virtual org
> +virtual report
> +
> +@initialize:python@
> +@@
> +# kmalloc_oob_in_memset uses memset to explicitly trigger out-of-bounds access
> +filter = frozenset(['kmalloc_oob_in_memset', 'kzfree', 'kvfree_sensitive'])
> +
> +def relevant(p):
> + return not (filter & {el.current_element for el in p})
> +
> +@cond@
> +position ok;
> +@@
> +
> +if (...)
> + $memset@ok\|memzero_explicit@ok$(...);
> +
> +@r depends on !patch forall@
> +expression E;
> +position p : script:python() { relevant(p) };
> +position m != cond.ok;
> +type T;
> +@@
> +
> +(
> +* memset@m((T)E, 0, ...);
> +|
> +* memzero_explicit@m((T)E, ...);
> +)
> + ... when != E
> + when strict
> +* $kfree\|vfree\|kvfree$(E)@p;
> +
> +@rp_memzero depends on patch@
> +expression E, size;
> +position p : script:python() { relevant(p) };
> +type T;
> +@@
> +
> +- memzero_explicit((T)E, size)@p;
> + ... when != E
> + when strict
> +- $kfree\|vfree\|kvfree$(E);
> ++ kvfree_sensitive(E, size);
> +
> +@rp_memset depends on patch@
> +expression E, size;
> +position p : script:python() { relevant(p) };
> +type T;
> +@@
> +
> +- memset((T)E, size)@p;

This is missing a 0 argument.

> + ... when != E
> + when strict
> +(
> +- kfree(E);
> ++ kzfree(E);
> +|
> +- $vfree\|kvfree$(E);
> ++ kvfree_sensitive(E, size);
> +)

I'm not sure why you want kzfree in the first case, but kvfree_sensitive
in the second case.

julia

> +
> +@script:python depends on report@
> +p << r.p;
> +@@
> +
> +coccilib.report.print_report(p[0],
> + "WARNING: opportunity for kzfree/kvfree_sensitive")
> +
> +@script:python depends on org@
> +p << r.p;
> +@@
> +
> +coccilib.org.print_todo(p[0],
> + "WARNING: opportunity for kzfree/kvfree_sensitive")
> --
> 2.26.2
>
> _______________________________________________
> Cocci mailing list
> [email protected]
> https://systeme.lip6.fr/mailman/listinfo/cocci
>

2020-06-17 21:44:53

[permalink] [raw]

Subject: Re: [Cocci] [PATCH v3] coccinelle: api: add kzfree script

>> +@rp_memset depends on patch@
>> +expression E, size;
>> +position p : script:python() { relevant(p) };
>> +type T;
>> +@@
>> +
>> +- memset((T)E, size)@p;
>
> This is missing a 0 argument.
>

Thanks, I will send v4.

>
>
>> + ... when != E
>> + when strict
>> +(
>> +- kfree(E);
>> ++ kzfree(E);
>> +|
>> +- $vfree\|kvfree$(E);
>> ++ kvfree_sensitive(E, size);
>> +)
>
> I'm not sure why you want kzfree in the first case, but kvfree_sensitive
> in the second case.
>

As for now in kernel:

memset(E,0,...) && kfree(E) is kzfree()

There are no vzfree or kvzfree functions.
Thus, we use kvfree_sensitive().

Maybe it's worth to wait for this patchset:
https://lkml.org/lkml/2020/6/16/1163

With it the rule will use:

(
- kfree(E);
+ kfree_sensitive(E);
|
- $vfree\|kvfree$(E);
+ kvfree_sensitive(E, size);
)

Thanks,
Denis

2020-07-07 21:38:03

[permalink] [raw]

Subject: Re: [Cocci] [PATCH v3] coccinelle: api: add kzfree script

On Mon, 15 Jun 2020, Denis Efremov wrote:

> Check for memset()/memzero_explicit() followed by kfree()/vfree()/kvfree().
>
> Signed-off-by: Denis Efremov <[email protected]>
> ---
> Changes in v2:
> - memset_explicit() added
> - kvfree_sensitive() added
> - forall added to r1
> - ... between memset and kfree added
> Changes in v3:
> - Explicit filter for definitions instead of !(file in "...") conditions
> - type T added to match casts
> - memzero_explicit() patterns fixed
> - additional rule "cond" added to filter false-positives
>
> scripts/coccinelle/api/kzfree.cocci | 90 +++++++++++++++++++++++++++++
> 1 file changed, 90 insertions(+)
> create mode 100644 scripts/coccinelle/api/kzfree.cocci
>
> diff --git a/scripts/coccinelle/api/kzfree.cocci b/scripts/coccinelle/api/kzfree.cocci
> new file mode 100644
> index 000000000000..4758ca5a781e
> --- /dev/null
> +++ b/scripts/coccinelle/api/kzfree.cocci
> @@ -0,0 +1,90 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +///
> +/// Use kzfree, kvfree_sensitive rather than memset or
> +/// memzero_explicit followed by kfree
> +///
> +// Confidence: High
> +// Copyright: (C) 2020 Denis Efremov ISPRAS
> +// Options: --no-includes --include-headers
> +//
> +// Keywords: kzfree, kvfree_sensitive
> +//
> +
> +virtual context
> +virtual patch
> +virtual org
> +virtual report
> +
> +@initialize:python@
> +@@
> +# kmalloc_oob_in_memset uses memset to explicitly trigger out-of-bounds access
> +filter = frozenset(['kmalloc_oob_in_memset', 'kzfree', 'kvfree_sensitive'])
> +
> +def relevant(p):
> + return not (filter & {el.current_element for el in p})
> +
> +@cond@
> +position ok;
> +@@
> +
> +if (...)
> + $memset@ok\|memzero_explicit@ok$(...);
> +
> +@r depends on !patch forall@
> +expression E;
> +position p : script:python() { relevant(p) };
> +position m != cond.ok;
> +type T;
> +@@
> +
> +(
> +* memset@m((T)E, 0, ...);
> +|
> +* memzero_explicit@m((T)E, ...);
> +)
> + ... when != E
> + when strict
> +* $kfree\|vfree\|kvfree$(E)@p;
> +
> +@rp_memzero depends on patch@
> +expression E, size;
> +position p : script:python() { relevant(p) };
> +type T;
> +@@
> +
> +- memzero_explicit((T)E, size)@p;

This rule also needs a @m, like in the rule above.

> + ... when != E
> + when strict
> +- $kfree\|vfree\|kvfree$(E);
> ++ kvfree_sensitive(E, size);
> +
> +@rp_memset depends on patch@
> +expression E, size;
> +position p : script:python() { relevant(p) };
> +type T;
> +@@
> +
> +- memset((T)E, size)@p;

This rule also needs a @m. It was also previously noted that this
call to memset is msising a 0.

julia

> + ... when != E
> + when strict
> +(
> +- kfree(E);
> ++ kzfree(E);
> +|
> +- $vfree\|kvfree$(E);
> ++ kvfree_sensitive(E, size);
> +)
> +
> +@script:python depends on report@
> +p << r.p;
> +@@
> +
> +coccilib.report.print_report(p[0],
> + "WARNING: opportunity for kzfree/kvfree_sensitive")
> +
> +@script:python depends on org@
> +p << r.p;
> +@@
> +
> +coccilib.org.print_todo(p[0],
> + "WARNING: opportunity for kzfree/kvfree_sensitive")
> --
> 2.26.2
>
> _______________________________________________
> Cocci mailing list
> [email protected]
> https://systeme.lip6.fr/mailman/listinfo/cocci
>

2020-07-17 11:58:22

[permalink] [raw]

Subject: [PATCH v4] coccinelle: api: add kzfree script

Check for memset()/memzero_explicit() followed by kfree()/vfree()/kvfree().

Signed-off-by: Denis Efremov <[email protected]>
---
Changes in v2:
- memset_explicit() added
- kvfree_sensitive() added
- forall added to r1
- ... between memset and kfree added
Changes in v3:
- Explicit filter for definitions instead of !(file in "...") conditions
- type T added to match casts
- memzero_explicit() patterns fixed
- additional rule "cond" added to filter false-positives
Changes in v4:
- memset call fixed in rp_memset
- @m added to rp_memset,rp_memzero rules

scripts/coccinelle/api/kzfree.cocci | 101 ++++++++++++++++++++++++++++
1 file changed, 101 insertions(+)
create mode 100644 scripts/coccinelle/api/kzfree.cocci

diff --git a/scripts/coccinelle/api/kzfree.cocci b/scripts/coccinelle/api/kzfree.cocci
new file mode 100644
index 000000000000..33625bd7cec9
--- /dev/null
+++ b/scripts/coccinelle/api/kzfree.cocci
@@ -0,0 +1,101 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Use kzfree, kvfree_sensitive rather than memset or
+/// memzero_explicit followed by kfree
+///
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers
+//
+// Keywords: kzfree, kvfree_sensitive
+//
+
+virtual context
+virtual patch
+virtual org
+virtual report
+
+@initialize:python@
+@@
+# kmalloc_oob_in_memset uses memset to explicitly trigger out-of-bounds access
+filter = frozenset(['kmalloc_oob_in_memset', 'kzfree', 'kvfree_sensitive'])
+
+def relevant(p):
+ return not (filter & {el.current_element for el in p})
+
+@cond@
+position ok;
+@@
+
+if (...)
+ $memset@ok\|memzero_explicit@ok$(...);
+
+@r depends on !patch forall@
+expression E;
+position p : script:python() { relevant(p) };
+position m != cond.ok;
+type T;
+@@
+
+(
+* memset@m((T)E, 0, ...);
+|
+* memzero_explicit@m((T)E, ...);
+)
+ ... when != E
+ when strict
+* $kfree\|vfree\|kvfree$(E)@p;
+
+@rp_memzero depends on patch@
+expression E, size;
+position p : script:python() { relevant(p) };
+position m != cond.ok;
+type T;
+@@
+
+- memzero_explicit@m((T)E, size);
+ ... when != E
+ when strict
+// TODO: uncomment when kfree_sensitive will be merged.
+// Only this case is commented out because developers
+// may not like patches like this since kzfree uses memset
+// internally (not memzero_explicit).
+//(
+//- kfree(E)@p;
+//+ kfree_sensitive(E);
+//|
+- $vfree\|kvfree$(E)@p;
++ kvfree_sensitive(E, size);
+//)
+
+@rp_memset depends on patch@
+expression E, size;
+position p : script:python() { relevant(p) };
+position m != cond.ok;
+type T;
+@@
+
+- memset@m((T)E, 0, size);
+ ... when != E
+ when strict
+(
+- kfree(E)@p;
++ kzfree(E);
+|
+- $vfree\|kvfree$(E)@p;
++ kvfree_sensitive(E, size);
+)
+
+@script:python depends on report@
+p << r.p;
+@@
+
+coccilib.report.print_report(p[0],
+ "WARNING: opportunity for kzfree/kvfree_sensitive")
+
+@script:python depends on org@
+p << r.p;
+@@
+
+coccilib.org.print_todo(p[0],
+ "WARNING: opportunity for kzfree/kvfree_sensitive")
--
2.26.2

2020-07-17 16:06:05

[permalink] [raw]

Subject: Re: [PATCH v4] coccinelle: api: add kzfree script

I dare to repeat previous patch review aspects once more.
https://lore.kernel.org/cocci/[email protected]/

…
> +virtual context
> +virtual patch
> +virtual org
> +virtual report

+virtual context, patch, org, report

Is such a SmPL code variant more succinct?

…
> +if (...)
> + $memset@ok\|memzero_explicit@ok$(...);

Would you like to tolerate any extra source code around such a function call
in an if branch?

…
> +(
> +* memset@m((T)E, 0, ...);
> +|
> +* memzero_explicit@m((T)E, ...);
> +)
…

I suggest to move a semicolon.

+(
+*memset@m((T)E, 0, ...)
+|
+*memzero_explicit@m((T)E, ...)
+);

…
> +- $kfree\|vfree\|kvfree$(E);
> ++ kvfree_sensitive(E, size);
…

Would you like to increase the precision a bit for the change specification?

+-$kfree\|vfree\|kvfree$
++kvfree_sensitive
+ (E
++ , size
+ );

…
> +(
> +- kfree(E);
> ++ kzfree(E);
> +|
> +- $vfree\|kvfree$(E);
> ++ kvfree_sensitive(E, size);
> +)
…

+(
+-kfree
++kzfree
+ (E)
+|
+-$vfree\|kvfree$
++kvfree_sensitive
+ (E
++ , size
+ )
+);

…
> +// TODO: uncomment when kfree_sensitive will be merged.
> +// Only this case is commented out because developers
> +// may not like patches like this since kzfree uses memset
> +// internally (not memzero_explicit).

Will this information trigger any further clarification?

…
> +coccilib.org.print_todo(p[0],
> + "WARNING: opportunity for kzfree/kvfree_sensitive")

I propose to align the second function parameter.

+coccilib.org.print_todo(p[0],
+ "WARNING: opportunity for kzfree/kvfree_sensitive")

Regards,
Markus

2020-07-17 20:40:30

[permalink] [raw]

Subject: Re: [PATCH v4] coccinelle: api: add kzfree script

On Fri, 17 Jul 2020, Denis Efremov wrote:

> Check for memset()/memzero_explicit() followed by kfree()/vfree()/kvfree().
>
> Signed-off-by: Denis Efremov <[email protected]>

Applied.

> ---
> Changes in v2:
> - memset_explicit() added
> - kvfree_sensitive() added
> - forall added to r1
> - ... between memset and kfree added
> Changes in v3:
> - Explicit filter for definitions instead of !(file in "...") conditions
> - type T added to match casts
> - memzero_explicit() patterns fixed
> - additional rule "cond" added to filter false-positives
> Changes in v4:
> - memset call fixed in rp_memset
> - @m added to rp_memset,rp_memzero rules
>
> scripts/coccinelle/api/kzfree.cocci | 101 ++++++++++++++++++++++++++++
> 1 file changed, 101 insertions(+)
> create mode 100644 scripts/coccinelle/api/kzfree.cocci
>
> diff --git a/scripts/coccinelle/api/kzfree.cocci b/scripts/coccinelle/api/kzfree.cocci
> new file mode 100644
> index 000000000000..33625bd7cec9
> --- /dev/null
> +++ b/scripts/coccinelle/api/kzfree.cocci
> @@ -0,0 +1,101 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +///
> +/// Use kzfree, kvfree_sensitive rather than memset or
> +/// memzero_explicit followed by kfree
> +///
> +// Confidence: High
> +// Copyright: (C) 2020 Denis Efremov ISPRAS
> +// Options: --no-includes --include-headers
> +//
> +// Keywords: kzfree, kvfree_sensitive
> +//
> +
> +virtual context
> +virtual patch
> +virtual org
> +virtual report
> +
> +@initialize:python@
> +@@
> +# kmalloc_oob_in_memset uses memset to explicitly trigger out-of-bounds access
> +filter = frozenset(['kmalloc_oob_in_memset', 'kzfree', 'kvfree_sensitive'])
> +
> +def relevant(p):
> + return not (filter & {el.current_element for el in p})
> +
> +@cond@
> +position ok;
> +@@
> +
> +if (...)
> + $memset@ok\|memzero_explicit@ok$(...);
> +
> +@r depends on !patch forall@
> +expression E;
> +position p : script:python() { relevant(p) };
> +position m != cond.ok;
> +type T;
> +@@
> +
> +(
> +* memset@m((T)E, 0, ...);
> +|
> +* memzero_explicit@m((T)E, ...);
> +)
> + ... when != E
> + when strict
> +* $kfree\|vfree\|kvfree$(E)@p;
> +
> +@rp_memzero depends on patch@
> +expression E, size;
> +position p : script:python() { relevant(p) };
> +position m != cond.ok;
> +type T;
> +@@
> +
> +- memzero_explicit@m((T)E, size);
> + ... when != E
> + when strict
> +// TODO: uncomment when kfree_sensitive will be merged.
> +// Only this case is commented out because developers
> +// may not like patches like this since kzfree uses memset
> +// internally (not memzero_explicit).
> +//(
> +//- kfree(E)@p;
> +//+ kfree_sensitive(E);
> +//|
> +- $vfree\|kvfree$(E)@p;
> ++ kvfree_sensitive(E, size);
> +//)
> +
> +@rp_memset depends on patch@
> +expression E, size;
> +position p : script:python() { relevant(p) };
> +position m != cond.ok;
> +type T;
> +@@
> +
> +- memset@m((T)E, 0, size);
> + ... when != E
> + when strict
> +(
> +- kfree(E)@p;
> ++ kzfree(E);
> +|
> +- $vfree\|kvfree$(E)@p;
> ++ kvfree_sensitive(E, size);
> +)
> +
> +@script:python depends on report@
> +p << r.p;
> +@@
> +
> +coccilib.report.print_report(p[0],
> + "WARNING: opportunity for kzfree/kvfree_sensitive")
> +
> +@script:python depends on org@
> +p << r.p;
> +@@
> +
> +coccilib.org.print_todo(p[0],
> + "WARNING: opportunity for kzfree/kvfree_sensitive")
> --
> 2.26.2
>
>

2020-08-10 23:49:35

by Eric Biggers

[permalink] [raw]

Subject: Re: [PATCH v4] coccinelle: api: add kzfree script

On Fri, Jul 17, 2020 at 10:39:20PM +0200, Julia Lawall wrote:
>
>
> On Fri, 17 Jul 2020, Denis Efremov wrote:
>
> > Check for memset()/memzero_explicit() followed by kfree()/vfree()/kvfree().
> >
> > Signed-off-by: Denis Efremov <[email protected]>
>
> Applied.

FYI, this new script is already outdated, since kzfree() has been renamed to
kfree_sensitive().

- Eric

2020-08-11 07:13:41

[permalink] [raw]

Subject: Re: [PATCH v4] coccinelle: api: add kzfree script

On 8/11/20 2:45 AM, Eric Biggers wrote:
> On Fri, Jul 17, 2020 at 10:39:20PM +0200, Julia Lawall wrote:
>>
>>
>> On Fri, 17 Jul 2020, Denis Efremov wrote:
>>
>>> Check for memset()/memzero_explicit() followed by kfree()/vfree()/kvfree().
>>>
>>> Signed-off-by: Denis Efremov <[email protected]>
>>
>> Applied.
>
> FYI, this new script is already outdated, since kzfree() has been renamed to
> kfree_sensitive().
>

Ok, I will send an update.

Thanks,Denis

2020-08-11 07:51:58

[permalink] [raw]

Subject: [PATCH] coccinelle: api: update kzfree script to kfree_sensitive

Commit 453431a54934 ("mm, treewide: rename kzfree() to kfree_sensitive()")
renames kzfree to kfree_sensitive and uses memzero_explicit(...) instead of
memset(..., 0, ...) internally. Update cocci script to reflect these
changes.

Signed-off-by: Denis Efremov <[email protected]>
---
Julia, I think you can squash this commit with original script, or I can
resend the whole script since it's not merged to the mainline.

.../{kzfree.cocci => kfree_sensitive.cocci} | 29 +++++++++----------
1 file changed, 13 insertions(+), 16 deletions(-)
rename scripts/coccinelle/api/{kzfree.cocci => kfree_sensitive.cocci} (70%)

diff --git a/scripts/coccinelle/api/kzfree.cocci b/scripts/coccinelle/api/kfree_sensitive.cocci
similarity index 70%
rename from scripts/coccinelle/api/kzfree.cocci
rename to scripts/coccinelle/api/kfree_sensitive.cocci
index 33625bd7cec9..e4a066a0b77d 100644
--- a/scripts/coccinelle/api/kzfree.cocci
+++ b/scripts/coccinelle/api/kfree_sensitive.cocci
@@ -1,13 +1,13 @@
// SPDX-License-Identifier: GPL-2.0-only
///
-/// Use kzfree, kvfree_sensitive rather than memset or
-/// memzero_explicit followed by kfree
+/// Use kfree_sensitive, kvfree_sensitive rather than memset or
+/// memzero_explicit followed by kfree.
///
// Confidence: High
// Copyright: (C) 2020 Denis Efremov ISPRAS
// Options: --no-includes --include-headers
//
-// Keywords: kzfree, kvfree_sensitive
+// Keywords: kfree_sensitive, kvfree_sensitive
//

virtual context
@@ -18,7 +18,8 @@ virtual report
@initialize:python@
@@
# kmalloc_oob_in_memset uses memset to explicitly trigger out-of-bounds access
-filter = frozenset(['kmalloc_oob_in_memset', 'kzfree', 'kvfree_sensitive'])
+filter = frozenset(['kmalloc_oob_in_memset',
+ 'kfree_sensitive', 'kvfree_sensitive'])

def relevant(p):
return not (filter & {el.current_element for el in p})
@@ -56,17 +57,13 @@ type T;
- memzero_explicit@m((T)E, size);
... when != E
when strict
-// TODO: uncomment when kfree_sensitive will be merged.
-// Only this case is commented out because developers
-// may not like patches like this since kzfree uses memset
-// internally (not memzero_explicit).
-//(
-//- kfree(E)@p;
-//+ kfree_sensitive(E);
-//|
+(
+- kfree(E)@p;
++ kfree_sensitive(E);
+|
- $vfree\|kvfree$(E)@p;
+ kvfree_sensitive(E, size);
-//)
+)

@rp_memset depends on patch@
expression E, size;
@@ -80,7 +77,7 @@ type T;
when strict
(
- kfree(E)@p;
-+ kzfree(E);
++ kfree_sensitive(E);
|
- $vfree\|kvfree$(E)@p;
+ kvfree_sensitive(E, size);
@@ -91,11 +88,11 @@ p << r.p;
@@

coccilib.report.print_report(p[0],
- "WARNING: opportunity for kzfree/kvfree_sensitive")
+ "WARNING: opportunity for kfree_sensitive/kvfree_sensitive")

@script:python depends on org@
p << r.p;
@@

coccilib.org.print_todo(p[0],
- "WARNING: opportunity for kzfree/kvfree_sensitive")
+ "WARNING: opportunity for kfree_sensitive/kvfree_sensitive")
--
2.26.2

2020-08-26 14:05:19

[permalink] [raw]

Subject: Re: [PATCH] coccinelle: api: update kzfree script to kfree_sensitive

Ping?

On 8/11/20 10:49 AM, Denis Efremov wrote:
> Commit 453431a54934 ("mm, treewide: rename kzfree() to kfree_sensitive()")
> renames kzfree to kfree_sensitive and uses memzero_explicit(...) instead of
> memset(..., 0, ...) internally. Update cocci script to reflect these
> changes.
>
> Signed-off-by: Denis Efremov <[email protected]>
> ---
> Julia, I think you can squash this commit with original script, or I can
> resend the whole script since it's not merged to the mainline.
>
> .../{kzfree.cocci => kfree_sensitive.cocci} | 29 +++++++++----------
> 1 file changed, 13 insertions(+), 16 deletions(-)
> rename scripts/coccinelle/api/{kzfree.cocci => kfree_sensitive.cocci} (70%)
>
> diff --git a/scripts/coccinelle/api/kzfree.cocci b/scripts/coccinelle/api/kfree_sensitive.cocci
> similarity index 70%
> rename from scripts/coccinelle/api/kzfree.cocci
> rename to scripts/coccinelle/api/kfree_sensitive.cocci
> index 33625bd7cec9..e4a066a0b77d 100644
> --- a/scripts/coccinelle/api/kzfree.cocci
> +++ b/scripts/coccinelle/api/kfree_sensitive.cocci
> @@ -1,13 +1,13 @@
> // SPDX-License-Identifier: GPL-2.0-only
> ///
> -/// Use kzfree, kvfree_sensitive rather than memset or
> -/// memzero_explicit followed by kfree
> +/// Use kfree_sensitive, kvfree_sensitive rather than memset or
> +/// memzero_explicit followed by kfree.
> ///
> // Confidence: High
> // Copyright: (C) 2020 Denis Efremov ISPRAS
> // Options: --no-includes --include-headers
> //
> -// Keywords: kzfree, kvfree_sensitive
> +// Keywords: kfree_sensitive, kvfree_sensitive
> //
>
> virtual context
> @@ -18,7 +18,8 @@ virtual report
> @initialize:python@
> @@
> # kmalloc_oob_in_memset uses memset to explicitly trigger out-of-bounds access
> -filter = frozenset(['kmalloc_oob_in_memset', 'kzfree', 'kvfree_sensitive'])
> +filter = frozenset(['kmalloc_oob_in_memset',
> + 'kfree_sensitive', 'kvfree_sensitive'])
>
> def relevant(p):
> return not (filter & {el.current_element for el in p})
> @@ -56,17 +57,13 @@ type T;
> - memzero_explicit@m((T)E, size);
> ... when != E
> when strict
> -// TODO: uncomment when kfree_sensitive will be merged.
> -// Only this case is commented out because developers
> -// may not like patches like this since kzfree uses memset
> -// internally (not memzero_explicit).
> -//(
> -//- kfree(E)@p;
> -//+ kfree_sensitive(E);
> -//|
> +(
> +- kfree(E)@p;
> ++ kfree_sensitive(E);
> +|
> - $vfree\|kvfree$(E)@p;
> + kvfree_sensitive(E, size);
> -//)
> +)
>
> @rp_memset depends on patch@
> expression E, size;
> @@ -80,7 +77,7 @@ type T;
> when strict
> (
> - kfree(E)@p;
> -+ kzfree(E);
> ++ kfree_sensitive(E);
> |
> - $vfree\|kvfree$(E)@p;
> + kvfree_sensitive(E, size);
> @@ -91,11 +88,11 @@ p << r.p;
> @@
>
> coccilib.report.print_report(p[0],
> - "WARNING: opportunity for kzfree/kvfree_sensitive")
> + "WARNING: opportunity for kfree_sensitive/kvfree_sensitive")
>
> @script:python depends on org@
> p << r.p;
> @@
>
> coccilib.org.print_todo(p[0],
> - "WARNING: opportunity for kzfree/kvfree_sensitive")
> + "WARNING: opportunity for kfree_sensitive/kvfree_sensitive")
>

2020-09-12 15:10:55