Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753442AbdGSKzB (ORCPT ); Wed, 19 Jul 2017 06:55:01 -0400 Received: from mga09.intel.com ([134.134.136.24]:43671 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752257AbdGSKy6 (ORCPT ); Wed, 19 Jul 2017 06:54:58 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,380,1496127600"; d="scan'208";a="126867502" From: "Reshetova, Elena" To: Kees Cook CC: Julia Lawall , LKML , "cocci@systeme.lip6.fr" , Gilles Muller , Nicolas Palix , Michal Marek , Hans Liljestrand Subject: RE: [PATCH] Coccinelle: add atomic_as_refcounter script Thread-Topic: [PATCH] Coccinelle: add atomic_as_refcounter script Thread-Index: AQHS/5pZYfsOITMoCkmPTRfwuYc9yKJZs7+AgAFAJBA= Date: Wed, 19 Jul 2017 10:54:54 +0000 Message-ID: <2236FBA76BA1254E88B949DDB74E612B6FF27362@IRSMSX102.ger.corp.intel.com> References: <1500364111-2192-1-git-send-email-elena.reshetova@intel.com> <1500364111-2192-2-git-send-email-elena.reshetova@intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id v6JAt4Qd022501 Content-Length: 3554 Lines: 108 On Tue, Jul 18, 2017 at 12:48 AM, Elena Reshetova > 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 > > --- > > 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