scripts/coccinelle: catch freeing cryptographic structures via kfree

PATCH Superseded Konstantin Khlebnikov 3 participants 8 messages Nov 17, 2014 – Nov 23, 2014

Subsystems (5)

Crypto API Live update kexec handover (KHO) kexec +1 more subsystems

lore

Share & tools

lore

Konstantin Khlebnikov

author

2014-11-17 15:14 UTC #0 lore raw

PATCH 1/2 scripts/coccinelle: catch freeing cryptographic structures via kfree

Structures allocated by crypto_alloc_* must be freed using crypto_free_*.

Signed-off-by Konstantin Khlebnikov <[email protected]>

---
 scripts/coccinelle/free/crypto_free.cocci |   45 +++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 scripts/coccinelle/free/crypto_free.cocci

scripts/coccinelle/free/crypto_free.cocci new file +45 -0

new file mode 100644
index 0000000..0799b70
@@ -0,0 +1,45 @@
+///
+/// Structures allocated by crypto_alloc_* must be freed using crypto_free_*.
+/// This finds freeing them by kfree.
+///
+// Confidence: Moderate
+// Copyright: (C) 2014 Konstantin Khlebnikov,  GPLv2.
+// Comments: There are false positives in crypto/ where they are actually freed.
+// Keywords: crypto, kfree
+// Options: --no-includes --include-headers
+
+virtual org
+virtual report
+virtual context
+
+@r depends on context || org || report@
+expression x;
+identifier crypto_alloc =~ "^crypto_alloc_";
+@@
+
+(
+ x = crypto_alloc(...)
+)
+
+@pb@
+expression r.x;
+position p;
+@@
+
+(
+* kfree@p(x)
+)
+
+@script:python depends on org@
+p << pb.p;
+@@
+
+msg="WARNING: invalid free of crypto_alloc_* allocated data"
+coccilib.org.print_todo(p[0], msg)
+
+@script:python depends on report@
+p << pb.p;
+@@
+
+msg="WARNING: invalid free of crypto_alloc_* allocated data"
+coccilib.report.print_report(p[0], msg)

Konstantin Khlebnikov

author

2014-11-17 15:14 UTC #1 lore raw

PATCH 2/2 kernel/kexec: free crypto_shash using crypto_free_shash

These objects have special freeing functions which cares about proper destruction and reference counting.

Signed-off-by Konstantin Khlebnikov <[email protected]>

---
 kernel/kexec.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/kexec.c +1 -1

index 2abf9f6..5a62311 100644
@@ -2286,7 +2286,7 @@ out_free_sha_regions:
 out_free_desc:
 	kfree(desc);
 out_free_tfm:
-	kfree(tfm);
+	crypto_free_shash(tfm);
 out:
 	return ret;
 }

Julia Lawall

2014-11-17 15:30 UTC #2 lore raw

In reply to Konstantin Khlebnikov

Quoted 26 lines · Konstantin Khlebnikov

Structures allocated by crypto_alloc_* must be freed using crypto_free_*.

Signed-off-by Konstantin Khlebnikov <[email protected]>

---
 scripts/coccinelle/free/crypto_free.cocci |   45 +++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 scripts/coccinelle/free/crypto_free.cocci

scripts/coccinelle/free/crypto_free.cocci new file +12 -0

new file mode 100644
index 0000000..0799b70
@@ -0,0 +1,45 @@
+///
+/// Structures allocated by crypto_alloc_* must be freed using crypto_free_*.
+/// This finds freeing them by kfree.
+///
+// Confidence: Moderate
+// Copyright: (C) 2014 Konstantin Khlebnikov,  GPLv2.
+// Comments: There are false positives in crypto/ where they are actually freed.
+// Keywords: crypto, kfree
+// Options: --no-includes --include-headers
+
+virtual org
+virtual report

scripts/coccinelle/free/crypto_free.cocci line 22

Quoted 10 lines

scripts/coccinelle/free/crypto_free.cocci new file +10 -0

+virtual context
+
+@r depends on context || org || report@
+expression x;
+identifier crypto_alloc =~ "^crypto_alloc_";
+@@
+
+(
+ x = crypto_alloc(...)
+)

You can drop the outer parentheses, in this case and in the kfree case.

Are there many of these crypto_alloc_ functions? It would be nicer to avoid the regular expression. For one thing, you don't have much control over what it matches, and for another thing Coccinelle will not be able to optimize the selection of files. With the regular expression it will have to parse every file and analyze every function, which will be slow.

julia

Quoted 25 lines

scripts/coccinelle/free/crypto_free.cocci new file +23 -0

+
+@pb@
+expression r.x;
+position p;
+@@
+
+(
+* kfree@p(x)
+)
+
+@script:python depends on org@
+p << pb.p;
+@@
+
+msg="WARNING: invalid free of crypto_alloc_* allocated data"
+coccilib.org.print_todo(p[0], msg)
+
+@script:python depends on report@
+p << pb.p;
+@@
+
+msg="WARNING: invalid free of crypto_alloc_* allocated data"
+coccilib.report.print_report(p[0], msg)

Konstantin Khlebnikov

author

2014-11-17 15:40 UTC #3 lore raw

Quoted 41 lines · Julia Lawall

Quoted 26 lines · Konstantin Khlebnikov
Structures allocated by crypto_alloc_* must be freed using crypto_free_*.
Signed-off-by Konstantin Khlebnikov <[email protected]>
---
  scripts/coccinelle/free/crypto_free.cocci |   45 +++++++++++++++++++++++++++++
  1 file changed, 45 insertions(+)
create mode 100644 scripts/coccinelle/free/crypto_free.cocci
scripts/coccinelle/free/crypto_free.cocci new file +12 -0
new file mode 100644
index 0000000..0799b70
@@ -0,0 +1,45 @@
+///
+/// Structures allocated by crypto_alloc_* must be freed using crypto_free_*.
+/// This finds freeing them by kfree.
+///
+// Confidence: Moderate
+// Copyright: (C) 2014 Konstantin Khlebnikov,  GPLv2.
+// Comments: There are false positives in crypto/ where they are actually freed.
+// Keywords: crypto, kfree
+// Options: --no-includes --include-headers
+
+virtual org
+virtual report
scripts/coccinelle/free/crypto_free.cocci line 22
Quoted 10 lines
scripts/coccinelle/free/crypto_free.cocci new file +10 -0
+virtual context
+
+@r depends on context || org || report@
+expression x;
+identifier crypto_alloc =~ "^crypto_alloc_";
+@@
+
+(
+ x = crypto_alloc(...)
+)
You can drop the outer parentheses, in this case and in the kfree case.

Quoted 5 lines

Are there many of these crypto_alloc_ functions? It would be nicer to avoid the regular expression. For one thing, you don't have much control over what it matches, and for another thing Coccinelle will not be able to optimize the selection of files. With the regular expression it will have to parse every file and analyze every function, which will be slow.

As I see here is eight .. ten candidates, maybe some of them are internal. Ok, I'll resend patch without regex.

Quoted 28 lines

julia

Quoted 25 lines

scripts/coccinelle/free/crypto_free.cocci new file +23 -0

+
+@pb@
+expression r.x;
+position p;
+@@
+
+(
+* kfree@p(x)
+)
+
+@script:python depends on org@
+p << pb.p;
+@@
+
+msg="WARNING: invalid free of crypto_alloc_* allocated data"
+coccilib.org.print_todo(p[0], msg)
+
+@script:python depends on report@
+p << pb.p;
+@@
+
+msg="WARNING: invalid free of crypto_alloc_* allocated data"
+coccilib.report.print_report(p[0], msg)

Konstantin Khlebnikov

author

2014-11-18 10:50 UTC #4 lore raw

PATCH v2 1/2 scripts/coccinelle: catch freeing cryptographic structures via kfree

In reply to Konstantin Khlebnikov

Structures allocated by crypto_alloc_* must be freed using crypto_free_*.

Signed-off-by Konstantin Khlebnikov <[email protected]>

---
 scripts/coccinelle/free/crypto_free.cocci |   64 +++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)
 create mode 100644 scripts/coccinelle/free/crypto_free.cocci

scripts/coccinelle/free/crypto_free.cocci new file +64 -0

new file mode 100644
index 0000000..a717070
@@ -0,0 +1,64 @@
+///
+/// Structures allocated by crypto_alloc_* must be freed using crypto_free_*.
+/// This finds freeing them by kfree.
+///
+// Confidence: Moderate
+// Copyright: (C) 2014 Konstantin Khlebnikov,  GPLv2.
+// Comments: There are false positives in crypto/ where they are actually freed.
+// Keywords: crypto, kfree
+// Options: --no-includes --include-headers
+
+virtual org
+virtual report
+virtual context
+
+@r depends on context || org || report@
+expression x;
+@@
+
+(
+ x = crypto_alloc_base(...)
+|
+ x = crypto_alloc_cipher(...)
+|
+ x = crypto_alloc_ablkcipher(...)
+|
+ x = crypto_alloc_aead(...)
+|
+ x = crypto_alloc_instance(...)
+|
+ x = crypto_alloc_instance2(...)
+|
+ x = crypto_alloc_comp(...)
+|
+ x = crypto_alloc_pcomp(...)
+|
+ x = crypto_alloc_hash(...)
+|
+ x = crypto_alloc_ahash(...)
+|
+ x = crypto_alloc_shash(...)
+|
+ x = crypto_alloc_rng(...)
+)
+
+@pb@
+expression r.x;
+position p;
+@@
+
+* kfree@p(x)
+
+@script:python depends on org@
+p << pb.p;
+@@
+
+msg="WARNING: invalid free of crypto_alloc_* allocated data"
+coccilib.org.print_todo(p[0], msg)
+
+@script:python depends on report@
+p << pb.p;
+@@
+
+msg="WARNING: invalid free of crypto_alloc_* allocated data"
+coccilib.report.print_report(p[0], msg)

Julia Lawall

2014-11-19 09:41 UTC #5 lore raw

Quoted 2 lines

+// Comments: There are false positives in crypto/ where they are actually freed.

I didn't really understand this comment. I ran the semantic patch and got around 10 results, but it wasn't clear to me how to see which were false positives.

I would suggest to extend the rule a little bit to include information about where the allocation call is:

///
/// Structures allocated by crypto_alloc_* must be freed using crypto_free_*. /// This finds freeing them by kfree.
///
// Confidence: Moderate
// Copyright: (C) 2014 Konstantin Khlebnikov, GPLv2.
// Comments: There are false positives in crypto/ where they are actually freed. // Keywords: crypto, kfree
// Options: --no-includes --include-headers

virtual org
virtual report
virtual context

@r depends on context || org || report@
expression x;
position p1;
@@

@pb@
expression r.x;
position p;
@@

* kfree@p(x)

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

msg="WARNING: invalid free of crypto_alloc_* allocated data" coccilib.org.print_todo(p[0], msg) coccilib.org.print_link(p1[0], "allocation")

@script:python depends on report@
p << pb.p;
p1 << r.p1;
@@

msg="WARNING: invalid free of crypto_alloc_* allocated data, allocated on line %s" % (p1[0].line) coccilib.report.print_report(p[0], msg)

Konstantin Khlebnikov

2014-11-19 15:06 UTC #6 lore raw

Quoted 6 lines · Julia Lawall

Quoted 2 lines
+// Comments: There are false positives in crypto/ where they are actually freed.
I didn't really understand this comment. I ran the semantic patch and got around 10 results, but it wasn't clear to me how to see which were false positives.

I mean false positives in code inside directory crypto/ where this subsystem lives.

./arch/x86/crypto/fpu.c:143:1-6: WARNING: invalid free of crypto_alloc_* allocated data
./crypto/algapi.c:846:1-6: WARNING: invalid free of crypto_alloc_* allocated data
./crypto/lrw.c:378:1-6: WARNING: invalid free of crypto_alloc_* allocated data ./crypto/ecb.c:163:1-6: WARNING: invalid free of crypto_alloc_* allocated data ./crypto/ctr.c:242:1-6: WARNING: invalid free of crypto_alloc_* allocated data ./crypto/ctr.c:419:1-6: WARNING: invalid free of crypto_alloc_* allocated data ./crypto/ctr.c:428:1-6: WARNING: invalid free of crypto_alloc_* allocated data ./crypto/pcbc.c:273:1-6: WARNING: invalid free of crypto_alloc_* allocated data ./crypto/cts.c:329:1-6: WARNING: invalid free of crypto_alloc_* allocated data ./crypto/cbc.c:267:1-6: WARNING: invalid free of crypto_alloc_* allocated data ./crypto/xts.c:340:1-6: WARNING: invalid free of crypto_alloc_* allocated data

This is kfree calls on error paths in constructors or in destructors which actually frees memory when crypto_free_* is called.

Quoted 3 lines

I would suggest to extend the rule a little bit to include information about where the allocation call is:

Not sure if this is necessary. This part of crypto-api is simple.

Quoted 74 lines

///
/// Structures allocated by crypto_alloc_* must be freed using crypto_free_*. /// This finds freeing them by kfree.
///
// Confidence: Moderate
// Copyright: (C) 2014 Konstantin Khlebnikov, GPLv2.
// Comments: There are false positives in crypto/ where they are actually freed. // Keywords: crypto, kfree
// Options: --no-includes --include-headers
virtual org
virtual report
virtual context
@r depends on context || org || report@
expression x;
position p1;
@@
(
x = crypto_alloc_base@p1(...)
|
x = crypto_alloc_cipher@p1(...)
|
x = crypto_alloc_ablkcipher@p1(...)
|
x = crypto_alloc_aead@p1(...)
|
x = crypto_alloc_instance@p1(...)
|
x = crypto_alloc_instance2@p1(...)
|
x = crypto_alloc_comp@p1(...)
|
x = crypto_alloc_pcomp@p1(...)
|
x = crypto_alloc_hash@p1(...)
|
x = crypto_alloc_ahash@p1(...)
|
x = crypto_alloc_shash@p1(...)
|
x = crypto_alloc_rng@p1(...)
)
@pb@
expression r.x;
position p;
@@
* kfree@p(x)
@script:python depends on org@
p << pb.p;
p1 << r.p1;
@@
msg="WARNING: invalid free of crypto_alloc_* allocated data" coccilib.org.print_todo(p[0], msg) coccilib.org.print_link(p1[0], "allocation")
@script:python depends on report@
p << pb.p;
p1 << r.p1;
@@
msg="WARNING: invalid free of crypto_alloc_* allocated data, allocated on line %s" % (p1[0].line) coccilib.report.print_report(p[0], msg)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Julia Lawall

2014-11-23 16:45 UTC #7 lore raw

Quoted 25 lines · Konstantin Khlebnikov

Quoted 6 lines · Julia Lawall
Quoted 2 lines
+// Comments: There are false positives in crypto/ where they are actually freed.
I didn't really understand this comment. I ran the semantic patch and got around 10 results, but it wasn't clear to me how to see which were false positives.
I mean false positives in code inside directory crypto/ where this subsystem lives.
./arch/x86/crypto/fpu.c:143:1-6: WARNING: invalid free of crypto_alloc_* allocated data
./crypto/algapi.c:846:1-6: WARNING: invalid free of crypto_alloc_* allocated data
./crypto/lrw.c:378:1-6: WARNING: invalid free of crypto_alloc_* allocated data ./crypto/ecb.c:163:1-6: WARNING: invalid free of crypto_alloc_* allocated data ./crypto/ctr.c:242:1-6: WARNING: invalid free of crypto_alloc_* allocated data ./crypto/ctr.c:419:1-6: WARNING: invalid free of crypto_alloc_* allocated data ./crypto/ctr.c:428:1-6: WARNING: invalid free of crypto_alloc_* allocated data ./crypto/pcbc.c:273:1-6: WARNING: invalid free of crypto_alloc_* allocated data ./crypto/cts.c:329:1-6: WARNING: invalid free of crypto_alloc_* allocated data ./crypto/cbc.c:267:1-6: WARNING: invalid free of crypto_alloc_* allocated data ./crypto/xts.c:340:1-6: WARNING: invalid free of crypto_alloc_* allocated data

Quoted 2 lines

This is kfree calls on error paths in constructors or in destructors which actually frees memory when crypto_free_* is called.

Sorry, but I am not sure to fully understand the issue. Is it actually just the case that crypto_alloc_instance and crypto_alloc_instance2 should not be in the list of allocation functions, because their results should be freed with kfree?

julia

Quoted 82 lines

Quoted 4 lines
I would suggest to extend the rule a little bit to include information about where the allocation call is:
Not sure if this is necessary. This part of crypto-api is simple.
Quoted 74 lines
///
/// Structures allocated by crypto_alloc_* must be freed using crypto_free_*. /// This finds freeing them by kfree.
///
// Confidence: Moderate
// Copyright: (C) 2014 Konstantin Khlebnikov, GPLv2.
// Comments: There are false positives in crypto/ where they are actually freed. // Keywords: crypto, kfree
// Options: --no-includes --include-headers
virtual org
virtual report
virtual context
@r depends on context || org || report@
expression x;
position p1;
@@
(
x = crypto_alloc_base@p1(...)
|
x = crypto_alloc_cipher@p1(...)
|
x = crypto_alloc_ablkcipher@p1(...)
|
x = crypto_alloc_aead@p1(...)
|
x = crypto_alloc_instance@p1(...)
|
x = crypto_alloc_instance2@p1(...)
|
x = crypto_alloc_comp@p1(...)
|
x = crypto_alloc_pcomp@p1(...)
|
x = crypto_alloc_hash@p1(...)
|
x = crypto_alloc_ahash@p1(...)
|
x = crypto_alloc_shash@p1(...)
|
x = crypto_alloc_rng@p1(...)
)
@pb@
expression r.x;
position p;
@@
* kfree@p(x)
@script:python depends on org@
p << pb.p;
p1 << r.p1;
@@
msg="WARNING: invalid free of crypto_alloc_* allocated data" coccilib.org.print_todo(p[0], msg) coccilib.org.print_link(p1[0], "allocation")
@script:python depends on report@
p << pb.p;
p1 << r.p1;
@@
msg="WARNING: invalid free of crypto_alloc_* allocated data, allocated on line %s" % (p1[0].line) coccilib.report.print_report(p[0], msg)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/