Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1689831 > unrolled thread

[PATCH] Coccinelle report script for refcounters

Back to article view | Back to linux.kernel

Contents

  [PATCH] Coccinelle report script for refcounters Elena Reshetova <elena.reshetova@intel.com> - 2017-07-18 09:50 +0200
    [PATCH] Coccinelle: add atomic_as_refcounter script Elena Reshetova <elena.reshetova@intel.com> - 2017-07-18 09:50 +0200
      Re: [PATCH] Coccinelle: add atomic_as_refcounter script Kees Cook <keescook@chromium.org> - 2017-07-18 18:30 +0200
        RE: [PATCH] Coccinelle: add atomic_as_refcounter script "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-07-19 13:00 +0200
    Re: [PATCH] Coccinelle report script for refcounters Julia Lawall <julia.lawall@lip6.fr> - 2017-07-18 10:50 +0200
      RE: [PATCH] Coccinelle report script for refcounters "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-07-18 11:40 +0200
        RE: [PATCH] Coccinelle report script for refcounters Julia Lawall <julia.lawall@lip6.fr> - 2017-07-18 14:00 +0200
          RE: [PATCH] Coccinelle report script for refcounters "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-07-18 14:30 +0200
            RE: [PATCH] Coccinelle report script for refcounters Julia Lawall <julia.lawall@lip6.fr> - 2017-07-18 17:20 +0200

#1689831 — [PATCH] Coccinelle report script for refcounters

The below script can be used to detect potential misusage
of atomic_t type and API for reference counting purposes.
Now when we have a dedicated refcount_t type and API with
security protection implemented, people should be using it
instead.

Currently it still reports many occurences since we are
nowhere near the end of our kernel-wide conversion execrise,
but hopefully after couple of cycles more, the amount of
output would be much more limited.

Each script result must be analysed manually before any
conversion, since refcount_t might not suit for certain
purposes (for example if an object is not always destroyed
upon refcounter reaching zero, if increments from zero are
allowed in the code etc.)

As we go further and get less results in output, we will
improve the pattern to detect conversion cases more precisely.

Elena Reshetova (1):
  Coccinelle: add atomic_as_refcounter script

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

-- 
2.7.4

[toc] | [next] | [standalone]

#1689839 — [PATCH] Coccinelle: add atomic_as_refcounter script

atomic_as_refcounter.cocci script allows detecting
cases when refcount_t type and API should be used
instead of atomic_t.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 scripts/coccinelle/api/atomic_as_refcounter.cocci | 102 ++++++++++++++++++++++
 1 file changed, 102 insertions(+)
 create mode 100644 scripts/coccinelle/api/atomic_as_refcounter.cocci

diff --git a/scripts/coccinelle/api/atomic_as_refcounter.cocci b/scripts/coccinelle/api/atomic_as_refcounter.cocci
new file mode 100644
index 0000000..a16d395
--- /dev/null
+++ b/scripts/coccinelle/api/atomic_as_refcounter.cocci
@@ -0,0 +1,102 @@
+// Check if refcount_t type and API should be used
+// instead of atomic_t type when dealing with refcounters
+//
+// Copyright (c) 2016-2017, Elena Reshetova, Intel Corporation
+//
+// Confidence: Moderate
+// URL: http://coccinelle.lip6.fr/
+// Options: --include-headers --very-quiet
+
+virtual report
+
+@r1 exists@
+identifier a, x, y;
+position p1, p2;
+identifier fname =~ ".*free.*";
+identifier fname2 =~ ".*destroy.*";
+identifier fname3 =~ ".*del.*";
+identifier fname4 =~ ".*queue_work.*";
+identifier fname5 =~ ".*schedule_work.*";
+identifier fname6 =~ ".*call_rcu.*";
+
+@@
+
+(
+ atomic_dec_and_test@p1(&(a)->x)
+|
+ atomic_dec_and_lock@p1(&(a)->x, ...)
+|
+ atomic_long_dec_and_lock@p1(&(a)->x, ...)
+|
+ atomic_long_dec_and_test@p1(&(a)->x)
+|
+ atomic64_dec_and_test@p1(&(a)->x)
+|
+ local_dec_and_test@p1(&(a)->x)
+)
+...
+?y=a
+...
+(
+ fname@p2(a, ...);
+|
+ fname@p2(y, ...);
+|
+ fname2@p2(...);
+|
+ fname3@p2(...);
+|
+ fname4@p2(...);
+|
+ fname5@p2(...);
+|
+ fname6@p2(...);
+)
+
+
+@script:python depends on report@
+p1 << r1.p1;
+p2 << r1.p2;
+@@
+msg = "atomic_dec_and_test variation before object free at line %s."
+coccilib.report.print_report(p1[0], msg % (p2[0].line))
+
+@r2 exists@
+identifier a, x;
+position p1;
+@@
+
+(
+atomic_add_unless(&(a)->x,-1,1)@p1
+|
+atomic_long_add_unless(&(a)->x,-1,1)@p1
+|
+atomic64_add_unless(&(a)->x,-1,1)@p1
+)
+
+@script:python depends on report@
+p1 << r2.p1;
+@@
+msg = "atomic_add_unless"
+coccilib.report.print_report(p1[0], msg)
+
+@r3 exists@
+identifier x;
+position p1;
+@@
+
+(
+x = atomic_add_return@p1(-1, ...);
+|
+x = atomic_long_add_return@p1(-1, ...);
+|
+x = atomic64_add_return@p1(-1, ...);
+)
+
+@script:python depends on report@
+p1 << r3.p1;
+@@
+msg = "x = atomic_add_return(-1, ...)"
+coccilib.report.print_report(p1[0], msg)
+
+
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1690417 — Re: [PATCH] Coccinelle: add atomic_as_refcounter script

On Tue, Jul 18, 2017 at 12:48 AM, Elena Reshetova
<elena.reshetova@intel.com> wrote:
> atomic_as_refcounter.cocci script allows detecting
> cases when refcount_t type and API should be used
> instead of atomic_t.
>
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> ---
>  scripts/coccinelle/api/atomic_as_refcounter.cocci | 102 ++++++++++++++++++++++
>  1 file changed, 102 insertions(+)
>  create mode 100644 scripts/coccinelle/api/atomic_as_refcounter.cocci
>
> diff --git a/scripts/coccinelle/api/atomic_as_refcounter.cocci b/scripts/coccinelle/api/atomic_as_refcounter.cocci
> new file mode 100644
> index 0000000..a16d395
> --- /dev/null
> +++ b/scripts/coccinelle/api/atomic_as_refcounter.cocci
> @@ -0,0 +1,102 @@
> +// Check if refcount_t type and API should be used
> +// instead of atomic_t type when dealing with refcounters
> +//
> +// Copyright (c) 2016-2017, Elena Reshetova, Intel Corporation
> +//
> +// Confidence: Moderate
> +// URL: http://coccinelle.lip6.fr/
> +// Options: --include-headers --very-quiet
> +
> +virtual report
> +
> +@r1 exists@
> +identifier a, x, y;
> +position p1, p2;
> +identifier fname =~ ".*free.*";
> +identifier fname2 =~ ".*destroy.*";
> +identifier fname3 =~ ".*del.*";
> +identifier fname4 =~ ".*queue_work.*";
> +identifier fname5 =~ ".*schedule_work.*";
> +identifier fname6 =~ ".*call_rcu.*";
> +
> +@@
> +
> +(
> + atomic_dec_and_test@p1(&(a)->x)
> [...]
> +)
> +...
> +?y=a
> +...
> +(
> + fname@p2(a, ...);
> +|
> + fname@p2(y, ...);
> +|
> [...]

Just to double check, this "?y=a" catches the seccomp case I pointed out?

        while (orig && atomic_dec_and_test(&orig->usage)) {
                struct seccomp_filter *freeme = orig;
                orig = orig->prev;
                seccomp_filter_free(freeme);
        }

Seems like it should match. Did this find anything else besides seccomp?

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1691473 — RE: [PATCH] Coccinelle: add atomic_as_refcounter script

 On Tue, Jul 18, 2017 at 12:48 AM, Elena Reshetova
> <elena.reshetova@intel.com> wrote:
> > atomic_as_refcounter.cocci script allows detecting
> > cases when refcount_t type and API should be used
> > instead of atomic_t.
> >
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > ---
> >  scripts/coccinelle/api/atomic_as_refcounter.cocci | 102
> ++++++++++++++++++++++
> >  1 file changed, 102 insertions(+)
> >  create mode 100644 scripts/coccinelle/api/atomic_as_refcounter.cocci
> >
> > diff --git a/scripts/coccinelle/api/atomic_as_refcounter.cocci
> b/scripts/coccinelle/api/atomic_as_refcounter.cocci
> > new file mode 100644
> > index 0000000..a16d395
> > --- /dev/null
> > +++ b/scripts/coccinelle/api/atomic_as_refcounter.cocci
> > @@ -0,0 +1,102 @@
> > +// Check if refcount_t type and API should be used
> > +// instead of atomic_t type when dealing with refcounters
> > +//
> > +// Copyright (c) 2016-2017, Elena Reshetova, Intel Corporation
> > +//
> > +// Confidence: Moderate
> > +// URL: http://coccinelle.lip6.fr/
> > +// Options: --include-headers --very-quiet
> > +
> > +virtual report
> > +
> > +@r1 exists@
> > +identifier a, x, y;
> > +position p1, p2;
> > +identifier fname =~ ".*free.*";
> > +identifier fname2 =~ ".*destroy.*";
> > +identifier fname3 =~ ".*del.*";
> > +identifier fname4 =~ ".*queue_work.*";
> > +identifier fname5 =~ ".*schedule_work.*";
> > +identifier fname6 =~ ".*call_rcu.*";
> > +
> > +@@
> > +
> > +(
> > + atomic_dec_and_test@p1(&(a)->x)
> > [...]
> > +)
> > +...
> > +?y=a
> > +...
> > +(
> > + fname@p2(a, ...);
> > +|
> > + fname@p2(y, ...);
> > +|
> > [...]
> 
> Just to double check, this "?y=a" catches the seccomp case I pointed out?
> 
>         while (orig && atomic_dec_and_test(&orig->usage)) {
>                 struct seccomp_filter *freeme = orig;
>                 orig = orig->prev;
>                 seccomp_filter_free(freeme);
>         }
> 

Yes, it does find the seccomp case, I was specifically testing this new addition on it. 


> Seems like it should match. Did this find anything else besides seccomp?

Yes, it found about 20 new things, but I haven't had a chance to look at them all yet.
In any case, I would really love to merge the existing conversions first (we still have about 80 patches left)
and only after add more of them. I looked at some new found cases and for example this was one:

./crypto/cryptd.c:474:38-57: atomic_dec_and_test variation before object free at line 475.

static void cryptd_skcipher_complete(struct skcipher_request *req, int err)
{
    struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
    struct cryptd_skcipher_ctx *ctx = crypto_skcipher_ctx(tfm);
    struct cryptd_skcipher_request_ctx *rctx = skcipher_request_ctx(req);
    int refcnt = atomic_read(&ctx->refcnt);

    local_bh_disable();
    rctx->complete(&req->base, err);
    local_bh_enable();

    if (err != -EINPROGRESS && refcnt && atomic_dec_and_test(&ctx->refcnt))
        crypto_free_skcipher(tfm);
}

While it isn't exactly the case I had in mind when trying to modify the pattern to work
for seccomp case, it came as a nice bonus IMO since we do want to catch these cases as well.
Overall it seems that pointers/structures can be so nicely wrapped around in some cases,
that keeping the pattern as generic as possible is a good way to go. Otherwise we might
start losing cases ( I would prefer a bit more false positives in this case instead as soon as
they are fine to manage). 

Best Regards,
Elena.

> 
> -Kees
> 
> --
> Kees Cook
> Pixel Security

[toc] | [prev] | [next] | [standalone]

#1689884

On Tue, 18 Jul 2017, Elena Reshetova wrote:

> The below script can be used to detect potential misusage
> of atomic_t type and API for reference counting purposes.
> Now when we have a dedicated refcount_t type and API with
> security protection implemented, people should be using it
> instead.
>
> Currently it still reports many occurences since we are
> nowhere near the end of our kernel-wide conversion execrise,
> but hopefully after couple of cycles more, the amount of
> output would be much more limited.
>
> Each script result must be analysed manually before any
> conversion, since refcount_t might not suit for certain
> purposes (for example if an object is not always destroyed
> upon refcounter reaching zero, if increments from zero are
> allowed in the code etc.)
>
> As we go further and get less results in output, we will
> improve the pattern to detect conversion cases more precisely.

The regexps are the best you can do?

julia

>
> Elena Reshetova (1):
>   Coccinelle: add atomic_as_refcounter script
>
>  scripts/coccinelle/api/atomic_as_refcounter.cocci | 102 ++++++++++++++++++++++
>  1 file changed, 102 insertions(+)
>  create mode 100644 scripts/coccinelle/api/atomic_as_refcounter.cocci
>
> --
> 2.7.4
>
>

[toc] | [prev] | [next] | [standalone]

#1689937

> On Tue, 18 Jul 2017, Elena Reshetova wrote:
> 
> > The below script can be used to detect potential misusage
> > of atomic_t type and API for reference counting purposes.
> > Now when we have a dedicated refcount_t type and API with
> > security protection implemented, people should be using it
> > instead.
> >
> > Currently it still reports many occurences since we are
> > nowhere near the end of our kernel-wide conversion execrise,
> > but hopefully after couple of cycles more, the amount of
> > output would be much more limited.
> >
> > Each script result must be analysed manually before any
> > conversion, since refcount_t might not suit for certain
> > purposes (for example if an object is not always destroyed
> > upon refcounter reaching zero, if increments from zero are
> > allowed in the code etc.)
> >
> > As we go further and get less results in output, we will
> > improve the pattern to detect conversion cases more precisely.
> 
> The regexps are the best you can do?

They are simple and so far they were sufficient for the purpose since
they found pretty much all the cases we are aware about. I was thinking
on working to improve the pattern later on after we merge the bulk of
conversions and I have some cycles free on that front.

What would you suggest to do instead of regexps?

Best Regards,
Elena.

> 
> julia
> 
> >
> > Elena Reshetova (1):
> >   Coccinelle: add atomic_as_refcounter script
> >
> >  scripts/coccinelle/api/atomic_as_refcounter.cocci | 102
> ++++++++++++++++++++++
> >  1 file changed, 102 insertions(+)
> >  create mode 100644 scripts/coccinelle/api/atomic_as_refcounter.cocci
> >
> > --
> > 2.7.4
> >
> >

[toc] | [prev] | [next] | [standalone]

#1690200

On Tue, 18 Jul 2017, Reshetova, Elena wrote:

> > On Tue, 18 Jul 2017, Elena Reshetova wrote:
> >
> > > The below script can be used to detect potential misusage
> > > of atomic_t type and API for reference counting purposes.
> > > Now when we have a dedicated refcount_t type and API with
> > > security protection implemented, people should be using it
> > > instead.
> > >
> > > Currently it still reports many occurences since we are
> > > nowhere near the end of our kernel-wide conversion execrise,
> > > but hopefully after couple of cycles more, the amount of
> > > output would be much more limited.
> > >
> > > Each script result must be analysed manually before any
> > > conversion, since refcount_t might not suit for certain
> > > purposes (for example if an object is not always destroyed
> > > upon refcounter reaching zero, if increments from zero are
> > > allowed in the code etc.)
> > >
> > > As we go further and get less results in output, we will
> > > improve the pattern to detect conversion cases more precisely.
> >
> > The regexps are the best you can do?
>
> They are simple and so far they were sufficient for the purpose since
> they found pretty much all the cases we are aware about. I was thinking
> on working to improve the pattern later on after we merge the bulk of
> conversions and I have some cycles free on that front.
>
> What would you suggest to do instead of regexps?

Is there anything about the definitions of these functions that indicates
why they are important?

julia

>
> Best Regards,
> Elena.
>
> >
> > julia
> >
> > >
> > > Elena Reshetova (1):
> > >   Coccinelle: add atomic_as_refcounter script
> > >
> > >  scripts/coccinelle/api/atomic_as_refcounter.cocci | 102
> > ++++++++++++++++++++++
> > >  1 file changed, 102 insertions(+)
> > >  create mode 100644 scripts/coccinelle/api/atomic_as_refcounter.cocci
> > >
> > > --
> > > 2.7.4
> > >
> > >
>

[toc] | [prev] | [next] | [standalone]

#1690226

> On Tue, 18 Jul 2017, Reshetova, Elena wrote:
> 
> > > On Tue, 18 Jul 2017, Elena Reshetova wrote:
> > >
> > > > The below script can be used to detect potential misusage
> > > > of atomic_t type and API for reference counting purposes.
> > > > Now when we have a dedicated refcount_t type and API with
> > > > security protection implemented, people should be using it
> > > > instead.
> > > >
> > > > Currently it still reports many occurences since we are
> > > > nowhere near the end of our kernel-wide conversion execrise,
> > > > but hopefully after couple of cycles more, the amount of
> > > > output would be much more limited.
> > > >
> > > > Each script result must be analysed manually before any
> > > > conversion, since refcount_t might not suit for certain
> > > > purposes (for example if an object is not always destroyed
> > > > upon refcounter reaching zero, if increments from zero are
> > > > allowed in the code etc.)
> > > >
> > > > As we go further and get less results in output, we will
> > > > improve the pattern to detect conversion cases more precisely.
> > >
> > > The regexps are the best you can do?
> >
> > They are simple and so far they were sufficient for the purpose since
> > they found pretty much all the cases we are aware about. I was thinking
> > on working to improve the pattern later on after we merge the bulk of
> > conversions and I have some cycles free on that front.
> >
> > What would you suggest to do instead of regexps?
> 
> Is there anything about the definitions of these functions that indicates
> why they are important?

I am not sure I understand the question fully. Do you mean the functions
used in the rules, such as atomic_dec_and_test() etc.?
If yes, then for example the combination of atomic_dec_and_test(&(a)->x)
on a pointer, then followed later by some kind of *free*(a) function (kfree, kmem_cache_free() etc.)
on that pointer is a quite common indicator that we are dealing with a reference counter since
they would normally free resources when counter reaches zero. 
Again, it is not a 100% indicator since I have seen weird schemes that for example
free a resource upon reaching -1, or free it in one case and don't free on another,
but such cases are more rare. 

Does this answer your questions?

Best Regards,
Elena.

> 
> julia
> 
> >
> > Best Regards,
> > Elena.
> >
> > >
> > > julia
> > >
> > > >
> > > > Elena Reshetova (1):
> > > >   Coccinelle: add atomic_as_refcounter script
> > > >
> > > >  scripts/coccinelle/api/atomic_as_refcounter.cocci | 102
> > > ++++++++++++++++++++++
> > > >  1 file changed, 102 insertions(+)
> > > >  create mode 100644 scripts/coccinelle/api/atomic_as_refcounter.cocci
> > > >
> > > > --
> > > > 2.7.4
> > > >
> > > >
> >

[toc] | [prev] | [next] | [standalone]

#1690367

On Tue, 18 Jul 2017, Reshetova, Elena wrote:

> > On Tue, 18 Jul 2017, Reshetova, Elena wrote:
> >
> > > > On Tue, 18 Jul 2017, Elena Reshetova wrote:
> > > >
> > > > > The below script can be used to detect potential misusage
> > > > > of atomic_t type and API for reference counting purposes.
> > > > > Now when we have a dedicated refcount_t type and API with
> > > > > security protection implemented, people should be using it
> > > > > instead.
> > > > >
> > > > > Currently it still reports many occurences since we are
> > > > > nowhere near the end of our kernel-wide conversion execrise,
> > > > > but hopefully after couple of cycles more, the amount of
> > > > > output would be much more limited.
> > > > >
> > > > > Each script result must be analysed manually before any
> > > > > conversion, since refcount_t might not suit for certain
> > > > > purposes (for example if an object is not always destroyed
> > > > > upon refcounter reaching zero, if increments from zero are
> > > > > allowed in the code etc.)
> > > > >
> > > > > As we go further and get less results in output, we will
> > > > > improve the pattern to detect conversion cases more precisely.
> > > >
> > > > The regexps are the best you can do?
> > >
> > > They are simple and so far they were sufficient for the purpose since
> > > they found pretty much all the cases we are aware about. I was thinking
> > > on working to improve the pattern later on after we merge the bulk of
> > > conversions and I have some cycles free on that front.
> > >
> > > What would you suggest to do instead of regexps?
> >
> > Is there anything about the definitions of these functions that indicates
> > why they are important?
>
> I am not sure I understand the question fully. Do you mean the functions
> used in the rules, such as atomic_dec_and_test() etc.?
> If yes, then for example the combination of atomic_dec_and_test(&(a)->x)
> on a pointer, then followed later by some kind of *free*(a) function (kfree, kmem_cache_free() etc.)
> on that pointer is a quite common indicator that we are dealing with a reference counter since
> they would normally free resources when counter reaches zero.
> Again, it is not a 100% indicator since I have seen weird schemes that for example
> free a resource upon reaching -1, or free it in one case and don't free on another,
> but such cases are more rare.

I just meant that the name of the function is not always reliable.  Maybe
there is some other property of the definition of the function, like being
a wrapper for kfree, that would be more reliable.  But in this case it is
quite possible that there is not.  We can try with the rule as is and see
what happens.

julia


>
> Does this answer your questions?
>
> Best Regards,
> Elena.
>
> >
> > julia
> >
> > >
> > > Best Regards,
> > > Elena.
> > >
> > > >
> > > > julia
> > > >
> > > > >
> > > > > Elena Reshetova (1):
> > > > >   Coccinelle: add atomic_as_refcounter script
> > > > >
> > > > >  scripts/coccinelle/api/atomic_as_refcounter.cocci | 102
> > > > ++++++++++++++++++++++
> > > > >  1 file changed, 102 insertions(+)
> > > > >  create mode 100644 scripts/coccinelle/api/atomic_as_refcounter.cocci
> > > > >
> > > > > --
> > > > > 2.7.4
> > > > >
> > > > >
> > >
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web