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

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

[Konstantin Khlebnikov]

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

Signed-off-by: Konstantin Khlebnikov <k.khlebnikov@samsung.com>
---
 scripts/coccinelle/free/crypto_free.cocci |   45 +++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 scripts/coccinelle/free/crypto_free.cocci

diff --git a/scripts/coccinelle/free/crypto_free.cocci b/scripts/coccinelle/free/crypto_free.cocci
new file mode 100644
index 0000000..0799b70
--- /dev/null
+++ b/scripts/coccinelle/free/crypto_free.cocci
@@ -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)

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

[Konstantin Khlebnikov]

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

Signed-off-by: Konstantin Khlebnikov <k.khlebnikov@samsung.com>
---
 kernel/kexec.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/kexec.c b/kernel/kexec.c
index 2abf9f6..5a62311 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -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;
 }

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

[Julia Lawall]

On Mon, 17 Nov 2014, Konstantin Khlebnikov wrote:

> Structures allocated by crypto_alloc_* must be freed using crypto_free_*.
>
> Signed-off-by: Konstantin Khlebnikov <k.khlebnikov@samsung.com>
> ---
>  scripts/coccinelle/free/crypto_free.cocci |   45 +++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 scripts/coccinelle/free/crypto_free.cocci
>
> diff --git a/scripts/coccinelle/free/crypto_free.cocci b/scripts/coccinelle/free/crypto_free.cocci
> new file mode 100644
> index 0000000..0799b70
> --- /dev/null
> +++ b/scripts/coccinelle/free/crypto_free.cocci
> @@ -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(...)
> +)

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

> +
> +@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)
>
>

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

[Konstantin Khlebnikov]

On 2014-11-17 18:30, Julia Lawall wrote:
>
> On Mon, 17 Nov 2014, Konstantin Khlebnikov wrote:
>
>> Structures allocated by crypto_alloc_* must be freed using crypto_free_*.
>>
>> Signed-off-by: Konstantin Khlebnikov <k.khlebnikov@samsung.com>
>> ---
>>   scripts/coccinelle/free/crypto_free.cocci |   45 +++++++++++++++++++++++++++++
>>   1 file changed, 45 insertions(+)
>>   create mode 100644 scripts/coccinelle/free/crypto_free.cocci
>>
>> diff --git a/scripts/coccinelle/free/crypto_free.cocci b/scripts/coccinelle/free/crypto_free.cocci
>> new file mode 100644
>> index 0000000..0799b70
>> --- /dev/null
>> +++ b/scripts/coccinelle/free/crypto_free.cocci
>> @@ -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(...)
>> +)
> 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.

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

>
> julia
>
>> +
>> +@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)
>>
>>

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

[Konstantin Khlebnikov]

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

Signed-off-by: Konstantin Khlebnikov <k.khlebnikov@samsung.com>
---
 scripts/coccinelle/free/crypto_free.cocci |   64 +++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)
 create mode 100644 scripts/coccinelle/free/crypto_free.cocci

diff --git a/scripts/coccinelle/free/crypto_free.cocci b/scripts/coccinelle/free/crypto_free.cocci
new file mode 100644
index 0000000..a717070
--- /dev/null
+++ b/scripts/coccinelle/free/crypto_free.cocci
@@ -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)

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

[Julia Lawall]

> +// 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;
@@

(
 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)

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

[Konstantin Khlebnikov]

On Wed, Nov 19, 2014 at 12:41 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> +// 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.

>
> 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.

>
> ///
> /// 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

[Julia Lawall]

On Wed, 19 Nov 2014, Konstantin Khlebnikov wrote:

> On Wed, Nov 19, 2014 at 12:41 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> >> +// 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.

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


> 
> >
> > 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.
> 
> >
> > ///
> > /// 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 majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
>