linux-kernel - Re: [PATCH v2] Coccinelle: Script to replace NULL test with IS_ERR test for devm_ioremap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Mon, 25 Jul 2016 10:50:28 +0200 (CEST)
From:	Julia Lawall <julia.lawall@...6.fr>
To:	Amitoj Kaur Chawla <amitoj1606@...il.com>
cc:	Gilles Muller <Gilles.Muller@...6.fr>, nicolas.palix@...g.fr,
	mmarek@...e.com, cocci@...teme.lip6.fr,
	linux-kernel@...r.kernel.org, wsa@...-dreams.de
Subject: Re: [PATCH v2] Coccinelle: Script to replace NULL test with IS_ERR
 test for devm_ioremap_resource



On Mon, 25 Jul 2016, Amitoj Kaur Chawla wrote:

> This script detects cases which have incorrect error handling for
> devm_ioremap_resource function, employing a NULL test instead of an
> IS_ERR() test.
>
> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@...il.com>
> ---
> Changes in v2:
>         -Changed script to correct error handling instead of just
>          detecting cases of incorrect error handling
>
>  .../null/devm_ioremap_resource_test.cocci          | 66 ++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 scripts/coccinelle/null/devm_ioremap_resource_test.cocci
>
> diff --git a/scripts/coccinelle/null/devm_ioremap_resource_test.cocci b/scripts/coccinelle/null/devm_ioremap_resource_test.cocci
> new file mode 100644
> index 0000000..671970a
> --- /dev/null
> +++ b/scripts/coccinelle/null/devm_ioremap_resource_test.cocci
> @@ -0,0 +1,66 @@
> +/// Correct error handling for devm_ioremap_resource
> +///
> +// Confidence: High
> +// Copyright: (C) 2016 Amitoj Kaur Chawla
> +// Keywords: devm, devm_ioremap_resource
> +
> +virtual patch
> +virtual context
> +virtual org
> +virtual report
> +
> +@err depends on patch && !context && !org && !report@
> +expression e,e1;
> +@@
> +
> +  e = devm_ioremap_resource(...);
> +  if(
> +-    e == NULL
> ++    IS_ERR(e)
> +    )
> +    {
> +  ...
> +  return
> +- e1
> ++ PTR_ERR(e)
> +  ;
> +  }

Sorry not to have thought about this earlier, but this rule is going to
cause problems.  The ... return e1; hops over any gotos in the if branch.
So it could end up at the return at the end of the function, which might
be reachable from places in which e is not defined in the way expected.
This is likely to result in an error about modifying inconsistent paths.
The rule could be made more complicated to protect against this issue, but
given that the problem found by this semantic patch is now rare in the
first place, perhaps it is not worth it.  People can just make the change
by hand based on the report, and we can just use the original version of
the semantic patch.

julia


> +// ----------------------------------------------------------------------------
> +
> +@..._context depends on !patch && (context || org || report)@
> +expression e, e1;
> +position j0, j1;
> +@@
> +
> +  e@j0 = devm_ioremap_resource(...);
> +  if(
> +*     e == NULL
> +    )
> +    {
> +  ...
> +  return
> +*  e1@j1
> +  ;
> +  }
> +
> +// ----------------------------------------------------------------------------
> +
> +@...ipt:python err_org depends on org@
> +j0 << err_context.j0;
> +j1 << err_context.j1;
> +@@
> +
> +msg = "Incorrect error handling."
> +coccilib.org.print_todo(j0[0], msg)
> +coccilib.org.print_link(j1[0], "")
> +
> +// ----------------------------------------------------------------------------
> +
> +@...ipt:python err_report depends on report@
> +j0 << err_context.j0;
> +j1 << err_context.j1;
> +@@
> +
> +msg = "Incorrect error handling around line %s." % (j1[0].line)
> +coccilib.report.print_report(j0[0], msg)
> --
> 1.9.1
>
>