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

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

[Markus Elfring]

> 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
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

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

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1183 bytes --]



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

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] coccinelle: api: add kzfree script

[Markus Elfring]

>>> +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
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

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

[Denis Efremov]

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
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

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

[Dan Carpenter]

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

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

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

[Joe Perches]

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


_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

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

[Denis Efremov]

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
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

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

[Joe Perches]

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


_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

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

[Denis Efremov]

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
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

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

[Julia Lawall]

> 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
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

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

[Denis Efremov]

> 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
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

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

[Joe Perches]

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.


_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

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

[Julia Lawall]

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
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

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

[Denis Efremov]

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
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

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

[Julia Lawall]

On Thu, 4 Jun 2020, Denis Efremov wrote:

> Check for memset() with 0 followed by kfree().
>
> Signed-off-by: Denis Efremov <efremov@linux.com>
> ---
> 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
>
>
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

[Cocci] [PATCH] coccinelle: api: add kzfree script

[Denis Efremov]

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

Signed-off-by: Denis Efremov <efremov@linux.com>
---
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', ...);
+)
+* 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

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci