On Tue, Jul 18, 2017 at 12:48 AM, Elena Reshetova > <elena.reshet...@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.reshet...@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 _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci