Re: [Cocci] [PATCH] checkpatch: Warn on self-assignments
On Sat, Sep 05, 2020 at 10:58:29AM -0700, Joe Perches wrote:
> The uninitialized_var() macro was removed recently via
> commit 63a0895d960a ("compiler: Remove uninitialized_var() macro")
> as it's not a particularly useful warning and its use can
> "paper over real bugs".
>
> Add a checkpatch test to warn on self-assignments as a means
> to avoid compiler warnings and as a back-door mechanism to
> reproduce the old uninitialized_var macro behavior.
>
> Signed-off-by: Joe Perches
I like it! :)
Can you add a section to code style and include a link in the checkpatch
warning to it? (Feel free to just reuse the text removed from
deprecated.rst)
--
Kees Cook
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs
On Thu, Aug 27, 2020 at 09:12:06PM -0700, Joe Perches wrote:
> Perhaps something like the below with a sample conversion
> that uses single and multiple sysfs_emit uses.
On quick review, I like it. :)
> [...]
> +int sysfs_emit(char *buf, char *pos, const char *fmt, ...)
> +{
> + int len;
> + va_list args;
> +
> + WARN(pos < buf, "pos < buf\n");
> + WARN(pos - buf >= PAGE_SIZE, "pos >= PAGE_SIZE (%tu > %lu)\n",
> + pos - buf, PAGE_SIZE);
> + if (pos < buf || pos - buf >= PAGE_SIZE)
> + return 0;
This can be:
if (WARN(pos < buf, "pos < buf\n") ||
WARN(pos - buf >= PAGE_SIZE, "pos >= PAGE_SIZE (%tu > %lu)\n",
pos - buf, PAGE_SIZE))
return 0;
--
Kees Cook
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs
On Fri, Aug 28, 2020 at 12:01:34AM +0300, Denis Efremov wrote:
> Just FYI, I've send an addition to the device_attr_show.cocci script[1] to
> turn
> simple cases of snprintf (e.g. "%i") to sprintf. Looks like many developers
> would
> like it more than changing snprintf to scnprintf. As for me, I don't like the
> idea
> of automated altering of the original logic from bounded snprint to unbouded
> one
> with sprintf.
Agreed. This just makes me cringe. If the API design declares that when
a show() callback starts, buf has been allocated with PAGE_SIZE bytes,
then that's how the logic should proceed, and it should be using
scnprintf...
show(...) {
size_t remaining = PAGE_SIZE;
...
remaining -= scnprintf(buf, remaining, "fmt", var args ...);
remaining -= scnprintf(buf, remaining, "fmt", var args ...);
remaining -= scnprintf(buf, remaining, "fmt", var args ...);
return PAGE_SIZE - remaining;
}
--
Kees Cook
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs
On Thu, Aug 27, 2020 at 03:11:57PM -0700, Joe Perches wrote:
> On Thu, 2020-08-27 at 22:03 +, David Laight wrote:
> > From: Joe Perches
> > > Sent: 27 August 2020 21:30
> > ...
> > > Perhaps what's necessary is to find any
> > > appropriate .show function and change
> > > any use of strcpy/sprintf within those
> > > function to some other name.
> > >
> > > For instance:
> > >
> > > drivers/isdn/mISDN/core.c-static ssize_t name_show(struct device *dev,
> > > drivers/isdn/mISDN/core.c- struct device_attribute
> > > *attr, char *buf)
> > > drivers/isdn/mISDN/core.c-{
> > > drivers/isdn/mISDN/core.c: strcpy(buf, dev_name(dev));
> > > drivers/isdn/mISDN/core.c- return strlen(buf);
> > > drivers/isdn/mISDN/core.c-}
> > > drivers/isdn/mISDN/core.c-static DEVICE_ATTR_RO(name);
> >
> > That form ends up calculating the string length twice.
> > Better would be:
> > len = strlen(msg);
> > memcpy(buf, msg, len);
> > return len;
>
> or given clang's requirement for stpcpy
>
> return stpcpy(buf, dev_name(dev)) - buf;
>
> (I do not advocate for this ;)
Heh. And humans aren't allowed to use stpcpy() in the kernel. :)
--
Kees Cook
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks
On Thu, Jun 18, 2020 at 09:56:18PM +0200, Julia Lawall wrote: > @@ > identifier i,fld; > expression e; > @@ > > \(\(i\|e.fld\|e->fld\) \& E\) > > The e will match all of the variants you are concerned about. Ah, I see! Okay, that's good. And the "& E" part is to effectively collect it into E (as in, both the left and right of the & must match). So to do the matching from earlier: @@ identifier i, fld; expression e, ARG1, ARG2; @@ array_size(\(\(i\|e.fld\|e->fld\) \& ARG1\), ARG2); ? -- Kees Cook ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks
On Wed, Jun 17, 2020 at 08:54:03PM +0200, Julia Lawall wrote:
>
>
> On Wed, 17 Jun 2020, Kees Cook wrote:
>
> > On Mon, Jun 15, 2020 at 01:20:45PM +0300, Denis Efremov wrote:
> > > +@as@
> > > +expression E1, E2;
> > > +@@
> > > +
> > > +array_size(E1, E2)
> >
> > BTW, is there a way yet in Coccinelle to match a fully qualified (?)
> > identifier? For example, if I have two lines in C:
> >
> > A)
> > array_size(variable, 5);
> > B)
> > array_size(instance->member.size, 5);
> > C)
> > array_size(instance->member.size + 1, 5);
> > D)
> > array_size(function_call(variable), 5);
> >
> >
> > This matches A, B, C, and D:
> >
> > @@
> > expression ARG1;
> > expression ARG2;
> > @@
> >
> > array_size(ARG1, ARG2);
> >
> >
> > This matches only A:
> >
> > @@
> > identifier ARG1;
> > expression ARG2;
> > @@
> >
> > array_size(ARG1, ARG2);
> >
> >
> > How do I get something to match A and B but not C and D (i.e. I do not
> > want to match any operations, function calls, etc, only a variable,
> > which may be identified through dereference, array index, or struct
> > member access.)
>
> \(i\|e.fld\|e->fld\)
>
> would probably do what you want. It will also match cases where e is a
> function/macr call, but that is unlikely.
>
> If you want a single metavariable that contains the whole thing, you can
> have an expression metavariable E and then write:
>
> \(\(i\|e.fld\|e->fld\) \& E\)
Can you give an example of how that would look for an @@ section?
The problem I have is that I don't know the depth or combination of such
metavariables. There are a lot of combinations:
a
a.b
a.b.c
a.b.c.d
a.b.c->d
a.b->c
a.b->c.d
a.b->c->d
a->b
a->b.c
a->b.c.d
a->b.c->d
a->b->c
a->b->c.d
a->b->c->d
...
--
Kees Cook
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks
On Mon, Jun 15, 2020 at 01:20:45PM +0300, Denis Efremov wrote: > +@as@ > +expression E1, E2; > +@@ > + > +array_size(E1, E2) BTW, is there a way yet in Coccinelle to match a fully qualified (?) identifier? For example, if I have two lines in C: A) array_size(variable, 5); B) array_size(instance->member.size, 5); C) array_size(instance->member.size + 1, 5); D) array_size(function_call(variable), 5); This matches A, B, C, and D: @@ expression ARG1; expression ARG2; @@ array_size(ARG1, ARG2); This matches only A: @@ identifier ARG1; expression ARG2; @@ array_size(ARG1, ARG2); How do I get something to match A and B but not C and D (i.e. I do not want to match any operations, function calls, etc, only a variable, which may be identified through dereference, array index, or struct member access.) -- Kees Cook ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks
On Mon, Jun 15, 2020 at 01:20:45PM +0300, Denis Efremov wrote: > Detect an opencoded expression that is used before or after > array_size()/array3_size()/struct_size() to compute the same size. > > Cc: Kees Cook > Signed-off-by: Denis Efremov Oh, very cool! How much does this find currently? -- Kees Cook ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH V2] kernel/hung_task.c: Introduce sysctl to print all traces when a hung task is detected
On Tue, Mar 24, 2020 at 09:45:40AM -0300, Guilherme G. Piccoli wrote: > Thanks Randy and Vlastimil for the comments. I really liked your > approach Vlastimil, I agree that we have no reason to not have a generic > sysctl setting via cmdline mechanism - I'll rework this patch removing > the kernel parameter (same for other patch I just submitted). I've been thinking we'll likely want to have a big patch series that removes all the old "duplicate" boot params and adds some kind of "alias" mechanism. Vlastimil, have you happened to keep a list of other "redundant" boot params you've noticed in the kernel? I bet there are a lot. :) -- Kees Cook ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH V2] kernel/hung_task.c: Introduce sysctl to print all traces when a hung task is detected
On Mon, Mar 23, 2020 at 06:46:18PM -0300, Guilherme G. Piccoli wrote:
> Commit 401c636a0eeb ("kernel/hung_task.c: show all hung tasks before panic")
> introduced a change in that we started to show all CPUs backtraces when a
> hung task is detected _and_ the sysctl/kernel parameter "hung_task_panic"
> is set. The idea is good, because usually when observing deadlocks (that
> may lead to hung tasks), the culprit is another task holding a lock and
> not necessarily the task detected as hung.
>
> The problem with this approach is that dumping backtraces is a slightly
> expensive task, specially printing that on console (and specially in many
> CPU machines, as servers commonly found nowadays). So, users that plan to
> collect a kdump to investigate the hung tasks and narrow down the deadlock
> definitely don't need the CPUs backtrace on dmesg/console, which will delay
> the panic and pollute the log (crash tool would easily grab all CPUs traces
> with 'bt -a' command).
> Also, there's the reciprocal scenario: some users may be interested in
> seeing the CPUs backtraces but not have the system panic when a hung task
> is detected. The current approach hence is almost as embedding a policy in
> the kernel, by forcing the CPUs backtraces' dump (only) on hung_task_panic.
>
> This patch decouples the panic event on hung task from the CPUs backtraces
> dump, by creating (and documenting) a new sysctl/kernel parameter called
> "hung_task_all_cpu_backtrace", analog to the approach taken on soft/hard
> lockups, that have both a panic and an "all_cpu_backtrace" sysctl to allow
> individual control. The new mechanism for dumping the CPUs backtraces on
> hung task detection respects "hung_task_warnings" by not dumping the
> traces in case there's no warnings left.
>
> Cc: Tetsuo Handa
> Signed-off-by: Guilherme G. Piccoli
> ---
>
>
> V2: Followed suggestions from Kees and Tetsuo (and other grammar
> improvements). Also, followed Tetsuo suggestion to itereate kernel
> testing community - but I don't really know a ML for that, so I've
> CCed Coccinelle community and kernel-api ML.
>
> Also, Tetsuo suggested that this option could be default to 1 - I'm
> open to it, but given it is only available if hung_task panic is set
> as of now and the goal of this patch is give users more flexibility,
> I vote to keep default as 0. I can respin a V3 in case more people
> want to see it enabled by default. Thanks in advance for the review!
Yeah, most things like this we've tried to be conservative. I'd like to
see it stay zero too.
Reviewed-by: Kees Cook
-Kees
> Cheers,
>
> Guilherme
>
>
> .../admin-guide/kernel-parameters.txt | 6
> Documentation/admin-guide/sysctl/kernel.rst | 15 ++
> include/linux/sched/sysctl.h | 7 +
> kernel/hung_task.c| 30 +--
> kernel/sysctl.c | 11 +++
> 5 files changed, 67 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt
> b/Documentation/admin-guide/kernel-parameters.txt
> index c07815d230bc..7a14caac6c94 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1453,6 +1453,12 @@
> x86-64 are 2M (when the CPU supports "pse") and 1G
> (when the CPU supports the "pdpe1gb" cpuinfo flag).
>
> + hung_task_all_cpu_backtrace=
> + [KNL] Should kernel generate backtraces on all cpus
> + when a hung task is detected. Defaults to 0 and can
> + be controlled by hung_task_all_cpu_backtrace sysctl.
> + Format:
> +
> hung_task_panic=
> [KNL] Should the hung task detector generate panics.
> Format:
> diff --git a/Documentation/admin-guide/sysctl/kernel.rst
> b/Documentation/admin-guide/sysctl/kernel.rst
> index def074807cee..8b4ff69d2348 100644
> --- a/Documentation/admin-guide/sysctl/kernel.rst
> +++ b/Documentation/admin-guide/sysctl/kernel.rst
> @@ -40,6 +40,7 @@ show up in /proc/sys/kernel:
> - hotplug
> - hardlockup_all_cpu_backtrace
> - hardlockup_panic
> +- hung_task_all_cpu_backtrace
> - hung_task_panic
> - hung_task_check_count
> - hung_task_timeout_secs
> @@ -338,6 +339,20 @@ Path for the hotplug policy agent.
> Default value is "/sbin/hotplug".
>
>
> +hung_task_all_cpu_backtrace:
> +
> +
> +If this option is set, the kernel will send an NMI to all CPUs to dump
> +their backtraces when a hung t
[Cocci] Confused by regex usage
Hi, I have this .cocci: /// Unchecked use of snprintf() return values can lead to bugs, especially /// when returned or used to increment a buffer position. This is because /// snprintf() can return how much it WOULD have written, had it not run out /// of space. Instead, use scnprintf() which will report only how much was /// actually written, keeping any overflows from happening. /// // Confidence: Moderate // Copyright: (C) 2018 Kees Cook, Google. GPLv2. // URL: http://coccinelle.lip6.fr/ // Options: --all-includes --include-headers virtual patch @sum_patch depends on patch exists@ expression LEN, BUF, SIZE; identifier FUNC !~ "^\(snprintf\|scnprintf\)$"; @@ ( LEN = -snprintf +scnprintf (BUF, SIZE, ...); | LEN += -snprintf +scnprintf (BUF + LEN, SIZE - LEN, ...); ) ... when != LEN > SIZE when != LEN >= SIZE when any ( return LEN; | FUNC(..., <+...LEN...+>, ...) ) It matches net/sunrpc/addr.c: --- net/sunrpc/addr.c +++ /tmp/cocci-output-43547-394eff-addr.c @@ -79,7 +79,7 @@ static size_t rpc_ntop6(const struct soc if (sin6->sin6_scope_id == 0) return len; - rc = snprintf(scopebuf, sizeof(scopebuf), "%c%u", + rc = scnprintf(scopebuf, sizeof(scopebuf), "%c%u", IPV6_SCOPE_DELIMITER, sin6->sin6_scope_id); if (unlikely((size_t)rc > sizeof(scopebuf))) return 0; But I can't figure out why. I was trying to exclude matches against sc?nprintf, but the FUNC line appears to make things crazy and break the "when" check. If I remove the FUNC portion of the pattern, it's fine, but then I miss a bunch of cases I *do* want to catch, etc. Thanks! -- Kees Cook ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] =~ runtime improvements?
On Sun, Sep 30, 2018 at 8:40 AM, Julia Lawall wrote: > > > On Sun, 30 Sep 2018, Lars-Peter Clausen wrote: > >> On 09/27/2018 08:51 PM, Kees Cook wrote: >> > Hi, >> > >> > This .cocci takes a VERY long time to run against the kernel, and I'd >> > love to know what I could do to improve it. I assume it's related to >> > the use of the "=~" operand: >> > >> >> Maybe I'm missing something, but do you need all of those variations? An >> expression should match an identifier. I'd expect ((ECOUNT)) * >> ((ESTRIDE)) * ((ESIZE)) matches the superset of all the other statements >> with ICOUNT, ISIZE and ISTRIDE in them. So you only need two rules one >> for array_size and one for array3_size. > > I agree about the indentifiers and expressions, although he also needs > some rules for the constant case. I had to go progressively to exclude cases in an attempt to isolate individual factors. For example: E1 * E2 will match: var1 * var2 * var3 In order to make a best-effort at extracting the multiplication factors, I need to go in order from constants (ignore) to identifiers (explicitly correct) to expressions (may overly match) But yes, it seems the problem is mainly the "..." part, which is unavoidable. -Kees -- Kees Cook Pixel Security ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] =~ runtime improvements?
Hi, This .cocci takes a VERY long time to run against the kernel, and I'd love to know what I could do to improve it. I assume it's related to the use of the "=~" operand: // Replace multi-factor out-of-line products with array_size() usage. @@ identifier alloc =~ ".*alloc.*"; constant C1, C2, C3; identifier ISTRIDE, ISIZE, ICOUNT; expression ESTRIDE, ESIZE, ECOUNT; expression PRODUCT, OTHER; @@ ( PRODUCT = ((C1)) * ((C2)) * ((C3)) | PRODUCT = ((C1)) * ((C2)) | - PRODUCT = ((ICOUNT)) * ((ISTRIDE)) * ((ISIZE)) + PRODUCT = array3_size(ICOUNT, ISTRIDE, ISIZE) | - PRODUCT = ((ICOUNT)) * ((ISTRIDE)) * ((ESIZE)) + PRODUCT = array3_size(ICOUNT, ISTRIDE, ESIZE) | - PRODUCT = ((ICOUNT)) * ((ESTRIDE)) * ((ESIZE)) + PRODUCT = array3_size(ICOUNT, ESTRIDE, ESIZE) | - PRODUCT = ((ECOUNT)) * ((ESTRIDE)) * ((ESIZE)) + PRODUCT = array3_size(ECOUNT, ESTRIDE, ESIZE) | - PRODUCT = ((ICOUNT)) * ((ISIZE)) + PRODUCT = array_size(ICOUNT, ISTRIDE, ISIZE) | - PRODUCT = ((ICOUNT)) * ((ESIZE)) + PRODUCT = array_size(ICOUNT, ESIZE) | - PRODUCT = ((ECOUNT)) * ((ESIZE)) + PRODUCT = array_size(ECOUNT, ESIZE) ) ... when != PRODUCT = OTHER alloc(..., PRODUCT, ...) Thanks! -Kees -- Kees Cook Pixel Security ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [Fwd: Re: [PATCH] rtc: sun6i: Use struct_size() in kzalloc()]
On Thu, Aug 23, 2018 at 3:21 PM, Joe Perches wrote:
> On Thu, 2018-08-23 at 18:13 -0400, Julia Lawall wrote:
>>
>> On Thu, 23 Aug 2018, Kees Cook wrote:
>>
>> (a + b) * c
>>
>> It will consider a pattern with the parentheses removed, but that pattern
>> won't match anything, because real trees that consist of a + b * c are
>> parsed in a different way.
>
> spatch 1.0.4 doesn't seem to:
>
> $ spatch --version
> spatch version 1.0.4 with Python support and with PCRE support
> $ cat match_mul.cocci
> @@
> expression a, b, c;
> int d;
> @@
>
> * d = a * b + c;
But "(a * b) + c" for the rule does:
$ cat isomorphisms.c
#include
#include
int main(int argc, char *argv[])
{
int a, b, c;
a = atoi(argv[1]);
b = atoi(argv[2]);
c = atoi(argv[3]);
printf("%d\n", a + b * c);
printf("%d\n", (a + b) * c);
printf("%d\n", a + (b * c));
printf("%d\n", b * c + a);
printf("%d\n", b * (c + a));
printf("%d\n", (b * c) + a);
return 0;
}
$ cat match.cocci
@@
identifier A, B, C;
@@
* (A * B) + C
$ spatch -sp-file match.cocci isomorphisms.c
init_defs_builtins: /usr/lib/coccinelle/standard.h
HANDLING: isomorphisms.c
diff =
--- isomorphisms.c
+++ /tmp/cocci-output-8402-870fd6-isomorphisms.c
@@ -9,12 +9,8 @@ int main(int argc, char *argv[])
b = atoi(argv[2]);
c = atoi(argv[3]);
- printf("%d\n", a + b * c);
printf("%d\n", (a + b) * c);
- printf("%d\n", a + (b * c));
- printf("%d\n", b * c + a);
printf("%d\n", b * (c + a));
- printf("%d\n", (b * c) + a);
return 0;
}
So it sounds like I should put parens around all kinds of things in my rules...
-Kees
--
Kees Cook
Pixel Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [Fwd: Re: [PATCH] rtc: sun6i: Use struct_size() in kzalloc()]
On Thu, Aug 23, 2018 at 3:00 PM, Julia Lawall wrote:
>
>
> On Thu, 23 Aug 2018, Kees Cook wrote:
>
>> On Thu, Aug 23, 2018 at 2:48 PM, Joe Perches wrote:
>> > Forwarding a question about coccinelle and isomorphisms from Kees Cook
>> >
>> > -- Forwarded message --
>> > From: Kees Cook
>> > To: "Gustavo A. R. Silva"
>> > Cc: Alessandro Zummo , Alexandre Belloni
>> > , Maxime Ripard
>> > , Chen-Yu Tsai ,
>> > linux-...@vger.kernel.org, linux-arm-kernel
>> > , LKML
>> > Bcc:
>> > Date: Thu, 23 Aug 2018 13:56:35 -0700
>> > Subject: Re: [PATCH] rtc: sun6i: Use struct_size() in kzalloc()
>> > On Thu, Aug 23, 2018 at 11:51 AM, Gustavo A. R. Silva
>> > wrote:
>> >> One of the more common cases of allocation size calculations is finding
>> >> the size of a structure that has a zero-sized array at the end, along
>> >> with memory for some number of elements for that array. For example:
>> >>
>> >> struct foo {
>> >> int stuff;
>> >> void *entry[];
>> >> };
>> >>
>> >> instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count,
>> >> GFP_KERNEL);
>> >>
>> >> Instead of leaving these open-coded and prone to type mistakes, we can
>> >> now use the new struct_size() helper:
>> >>
>> >> instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);
>> >>
>> >> Signed-off-by: Gustavo A. R. Silva
>> >> ---
>> >> drivers/rtc/rtc-sun6i.c | 3 +--
>> >> 1 file changed, 1 insertion(+), 2 deletions(-)
>> >>
>> >> diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
>> >> index 2cd5a7b..fe07310 100644
>> >> --- a/drivers/rtc/rtc-sun6i.c
>> >> +++ b/drivers/rtc/rtc-sun6i.c
>> >> @@ -199,8 +199,7 @@ static void __init sun6i_rtc_clk_init(struct
>> >> device_node *node)
>> >> if (!rtc)
>> >> return;
>> >>
>> >> - clk_data = kzalloc(sizeof(*clk_data) + (sizeof(*clk_data->hws) *
>> >> 2),
>> >> - GFP_KERNEL);
>> >> + clk_data = kzalloc(struct_size(clk_data, hws, 2), GFP_KERNEL);
>> >> if (!clk_data) {
>> >> kfree(rtc);
>> >> return;
>> >
>> > This looks like entirely correct to me, but I'm surprised the
>> > Coccinelle script didn't discover this. I guess the isomorphisms don't
>> > cover the parenthesis?
>>
>> I had this:
>>
>> @@
>> identifier alloc =~
>> "kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc";
>> identifier VAR, ELEMENT;
>> expression COUNT;
>> @@
>>
>> - alloc(sizeof(*VAR) + COUNT * sizeof(*VAR->ELEMENT)
>> + alloc(struct_size(VAR, ELEMENT, COUNT)
>> , ...)
>>
>> But I needed to explicitly change the rule to:
>>
>> (
>> - alloc(sizeof(*VAR) + COUNT * sizeof(*VAR->ELEMENT)
>> + alloc(struct_size(VAR, ELEMENT, COUNT)
>> , ...)
>> |
>> - alloc(sizeof(*VAR) + (COUNT * sizeof(*VAR->ELEMENT))
>> + alloc(struct_size(VAR, ELEMENT, COUNT)
>> , ...)
>> )
>>
>> to add the ()s. I would expect arithmetic commutative expressions to
>> match? But I had to add parens?
>
> Isomorphisms don't add parens. They only remove them. If they added
> them, you would end up with the possibility of having them everywhere, in
> all permutations, which would be slow and useless.
Would a rule for:
a + (b * c)
match:
a + b * c
?
-Kees
--
Kees Cook
Pixel Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [Fwd: Re: [PATCH] rtc: sun6i: Use struct_size() in kzalloc()]
On Thu, Aug 23, 2018 at 2:48 PM, Joe Perches wrote:
> Forwarding a question about coccinelle and isomorphisms from Kees Cook
>
> -- Forwarded message --
> From: Kees Cook
> To: "Gustavo A. R. Silva"
> Cc: Alessandro Zummo , Alexandre Belloni
> , Maxime Ripard ,
> Chen-Yu Tsai , linux-...@vger.kernel.org, linux-arm-kernel
> , LKML
> Bcc:
> Date: Thu, 23 Aug 2018 13:56:35 -0700
> Subject: Re: [PATCH] rtc: sun6i: Use struct_size() in kzalloc()
> On Thu, Aug 23, 2018 at 11:51 AM, Gustavo A. R. Silva
> wrote:
>> One of the more common cases of allocation size calculations is finding
>> the size of a structure that has a zero-sized array at the end, along
>> with memory for some number of elements for that array. For example:
>>
>> struct foo {
>> int stuff;
>> void *entry[];
>> };
>>
>> instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);
>>
>> Instead of leaving these open-coded and prone to type mistakes, we can
>> now use the new struct_size() helper:
>>
>> instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);
>>
>> Signed-off-by: Gustavo A. R. Silva
>> ---
>> drivers/rtc/rtc-sun6i.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
>> index 2cd5a7b..fe07310 100644
>> --- a/drivers/rtc/rtc-sun6i.c
>> +++ b/drivers/rtc/rtc-sun6i.c
>> @@ -199,8 +199,7 @@ static void __init sun6i_rtc_clk_init(struct device_node
>> *node)
>> if (!rtc)
>> return;
>>
>> - clk_data = kzalloc(sizeof(*clk_data) + (sizeof(*clk_data->hws) * 2),
>> - GFP_KERNEL);
>> + clk_data = kzalloc(struct_size(clk_data, hws, 2), GFP_KERNEL);
>> if (!clk_data) {
>> kfree(rtc);
>> return;
>
> This looks like entirely correct to me, but I'm surprised the
> Coccinelle script didn't discover this. I guess the isomorphisms don't
> cover the parenthesis?
I had this:
@@
identifier alloc =~
"kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc";
identifier VAR, ELEMENT;
expression COUNT;
@@
- alloc(sizeof(*VAR) + COUNT * sizeof(*VAR->ELEMENT)
+ alloc(struct_size(VAR, ELEMENT, COUNT)
, ...)
But I needed to explicitly change the rule to:
(
- alloc(sizeof(*VAR) + COUNT * sizeof(*VAR->ELEMENT)
+ alloc(struct_size(VAR, ELEMENT, COUNT)
, ...)
|
- alloc(sizeof(*VAR) + (COUNT * sizeof(*VAR->ELEMENT))
+ alloc(struct_size(VAR, ELEMENT, COUNT)
, ...)
)
to add the ()s. I would expect arithmetic commutative expressions to
match? But I had to add parens?
-Kees
--
Kees Cook
Pixel Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] Something like --exclude?
Hi, I normally use "--dir ." when doing recursive runs in the kernel tree, but I want to avoid changes in the tools/ subdirectory. I can't find anything like --exclude, and --dir can't be specified multiple times. Is there an existing solution I'm missing? Thanks! -Kees -- Kees Cook Pixel Security ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH] Coccinelle: add atomic_as_refcounter script
On Fri, Sep 1, 2017 at 2:40 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 > Acked-by: Julia Lawall Reviewed-by: Kees Cook Oops, I think this got lost. Who can take this patch? I thought Julia ran the scripts/coccinelle/ tree, but looking at git log, it looks more like it's Masahiro? Either way, let's get this in the tree. Who can take it? Thanks! -Kees > --- > scripts/coccinelle/api/atomic_as_refcounter.cocci | 131 > ++ > 1 file changed, 131 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 000..bfa880d > --- /dev/null > +++ b/scripts/coccinelle/api/atomic_as_refcounter.cocci > @@ -0,0 +1,131 @@ > +// 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; > +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) > +) > +... > +( > + fname@p2(a, ...); > +| > + 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)) > + > +@r4 exists@ > +identifier a, x, y; > +position p1, p2; > +identifier fname =~ ".*free.*"; > + > +@@ > + > +( > + 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(y, ...); > + > + > +@script:python depends on report@ > +p1 << r4.p1; > +p2 << r4.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 > -- Kees Cook Pixel Security ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Weird whitespace behavior?
On Mon, Jun 11, 2018 at 4:17 PM, Kees Cook wrote: > Hi, > > I've been doing some large treewide changes to the allocators, and I > notice that Coccinelle does something odd for a specific case. I have > two scripts, one operating on kmalloc() and one operating on > devm_kmalloc(). They are identical script except for the function > names, however, while kmalloc produces patches like this: > > - foo = kmalloc(a * b, gfp); > + foo = kmalloc_array(a, b, gfp); > > the devm_kmalloc one produces: > > - foo = devm_kmalloc(handle, a * b, gfp); > + foo =devm_kmalloc_array(a, b, gfp); > > I can't figure out why the space after "=" is missing and have been > manually fixing it up... And while I'm at it... line breaks aren't working like I'd expect: - collection = kmalloc(sizeof(struct hid_collection) * - parser->device->collection_size * 2, GFP_KERNEL); + collection = kmalloc(array3_size(sizeof(struct hid_collection), parser->device->collection_size, 2), + GFP_KERNEL); Why wasn't array3_size() correctly line wrapped? -Kees -- Kees Cook Pixel Security ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] Weird whitespace behavior?
Hi, I've been doing some large treewide changes to the allocators, and I notice that Coccinelle does something odd for a specific case. I have two scripts, one operating on kmalloc() and one operating on devm_kmalloc(). They are identical script except for the function names, however, while kmalloc produces patches like this: - foo = kmalloc(a * b, gfp); + foo = kmalloc_array(a, b, gfp); the devm_kmalloc one produces: - foo = devm_kmalloc(handle, a * b, gfp); + foo =devm_kmalloc_array(a, b, gfp); I can't figure out why the space after "=" is missing and have been manually fixing it up... -Kees -- Kees Cook Pixel Security ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
On Thu, May 3, 2018 at 5:36 PM, Kees Cook <keesc...@chromium.org> wrote: > On Thu, May 3, 2018 at 4:00 PM, Rasmus Villemoes > <li...@rasmusvillemoes.dk> wrote: >> On 2018-05-01 19:00, Kees Cook wrote: >>> On Mon, Apr 30, 2018 at 2:29 PM, Rasmus Villemoes >>> <li...@rasmusvillemoes.dk> wrote: >>>> >>>> gcc 5.1+ (I think) have the __builtin_OP_overflow checks that should >>>> generate reasonable code. Too bad there's no completely generic >>>> check_all_ops_in_this_expression(a+b*c+d/e, or_jump_here). Though it's >>>> hard to define what they should be checked against - probably would >>>> require all subexpressions (including the variables themselves) to have >>>> the same type. >>>> >>>> plug: https://lkml.org/lkml/2015/7/19/358 >>> >>> That's a very nice series. Why did it never get taken? >> >> Well, nobody seemed particularly interested, and then >> https://lkml.org/lkml/2015/10/28/215 happened... but he did later seem >> to admit that it could be useful for the multiplication checking, and >> that "the gcc interface for multiplication overflow is fine". > > Oh, excellent. Thank you for that pointer! That conversation covered a > lot of ground. I need to think a little more about how to apply the > thoughts there with the kmalloc() needs and the GPU driver needs... > >> I still think even for unsigned types overflow checking can be subtle. E.g. >> >> u32 somevar; >> >> if (somevar + sizeof(foo) < somevar) >> return -EOVERFLOW; >> somevar += sizeof(this); >> >> is broken, because the LHS is promoted to unsigned long/size_t, then so >> is the RHS for the comparison, and the comparison is thus always false >> (on 64bit). It gets worse if the two types are more "opaque", and in any >> case it's not always easy to verify at a glance that the types are the >> same, or at least that the expression of the widest type is on the RHS. > > That's an excellent example, yes. (And likely worth including in the > commit log somewhere.) > >> >>> It seems to do the right things quite correctly. >> >> Yes, I wouldn't suggest it without the test module verifying corner >> cases, and checking it has the same semantics whether used with old or >> new gcc. >> >> Would you shepherd it through if I updated the patches and resent? > > Yes, though we may need reworking if we actually want to do the > try/catch style (since that was talked about with GPU stuff too...) > > Either way, yes, a refresh would be lovely! :) Whatever the case, I think we need to clean up all the kmalloc() math anyway. As mentioned earlier, there are a handful of more complex cases, but the vast majority are just A * B. I've put up a series here now, and I'll send it out soon. I want to think more about 3-factor products, addition, etc: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/kmalloc/2-factor-products The commit logs need more details (i.e. about making constants the second argument for optimal compiler results, etc), but there's a Coccinelle-generated first pass. -Kees -- Kees Cook Pixel Security ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
On Thu, May 3, 2018 at 4:00 PM, Rasmus Villemoes <li...@rasmusvillemoes.dk> wrote: > On 2018-05-01 19:00, Kees Cook wrote: >> On Mon, Apr 30, 2018 at 2:29 PM, Rasmus Villemoes >> <li...@rasmusvillemoes.dk> wrote: >>> >>> gcc 5.1+ (I think) have the __builtin_OP_overflow checks that should >>> generate reasonable code. Too bad there's no completely generic >>> check_all_ops_in_this_expression(a+b*c+d/e, or_jump_here). Though it's >>> hard to define what they should be checked against - probably would >>> require all subexpressions (including the variables themselves) to have >>> the same type. >>> >>> plug: https://lkml.org/lkml/2015/7/19/358 >> >> That's a very nice series. Why did it never get taken? > > Well, nobody seemed particularly interested, and then > https://lkml.org/lkml/2015/10/28/215 happened... but he did later seem > to admit that it could be useful for the multiplication checking, and > that "the gcc interface for multiplication overflow is fine". Oh, excellent. Thank you for that pointer! That conversation covered a lot of ground. I need to think a little more about how to apply the thoughts there with the kmalloc() needs and the GPU driver needs... > I still think even for unsigned types overflow checking can be subtle. E.g. > > u32 somevar; > > if (somevar + sizeof(foo) < somevar) > return -EOVERFLOW; > somevar += sizeof(this); > > is broken, because the LHS is promoted to unsigned long/size_t, then so > is the RHS for the comparison, and the comparison is thus always false > (on 64bit). It gets worse if the two types are more "opaque", and in any > case it's not always easy to verify at a glance that the types are the > same, or at least that the expression of the widest type is on the RHS. That's an excellent example, yes. (And likely worth including in the commit log somewhere.) > >> It seems to do the right things quite correctly. > > Yes, I wouldn't suggest it without the test module verifying corner > cases, and checking it has the same semantics whether used with old or > new gcc. > > Would you shepherd it through if I updated the patches and resent? Yes, though we may need reworking if we actually want to do the try/catch style (since that was talked about with GPU stuff too...) Either way, yes, a refresh would be lovely! :) -Kees -- Kees Cook Pixel Security ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
On Mon, Apr 30, 2018 at 2:29 PM, Rasmus Villemoes
<li...@rasmusvillemoes.dk> wrote:
> On 2018-04-30 22:16, Matthew Wilcox wrote:
>> On Mon, Apr 30, 2018 at 12:02:14PM -0700, Kees Cook wrote:
>>>
>>> Getting the constant ordering right could be part of the macro
>>> definition, maybe? i.e.:
>>>
>>> static inline void *kmalloc_ab(size_t a, size_t b, gfp_t flags)
>>> {
>>> if (__builtin_constant_p(a) && a != 0 && \
>>> b > SIZE_MAX / a)
>>> return NULL;
>>> else if (__builtin_constant_p(b) && b != 0 && \
>>>a > SIZE_MAX / b)
>>> return NULL;
>>>
>>> return kmalloc(a * b, flags);
>>> }
>>
>> Ooh, if neither a nor b is constant, it just didn't do a check ;-( This
>> stuff is hard.
>>
>>> (I just wish C had a sensible way to catch overflow...)
>>
>> Every CPU I ever worked with had an "overflow" bit ... do we have a
>> friend on the C standards ctte who might figure out a way to let us
>> write code that checks it?
>
> gcc 5.1+ (I think) have the __builtin_OP_overflow checks that should
> generate reasonable code. Too bad there's no completely generic
> check_all_ops_in_this_expression(a+b*c+d/e, or_jump_here). Though it's
> hard to define what they should be checked against - probably would
> require all subexpressions (including the variables themselves) to have
> the same type.
>
> plug: https://lkml.org/lkml/2015/7/19/358
That's a very nice series. Why did it never get taken? It seems to do
the right things quite correctly.
Daniel, while this isn't a perfect solution, is this something you'd
use in graphics-land?
-Kees
--
Kees Cook
Pixel Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
On Mon, Apr 30, 2018 at 1:16 PM, Matthew Wilcox <wi...@infradead.org> wrote:
> On Mon, Apr 30, 2018 at 12:02:14PM -0700, Kees Cook wrote:
>> For any longer multiplications, I've only found[1]:
>>
>> drivers/staging/rtl8188eu/os_dep/osdep_service.c: void **a =
>> kzalloc(h * sizeof(void *) + h * w * size, GFP_KERNEL);
>
> That's pretty good, although it's just an atrocious vendor driver and
> it turns out all of those things are constants, and it'd be far better
> off with just declaring an array. I bet they used to declare one on
> the stack ...
Yeah, it was just a quick hack to look for stuff.
>
>> At the end of the day, though, I don't really like having all these
>> different names...
>>
>> kmalloc(), kmalloc_array(), kmalloc_ab_c(), kmalloc_array_3d()
>>
>> with their "matching" zeroing function:
>>
>> kzalloc(), kcalloc(), kzalloc_ab_c(), kmalloc_array_3d(..., gfp | __GFP_ZERO)
>
> Yes, it's not very regular.
>
>> For the multiplication cases, I wonder if we could just have:
>>
>> kmalloc_multN(gfp, a, b, c, ...)
>> kzalloc_multN(gfp, a, b, c, ...)
>>
>> and we can replace all kcalloc() users with kzalloc_mult2(), all
>> kmalloc_array() users with kmalloc_mult2(), the abc uses with
>> kmalloc_mult3().
>
> I'm reluctant to do away with kcalloc() as it has the obvious heritage
> from user-space calloc() with the addition of GFP flags.
But it encourages misuse with calloc(N * M, gfp) ... if we removed
calloc and kept k[mz]alloc_something(gfp, a, b, c...) I think we'd
have better adoption.
>> That said, I *do* like kmalloc_struct() as it's a very common pattern...
>
> Thanks! And way harder to misuse than kmalloc_ab_c().
Yes, quite so. It's really why I went with kmalloc_array_3d(), but now
I'm thinking better of it...
>> Or maybe, just leave the pattern in the name? kmalloc_ab(),
>> kmalloc_abc(), kmalloc_ab_c(), kmalloc_ab_cd() ?
>>
>> Getting the constant ordering right could be part of the macro
>> definition, maybe? i.e.:
>>
>> static inline void *kmalloc_ab(size_t a, size_t b, gfp_t flags)
>> {
>> if (__builtin_constant_p(a) && a != 0 && \
>> b > SIZE_MAX / a)
>> return NULL;
>> else if (__builtin_constant_p(b) && b != 0 && \
>>a > SIZE_MAX / b)
>> return NULL;
>>
>> return kmalloc(a * b, flags);
>> }
>
> Ooh, if neither a nor b is constant, it just didn't do a check ;-( This
> stuff is hard.
Yup, quite true. Obviously not the final form. ;) I meant to
illustrate that we could do compile-time tricks to reorder the
division in an efficient manner.
>> (I just wish C had a sensible way to catch overflow...)
>
> Every CPU I ever worked with had an "overflow" bit ... do we have a
> friend on the C standards ctte who might figure out a way to let us
> write code that checks it?
On the CPU it's not retained across multiple calculations. And the
type matters too. This came up recently in a separate thread too:
http://openwall.com/lists/kernel-hardening/2018/03/26/4
>> [1] git grep -E 'alloc\([^,]+[^(]\*[^)][^,]+[^(]\*[^)][^,]+[^(]\*[^)][^,]+,'
>
> I'm impressed, but it's not going to catch
>
> veryLongPointerNameThatsMeaningfulToMe = kmalloc(initialSize +
> numberOfEntries * entrySize + someOtherThing * yourMum,
> GFP_KERNEL);
Right, it wasn't meant to be exhaustive. I just included it in case
anyone wanted to go grepping around for themselves.
-Kees
--
Kees Cook
Pixel Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
On Sun, Apr 29, 2018 at 1:30 PM, Matthew Wilcox <wi...@infradead.org> wrote:
> On Sun, Apr 29, 2018 at 09:59:27AM -0700, Kees Cook wrote:
>> Did this ever happen?
>
> Not yet. I brought it up at LSFMM, and I'll repost the patches soon.
>
>> I'd also like to see kmalloc_array_3d() or
>> something that takes three size arguments. We have a lot of this
>> pattern too:
>>
>> kmalloc(sizeof(foo) * A * B, gfp...)
>>
>> And we could turn that into:
>>
>> kmalloc_array_3d(sizeof(foo), A, B, gfp...)
>
> Are either of A or B constant? Because if so, we could just use
> kmalloc_array. If not, then kmalloc_array_3d becomes a little more
> expensive than kmalloc_array because we have to do a divide at runtime
> instead of compile-time. that's still better than allocating too few
> bytes, of course.
Yeah, getting the order of the division is nice. Some thoughts below...
>
> I'm wondering how far down the abc + ab + ac + bc + d rabbit-hole we're
> going to end up going. As far as we have to, I guess.
Well, the common patterns I've seen so far are:
a
ab
abc
a + bc
ab + cd
For any longer multiplications, I've only found[1]:
drivers/staging/rtl8188eu/os_dep/osdep_service.c: void **a =
kzalloc(h * sizeof(void *) + h * w * size, GFP_KERNEL);
At the end of the day, though, I don't really like having all these
different names...
kmalloc(), kmalloc_array(), kmalloc_ab_c(), kmalloc_array_3d()
with their "matching" zeroing function:
kzalloc(), kcalloc(), kzalloc_ab_c(), kmalloc_array_3d(..., gfp | __GFP_ZERO)
For the multiplication cases, I wonder if we could just have:
kmalloc_multN(gfp, a, b, c, ...)
kzalloc_multN(gfp, a, b, c, ...)
and we can replace all kcalloc() users with kzalloc_mult2(), all
kmalloc_array() users with kmalloc_mult2(), the abc uses with
kmalloc_mult3().
That said, I *do* like kmalloc_struct() as it's a very common pattern...
Or maybe, just leave the pattern in the name? kmalloc_ab(),
kmalloc_abc(), kmalloc_ab_c(), kmalloc_ab_cd() ?
Getting the constant ordering right could be part of the macro
definition, maybe? i.e.:
static inline void *kmalloc_ab(size_t a, size_t b, gfp_t flags)
{
if (__builtin_constant_p(a) && a != 0 && \
b > SIZE_MAX / a)
return NULL;
else if (__builtin_constant_p(b) && b != 0 && \
a > SIZE_MAX / b)
return NULL;
return kmalloc(a * b, flags);
}
(I just wish C had a sensible way to catch overflow...)
-Kees
[1] git grep -E 'alloc\([^,]+[^(]\*[^)][^,]+[^(]\*[^)][^,]+[^(]\*[^)][^,]+,'
--
Kees Cook
Pixel Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH][RFC] kernel.h: provide array iterator
On Thu, Mar 15, 2018 at 3:00 AM, Kieran Bingham
<kieran.bing...@ideasonboard.com> wrote:
> Simplify array iteration with a helper to iterate each entry in an array.
> Utilise the existing ARRAY_SIZE macro to identify the length of the array
> and pointer arithmetic to process each item as a for loop.
>
> Signed-off-by: Kieran Bingham <kieran.bing...@ideasonboard.com>
> ---
> include/linux/kernel.h | 10 ++
> 1 file changed, 10 insertions(+)
>
> The use of static arrays to store data is a common use case throughout the
> kernel. Along with that is the obvious need to iterate that data.
>
> In fact there are just shy of 5000 instances of iterating a static array:
> git grep "for .*ARRAY_SIZE" | wc -l
> 4943
I suspect the main question is "Does this macro make the code easier to read?"
I think it does, and we have other kinds of iterators like this in the
kernel already. Would it be worth building a Coccinelle script to do
the 5000 replacements?
-Kees
>
> When working on the UVC driver - I found that I needed to split one such
> iteration into two parts, and at the same time felt that this could be
> refactored to be cleaner / easier to read.
>
> I do however worry that this simple short patch might not be desired or could
> also be heavily bikeshedded due to it's potential wide spread use (though
> perhaps that would be a good thing to have more users) ... but here it is,
> along with an example usage below which is part of a separate series.
>
> The aim is to simplify iteration on static arrays, in the same way that we
> have
> iterators for lists. The use of the ARRAY_SIZE macro, provides all the
> protections given by "__must_be_array(arr)" to this macro too.
>
> Regards
>
> Kieran
>
> =
> Example Usage from a pending UVC development:
>
> +#define for_each_uvc_urb(uvc_urb, uvc_streaming) \
> + for_each_array_element(uvc_urb, uvc_streaming->uvc_urb)
>
> /*
> * Uninitialize isochronous/bulk URBs and free transfer buffers.
> */
> static void uvc_uninit_video(struct uvc_streaming *stream, int free_buffers)
> {
> - struct urb *urb;
> - unsigned int i;
> + struct uvc_urb *uvc_urb;
>
> uvc_video_stats_stop(stream);
>
> - for (i = 0; i < UVC_URBS; ++i) {
> - struct uvc_urb *uvc_urb = >uvc_urb[i];
> + for_each_uvc_urb(uvc_urb, stream)
> + usb_kill_urb(uvc_urb->urb);
>
> - urb = uvc_urb->urb;
> - if (urb == NULL)
> - continue;
> + flush_workqueue(stream->async_wq);
>
> - usb_kill_urb(urb);
> - usb_free_urb(urb);
> + for_each_uvc_urb(uvc_urb, stream) {
> + usb_free_urb(uvc_urb->urb);
> uvc_urb->urb = NULL;
> }
>
> if (free_buffers)
> uvc_free_urb_buffers(stream);
> }
> =
>
>
>
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index ce51455e2adf..95d7dae248b7 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -70,6 +70,16 @@
> */
> #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
> __must_be_array(arr))
>
> +/**
> + * for_each_array_element - Iterate all items in an array
> + * @elem: pointer of array type for iteration cursor
> + * @array: array to be iterated
> + */
> +#define for_each_array_element(elem, array) \
> + for (elem = &(array)[0]; \
> +elem < &(array)[ARRAY_SIZE(array)]; \
> +++elem)
> +
> #define u64_to_user_ptr(x) ( \
> { \
> typecheck(u64, x); \
> --
> 2.7.4
>
--
Kees Cook
Pixel Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
On Wed, Feb 14, 2018 at 10:26 AM, Matthew Wilcox <wi...@infradead.org> wrote:
> From: Matthew Wilcox <mawil...@microsoft.com>
>
> We have kvmalloc_array in order to safely allocate an array with a
> number of elements specified by userspace (avoiding arithmetic overflow
> leading to a buffer overrun). But it's fairly common to have a header
> in front of that array (eg specifying the length of the array), so we
> need a helper function for that situation.
>
> kvmalloc_ab_c() is the workhorse that does the calculation, but in spite
> of our best efforts to name the arguments, it's really hard to remember
> which order to put the arguments in. kvzalloc_struct() eliminates that
> effort; you tell it about the struct you're allocating, and it puts the
> arguments in the right order for you (and checks that the arguments
> you've given are at least plausible).
>
> For comparison between the three schemes:
>
> sev = kvzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems,
> GFP_KERNEL);
> sev = kvzalloc_ab_c(elems, sizeof(struct v4l2_kevent), sizeof(*sev),
> GFP_KERNEL);
> sev = kvzalloc_struct(sev, events, elems, GFP_KERNEL);
>
> Signed-off-by: Matthew Wilcox <mawil...@microsoft.com>
> ---
> include/linux/mm.h | 51 +++
> 1 file changed, 51 insertions(+)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 81bd7f0be286..ddf929c5aaee 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -557,6 +557,57 @@ static inline void *kvmalloc_array(size_t n, size_t
> size, gfp_t flags)
> return kvmalloc(n * size, flags);
> }
>
> +/**
> + * kvmalloc_ab_c() - Allocate memory.
Longer description, maybe? "Allocate a *b + c bytes of memory"?
> + * @n: Number of elements.
> + * @size: Size of each element (should be constant).
> + * @c: Size of header (should be constant).
If these should be constant, should we mark them as "const"? Or WARN
if __builtin_constant_p() isn't true?
> + * @gfp: Memory allocation flags.
> + *
> + * Use this function to allocate @n * @size + @c bytes of memory. This
> + * function is safe to use when @n is controlled from userspace; it will
> + * return %NULL if the required amount of memory cannot be allocated.
> + * Use kvfree() to free the allocated memory.
> + *
> + * The kvzalloc_hdr_arr() function is easier to use as it has typechecking
renaming typo? Should this be "kvzalloc_struct()"?
> + * and you do not need to remember which of the arguments should be
> constants.
> + *
> + * Context: Process context. May sleep; the @gfp flags should be based on
> + * %GFP_KERNEL.
> + * Return: A pointer to the allocated memory or %NULL.
> + */
> +static inline __must_check
> +void *kvmalloc_ab_c(size_t n, size_t size, size_t c, gfp_t gfp)
> +{
> + if (size != 0 && n > (SIZE_MAX - c) / size)
> + return NULL;
> +
> + return kvmalloc(n * size + c, gfp);
> +}
> +#define kvzalloc_ab_c(a, b, c, gfp)kvmalloc_ab_c(a, b, c, gfp |
> __GFP_ZERO)
Nit: "(gfp) | __GFP_ZERO" just in case of insane usage.
> +
> +/**
> + * kvzalloc_struct() - Allocate and zero-fill a structure containing a
> + *variable length array.
> + * @p: Pointer to the structure.
> + * @member: Name of the array member.
> + * @n: Number of elements in the array.
> + * @gfp: Memory allocation flags.
> + *
> + * Allocate (and zero-fill) enough memory for a structure with an array
> + * of @n elements. This function is safe to use when @n is specified by
> + * userspace as the arithmetic will not overflow.
> + * Use kvfree() to free the allocated memory.
> + *
> + * Context: Process context. May sleep; the @gfp flags should be based on
> + * %GFP_KERNEL.
> + * Return: Zero-filled memory or a NULL pointer.
> + */
> +#define kvzalloc_struct(p, member, n, gfp) \
> + (typeof(p))kvzalloc_ab_c(n, \
> + sizeof(*(p)->member) + __must_be_array((p)->member),\
> + offsetof(typeof(*(p)), member), gfp)
> +
> extern void kvfree(const void *addr);
>
> static inline atomic_t *compound_mapcount_ptr(struct page *page)
It might be nice to include another patch that replaces some of the
existing/common uses of a*b+c with the new function...
Otherwise, yes, please. We could build a coccinelle rule for
additional replacements...
-Kees
--
Kees Cook
Pixel Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] [PATCH v2 01/31] coccinelle: Improve setup_timer.cocci matching
This improves the patch mode of setup_timer.cocci. Several patterns
were missing:
- assignments-before-init_timer() cases
- limit the .data case removal to the specific struct timer_list instance
- handling calls by dereference (timer->field vs timer.field)
Cc: Julia Lawall <julia.law...@lip6.fr>
Cc: Gilles Muller <gilles.mul...@lip6.fr>
Cc: Nicolas Palix <nicolas.pa...@imag.fr>
Cc: Michal Marek <mma...@suse.com>
Cc: cocci@systeme.lip6.fr
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
scripts/coccinelle/api/setup_timer.cocci | 129 +--
1 file changed, 105 insertions(+), 24 deletions(-)
diff --git a/scripts/coccinelle/api/setup_timer.cocci
b/scripts/coccinelle/api/setup_timer.cocci
index eb6bd9e4ab1a..279767f3bbef 100644
--- a/scripts/coccinelle/api/setup_timer.cocci
+++ b/scripts/coccinelle/api/setup_timer.cocci
@@ -2,6 +2,7 @@
/// and data fields
// Confidence: High
// Copyright: (C) 2016 Vaishali Thakkar, Oracle. GPLv2
+// Copyright: (C) 2017 Kees Cook, Google. GPLv2
// Options: --no-includes --include-headers
// Keywords: init_timer, setup_timer
@@ -10,60 +11,123 @@ virtual context
virtual org
virtual report
+// Match the common cases first to avoid Coccinelle parsing loops with
+// "... when" clauses.
+
@match_immediate_function_data_after_init_timer
depends on patch && !context && !org && !report@
expression e, func, da;
@@
--init_timer ();
-+setup_timer (, func, da);
+-init_timer
++setup_timer
+ ( \(\|e\)
++, func, da
+ );
+(
+-\(e.function\|e->function\) = func;
+-\(e.data\|e->data\) = da;
+|
+-\(e.data\|e->data\) = da;
+-\(e.function\|e->function\) = func;
+)
+
+@match_immediate_function_data_before_init_timer
+depends on patch && !context && !org && !report@
+expression e, func, da;
+@@
(
+-\(e.function\|e->function\) = func;
+-\(e.data\|e->data\) = da;
+|
+-\(e.data\|e->data\) = da;
+-\(e.function\|e->function\) = func;
+)
+-init_timer
++setup_timer
+ ( \(\|e\)
++, func, da
+ );
+
+@match_function_and_data_after_init_timer
+depends on patch && !context && !org && !report@
+expression e, e2, e3, e4, e5, func, da;
+@@
+
+-init_timer
++setup_timer
+ ( \(\|e\)
++, func, da
+ );
+ ... when != func = e2
+ when != da = e3
+(
-e.function = func;
+... when != da = e4
-e.data = da;
|
+-e->function = func;
+... when != da = e4
+-e->data = da;
+|
-e.data = da;
+... when != func = e5
-e.function = func;
+|
+-e->data = da;
+... when != func = e5
+-e->function = func;
)
-@match_function_and_data_after_init_timer
+@match_function_and_data_before_init_timer
depends on patch && !context && !org && !report@
-expression e1, e2, e3, e4, e5, a, b;
+expression e, e2, e3, e4, e5, func, da;
@@
-
--init_timer ();
-+setup_timer (, a, b);
-
-... when != a = e2
-when != b = e3
(
--e1.function = a;
-... when != b = e4
--e1.data = b;
+-e.function = func;
+... when != da = e4
+-e.data = da;
|
--e1.data = b;
-... when != a = e5
--e1.function = a;
+-e->function = func;
+... when != da = e4
+-e->data = da;
+|
+-e.data = da;
+... when != func = e5
+-e.function = func;
+|
+-e->data = da;
+... when != func = e5
+-e->function = func;
)
+... when != func = e2
+when != da = e3
+-init_timer
++setup_timer
+ ( \(\|e\)
++, func, da
+ );
@r1 exists@
+expression t;
identifier f;
position p;
@@
f(...) { ... when any
- init_timer@p(...)
+ init_timer@p(\(\|t\))
... when any
}
@r2 exists@
+expression r1.t;
identifier g != r1.f;
-struct timer_list t;
expression e8;
@@
g(...) { ... when any
- t.data = e8
+ \(t.data\|t->data\) = e8
... when any
}
@@ -77,14 +141,31 @@ p << r1.p;
cocci.include_match(False)
@r3 depends on patch && !context && !org && !report@
-expression e6, e7, c;
+expression r1.t, func, e7;
position r1.p;
@@
--init_timer@p ();
-+setup_timer (, c, 0UL);
-... when != c = e7
--e6.function = c;
+(
+-init_timer@p();
++setup_timer(, func, 0UL);
+... when != func = e7
+-t.function = func;
+|
+-t.function = func;
+... when != func = e7
+-init_timer@p();
++setup_timer(, func, 0UL);
+|
+-init_timer@p(t);
++setup_timer(t, func, 0UL);
+... when != func = e7
+-t->function = func;
+|
+-t->function = func;
+... when != func = e7
+-init_timer@p(t);
++setup_timer(t, func, 0UL);
+)
//
--
2.7.4
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] what is needed to match a set of fields?
On Sat, Sep 16, 2017 at 10:25 PM, Julia Lawall <julia.law...@lip6.fr> wrote:
>
>
> On Sat, 16 Sep 2017, Kees Cook wrote:
>
>> Hi,
>>
>> If I have several structures inline, "identifier" doesn't match it. For
>> example:
>>
>> setup_timer(>thermal_throttle.ct_kill_exit_tm,
>> iwl_tt_check_exit_ct_kill, (unsigned long)priv);
>>
>> @@
>> expression _E;
>> identifier _timer;
>> @@
>>
>> -setup_timer(&_E->_timer, ...);
>>
>> This doesn't match (which makes sense, "thermal_throttle" and
>> "ct_kill_exit_tm" are different identifiers), but I can't use an
>> expression for what's after ->. For example:
>>
>> @@
>> expression _E;
>> expression _timer;
>> @@
>>
>> -setup_timer(&_E->_timer, ...);
>>
>> This refuses to parse for me:
>>
>> Fatal error: exception Failure("minus: parse error: \n = File
>> \"/tmp/metavar.cocci\", line 6, column 18, charpos = 59\naround =
>> '_timer', whole content = -setup_timer(&_E->_timer, ...);\n")
>>
>> How do I match a series of identifiers? I'd like to be able to match
>> all of these with a single type of metavariable:
>>
>> setup_timer(>timer, ...);
>> setup_timer(>struct1.timer, ...);
>> setup_timer(>struct1.struct2.timer, ...);
>> etc
>
> There is no way to do that. Do you need to know about the names struct 1
> and struct2?
Yeah, but I'm always using them together. For example, converting
callbacks to use container_of(timer_var, struct outer, struct1.timer)
So, no worries, I'll create a separate script that adds the "struct1"
match, etc.
-Kees
--
Kees Cook
Pixel Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] what is needed to match a set of fields?
Hi,
If I have several structures inline, "identifier" doesn't match it. For example:
setup_timer(>thermal_throttle.ct_kill_exit_tm,
iwl_tt_check_exit_ct_kill, (unsigned long)priv);
@@
expression _E;
identifier _timer;
@@
-setup_timer(&_E->_timer, ...);
This doesn't match (which makes sense, "thermal_throttle" and
"ct_kill_exit_tm" are different identifiers), but I can't use an
expression for what's after ->. For example:
@@
expression _E;
expression _timer;
@@
-setup_timer(&_E->_timer, ...);
This refuses to parse for me:
Fatal error: exception Failure("minus: parse error: \n = File
\"/tmp/metavar.cocci\", line 6, column 18, charpos = 59\naround =
'_timer', whole content = -setup_timer(&_E->_timer, ...);\n")
How do I match a series of identifiers? I'd like to be able to match
all of these with a single type of metavariable:
setup_timer(>timer, ...);
setup_timer(>struct1.timer, ...);
setup_timer(>struct1.struct2.timer, ...);
etc
Thanks!
-Kees
--
Kees Cook
Pixel Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] [PATCH 01/31] coccinelle: Improve setup_timer.cocci matching
This improves the patch mode of setup_timer.cocci. Several patterns
were missing:
- assignments-before-init_timer() cases
- limit the .data case removal to the specific struct timer_list instance
- handling calls by dereference (timer->field vs timer.field)
Cc: Julia Lawall <julia.law...@lip6.fr>
Cc: Gilles Muller <gilles.mul...@lip6.fr>
Cc: Nicolas Palix <nicolas.pa...@imag.fr>
Cc: Michal Marek <mma...@suse.com>
Cc: cocci@systeme.lip6.fr
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
scripts/coccinelle/api/setup_timer.cocci | 129 +--
1 file changed, 105 insertions(+), 24 deletions(-)
diff --git a/scripts/coccinelle/api/setup_timer.cocci
b/scripts/coccinelle/api/setup_timer.cocci
index eb6bd9e4ab1a..279767f3bbef 100644
--- a/scripts/coccinelle/api/setup_timer.cocci
+++ b/scripts/coccinelle/api/setup_timer.cocci
@@ -2,6 +2,7 @@
/// and data fields
// Confidence: High
// Copyright: (C) 2016 Vaishali Thakkar, Oracle. GPLv2
+// Copyright: (C) 2017 Kees Cook, Google. GPLv2
// Options: --no-includes --include-headers
// Keywords: init_timer, setup_timer
@@ -10,60 +11,123 @@ virtual context
virtual org
virtual report
+// Match the common cases first to avoid Coccinelle parsing loops with
+// "... when" clauses.
+
@match_immediate_function_data_after_init_timer
depends on patch && !context && !org && !report@
expression e, func, da;
@@
--init_timer ();
-+setup_timer (, func, da);
+-init_timer
++setup_timer
+ ( \(\|e\)
++, func, da
+ );
+(
+-\(e.function\|e->function\) = func;
+-\(e.data\|e->data\) = da;
+|
+-\(e.data\|e->data\) = da;
+-\(e.function\|e->function\) = func;
+)
+
+@match_immediate_function_data_before_init_timer
+depends on patch && !context && !org && !report@
+expression e, func, da;
+@@
(
+-\(e.function\|e->function\) = func;
+-\(e.data\|e->data\) = da;
+|
+-\(e.data\|e->data\) = da;
+-\(e.function\|e->function\) = func;
+)
+-init_timer
++setup_timer
+ ( \(\|e\)
++, func, da
+ );
+
+@match_function_and_data_after_init_timer
+depends on patch && !context && !org && !report@
+expression e, e2, e3, e4, e5, func, da;
+@@
+
+-init_timer
++setup_timer
+ ( \(\|e\)
++, func, da
+ );
+ ... when != func = e2
+ when != da = e3
+(
-e.function = func;
+... when != da = e4
-e.data = da;
|
+-e->function = func;
+... when != da = e4
+-e->data = da;
+|
-e.data = da;
+... when != func = e5
-e.function = func;
+|
+-e->data = da;
+... when != func = e5
+-e->function = func;
)
-@match_function_and_data_after_init_timer
+@match_function_and_data_before_init_timer
depends on patch && !context && !org && !report@
-expression e1, e2, e3, e4, e5, a, b;
+expression e, e2, e3, e4, e5, func, da;
@@
-
--init_timer ();
-+setup_timer (, a, b);
-
-... when != a = e2
-when != b = e3
(
--e1.function = a;
-... when != b = e4
--e1.data = b;
+-e.function = func;
+... when != da = e4
+-e.data = da;
|
--e1.data = b;
-... when != a = e5
--e1.function = a;
+-e->function = func;
+... when != da = e4
+-e->data = da;
+|
+-e.data = da;
+... when != func = e5
+-e.function = func;
+|
+-e->data = da;
+... when != func = e5
+-e->function = func;
)
+... when != func = e2
+when != da = e3
+-init_timer
++setup_timer
+ ( \(\|e\)
++, func, da
+ );
@r1 exists@
+expression t;
identifier f;
position p;
@@
f(...) { ... when any
- init_timer@p(...)
+ init_timer@p(\(\|t\))
... when any
}
@r2 exists@
+expression r1.t;
identifier g != r1.f;
-struct timer_list t;
expression e8;
@@
g(...) { ... when any
- t.data = e8
+ \(t.data\|t->data\) = e8
... when any
}
@@ -77,14 +141,31 @@ p << r1.p;
cocci.include_match(False)
@r3 depends on patch && !context && !org && !report@
-expression e6, e7, c;
+expression r1.t, func, e7;
position r1.p;
@@
--init_timer@p ();
-+setup_timer (, c, 0UL);
-... when != c = e7
--e6.function = c;
+(
+-init_timer@p();
++setup_timer(, func, 0UL);
+... when != func = e7
+-t.function = func;
+|
+-t.function = func;
+... when != func = e7
+-init_timer@p();
++setup_timer(, func, 0UL);
+|
+-init_timer@p(t);
++setup_timer(t, func, 0UL);
+... when != func = e7
+-t->function = func;
+|
+-t->function = func;
+... when != func = e7
+-init_timer@p(t);
++setup_timer(t, func, 0UL);
+)
//
--
2.7.4
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Automatic replacement of function declarations
On Mon, Aug 28, 2017 at 4:34 AM, Julia Lawall <julia.law...@lip6.fr> wrote:
>
>
> On Sun, 27 Aug 2017, Kees Cook wrote:
>
>> Hi,
>>
>> So, I noticed that if I replace argument types in a function,
>> coccinelle will normally replace them in any forward declarations too.
>> However, this:
>>
>> @change_callback
>> depends on patch@
>> identifier _callback;
>> type _origtype;
>> identifier _origarg;
>> type _handletype;
>> identifier _handle;
>> @@
>>
>> void _callback(
>> -_origtype _origarg
>> +struct timer_list *t
>> )
>> {
>> ... when != _origarg
>> _handletype *_handle =
>> -(_handletype *)_origarg;
>> +TIMER_CONTAINER(_handle, t, timer);
>> ... when != _origarg
>> }
>>
>> run against drivers/net/wireless/ray_cs.c will fix join_net and
>> start_net correctly:
>>
>> -static void join_net(u_long local);
>> -static void start_net(u_long local);
>> +static void join_net(struct timer_list *t);
>> +static void start_net(struct timer_list *t);
>>
>> but misses verify_dl_startup and authenticate_timeout.
>>
>> The difference is the latter have forward declarations without an argument
>> name:
>>
>> static void authenticate_timeout(u_long);
>> static void verify_dl_startup(u_long);
>
> This was the first use of u_long in the file, so it was considering it to
> be a K parameter name. Now, by default, as soon as there is any non-K
> parameter, the parser gives up on trying to find K parameters.
> --force-kr causes it to keep looking for K parameters, and --prevent-kr
> causes it to never look for K parameters.
>
> I don't know to what extent people are still working on K code. Another
> option would be to have --prevent-kr as the default.
>
> In any case, the example works now with no extra command line arguments.
Oh awesome, thanks for fixing this!
-Kees
--
Kees Cook
Pixel Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] Automatic replacement of function declarations
Hi,
So, I noticed that if I replace argument types in a function,
coccinelle will normally replace them in any forward declarations too.
However, this:
@change_callback
depends on patch@
identifier _callback;
type _origtype;
identifier _origarg;
type _handletype;
identifier _handle;
@@
void _callback(
-_origtype _origarg
+struct timer_list *t
)
{
... when != _origarg
_handletype *_handle =
-(_handletype *)_origarg;
+TIMER_CONTAINER(_handle, t, timer);
... when != _origarg
}
run against drivers/net/wireless/ray_cs.c will fix join_net and
start_net correctly:
-static void join_net(u_long local);
-static void start_net(u_long local);
+static void join_net(struct timer_list *t);
+static void start_net(struct timer_list *t);
but misses verify_dl_startup and authenticate_timeout.
The difference is the latter have forward declarations without an argument name:
static void authenticate_timeout(u_long);
static void verify_dl_startup(u_long);
Is this a bug, or did I write my rule in some way that excludes these
forward declarations?
Thanks!
-Kees
--
Kees Cook
Pixel Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Matching function prototypes and casts
On Wed, Aug 23, 2017 at 2:19 PM, Julia Lawall <julia.law...@lip6.fr> wrote:
>
>
> On Wed, 23 Aug 2017, Kees Cook wrote:
>
>> I think I'm getting closer. Here are some specific examples that don't
>> seem to work:
>>
>> ---match_callback.cocci---
>> virtual patch
>>
>> @match_timer_function_usage
>> depends on patch@
>> expression _E;
>> struct timer_list _e;
>> identifier _timer;
>> identifier _callback;
>> type _cast_func, _cast_data;
>> @@
>>
>> (
>> -setup_timer(&_E->_timer@_e, _callback, _E);
>> |
>> -setup_timer(&_E->_timer@_e, &_callback, _E);
>> |
>> -setup_timer(&_E->_timer@_e, _callback, (_cast_data)_E);
>> |
>> -setup_timer(&_E->_timer@_e, &_callback, (_cast_data)_E);
>> |
>> -setup_timer(&_E->_timer@_e, (_cast_func)_callback, _E);
>> |
>> -setup_timer(&_E->_timer@_e, (_cast_func)&_callback, _E);
>> |
>> -setup_timer(&_E->_timer@_e, (_cast_func)_callback, (_cast_data)_E);
>> |
>> -setup_timer(&_E->_timer@_e, (_cast_func)&_callback, (_cast_data)_E);
>> |
>> -_E->_timer@_e.function = _callback;
>> |
>> -_E->_timer@_e.function = &_callback;
>> |
>> -_E->_timer@_e.function = (_cast_func)_callback;
>> |
>> -_E->_timer@_e.function = (_cast_func)&_callback;
>> )
>> ---EOF---
>>
>> Doesn't match drivers/ide/ide-probe.c which has:
>>
>> setup_timer(>timer, _timer_expiry, (unsigned long)hwif);
>>
>> Even this doesn't:
>>
>> (
>> -setup_timer(&_E->_timer@_e, &_callback, (_cast_data)_E);
>> )
>>
>> Unless I remove the "@_e" part...? Am I using that wrong?
>
> For me it works. Do you have the latest version of Coccinelle from
> github? I used the option --all-includes.
Ah-ha! Thank you. :) I had --no-includes in my .cocci. :)
More insane corner cases:
I have this function:
static void
hfcmulti_dbusy_timer(struct hfc_multi *hc)
{
}
And this rule (which is using the working change_timer_function_usage
rule to find identifiers):
// callback(struct something *handle)
@change_callback_handle_arg
depends on patch &&
change_timer_function_usage &&
!change_callback_handle_cast@
identifier change_timer_function_usage._callback;
identifier change_timer_function_usage._timer;
type _handletype;
identifier _handle;
@@
-static void _callback(_handletype *_handle)
+static void _callback(struct timer_list *t)
{
+ _handletype *_handle = TIMER_CONTAINER(_handle, t, _timer);
...
}
It correctly produces:
-static void
-hfcmulti_dbusy_timer(struct hfc_multi *hc)
+static void hfcmulti_dbusy_timer(struct timer_list *t)
{
+ struct hfc_multi *hc = TIMER_CONTAINER(hc, t, timer);
}
But since this was an empty function originally, I don't want to add
just the variable declaration. I tried various ()-like things, but
they didn't work:
-static void _callback(_handletype *_handle)
+static void _callback(struct timer_list *t)
{
(
+ _handletype *_handle = TIMER_CONTAINER(_handle, t, _timer);
...
|
)
}
etc...
-Kees
--
Kees Cook
Pixel Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Matching function prototypes and casts
I think I'm getting closer. Here are some specific examples that don't seem to work: ---match_callback.cocci--- virtual patch @match_timer_function_usage depends on patch@ expression _E; struct timer_list _e; identifier _timer; identifier _callback; type _cast_func, _cast_data; @@ ( -setup_timer(&_E->_timer@_e, _callback, _E); | -setup_timer(&_E->_timer@_e, &_callback, _E); | -setup_timer(&_E->_timer@_e, _callback, (_cast_data)_E); | -setup_timer(&_E->_timer@_e, &_callback, (_cast_data)_E); | -setup_timer(&_E->_timer@_e, (_cast_func)_callback, _E); | -setup_timer(&_E->_timer@_e, (_cast_func)&_callback, _E); | -setup_timer(&_E->_timer@_e, (_cast_func)_callback, (_cast_data)_E); | -setup_timer(&_E->_timer@_e, (_cast_func)&_callback, (_cast_data)_E); | -_E->_timer@_e.function = _callback; | -_E->_timer@_e.function = &_callback; | -_E->_timer@_e.function = (_cast_func)_callback; | -_E->_timer@_e.function = (_cast_func)&_callback; ) ---EOF--- Doesn't match drivers/ide/ide-probe.c which has: setup_timer(>timer, _timer_expiry, (unsigned long)hwif); Even this doesn't: ( -setup_timer(&_E->_timer@_e, &_callback, (_cast_data)_E); ) Unless I remove the "@_e" part...? Am I using that wrong? -Kees ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH] coccinelle: Improve setup_timer.cocci matching
On Wed, Aug 23, 2017 at 6:13 AM, Julia Lawall <julia.law...@lip6.fr> wrote: > > > On Tue, 22 Aug 2017, Kees Cook wrote: > >> This improves the patch mode of setup_timer.cocci. Several patterns were >> missing: >> - assignments-before-init_timer() cases >> - limiting the .data case removal to the struct timer_list instance >> - handling calls by dereference (timer->field vs timer.field) >> >> Running this on the current kernel tree produces a large diff that I >> would like to get applied as the first step in removing the .data >> field from struct timer_list: >> >> 208 files changed, 367 insertions(+), 757 deletions(-) >> >> Signed-off-by: Kees Cook <keesc...@chromium.org> >> --- >> scripts/coccinelle/api/setup_timer.cocci | 129 >> +-- >> 1 file changed, 105 insertions(+), 24 deletions(-) >> >> diff --git a/scripts/coccinelle/api/setup_timer.cocci >> b/scripts/coccinelle/api/setup_timer.cocci >> index eb6bd9e4ab1a..bc6bd8f0b4bf 100644 >> --- a/scripts/coccinelle/api/setup_timer.cocci >> +++ b/scripts/coccinelle/api/setup_timer.cocci >> @@ -2,6 +2,7 @@ >> /// and data fields >> // Confidence: High >> // Copyright: (C) 2016 Vaishali Thakkar, Oracle. GPLv2 >> +// Copyright: (C) 2017 Kees Cook, Google. GPLv2 >> // Options: --no-includes --include-headers >> // Keywords: init_timer, setup_timer >> >> @@ -10,60 +11,123 @@ virtual context >> virtual org >> virtual report >> >> +// Match the common cases first to avoid Coccinelle parsing loops with >> +// "... when" clauses. >> + >> @match_immediate_function_data_after_init_timer >> depends on patch && !context && !org && !report@ >> expression e, func, da; >> @@ >> >> --init_timer (); >> -+setup_timer (, func, da); >> +-init_timer >> ++setup_timer >> + ( \(\|e\) >> ++, func, da >> + ); >> +( >> +-\(e.function\|e->function\) = func; >> +-\(e.data\|e->data\) = da; >> +| >> +-\(e.function\|e->function\) = func; >> +-\(e.data\|e->data\) = da; > > Same thing twice; I think you want to invert the last two lines. > >> +) >> + >> +@match_immediate_function_data_before_init_timer >> +depends on patch && !context && !org && !report@ >> +expression e, func, da; >> +@@ >> >> ( >> +-\(e.function\|e->function\) = func; >> +-\(e.data\|e->data\) = da; >> +| >> +-\(e.function\|e->function\) = func; >> +-\(e.data\|e->data\) = da; > > Same as in the previous case. Oops, thanks! AIUI, missing these cases must makes runtime slower. I'll fix it up and resend. -Kees -- Kees Cook Pixel Security ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] isomorphism for &(structure)->field vs struct.field
On Fri, Aug 18, 2017 at 10:08 AM, Julia Lawall <julia.law...@lip6.fr> wrote: > > > On Fri, 18 Aug 2017, Kees Cook wrote: > >> I'd like to have a rule that would match both: >> >> function(ptr); >> ptr->field = 7; >> >> and >> >> function(); >> obj.field = 7; >> >> to produce: >> >> new_function(ptr, 7); >> >> and >> >> new_function(, 7); >> >> respectively. The internal isomorphisms don't seem to cover this? Or >> I'm maybe doing something wrong? > > There is no isomorphism for that. Perhaps you cold live with the > following? > > @@ > expression x,e; > @@ > > -f > +g > ( \(\|x\) > +, e > ); > - \(x.field\|x->field\) = e; Ah, perfect, thanks! -Kees -- Kees Cook Pixel Security ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] isomorphism for &(structure)->field vs struct.field
I'd like to have a rule that would match both: function(ptr); ptr->field = 7; and function(); obj.field = 7; to produce: new_function(ptr, 7); and new_function(, 7); respectively. The internal isomorphisms don't seem to cover this? Or I'm maybe doing something wrong? Thanks! -Kees -- Kees Cook Pixel Security ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH] Coccinelle: add atomic_as_refcounter script
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 000..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(>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
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [RFC] coccicheck: add a test for repeat memory fetches
On Tue, Jan 10, 2017 at 1:14 PM, Julia Lawall <julia.law...@lip6.fr> wrote: > OK, I have the impression that what you are looking for is the following, > that currently does not seem to work well. Still maybe it gives an idea. > > The basic pattern is the following sequence: > > 1. copy_from_user > 2. test on a field of the copied value > 3. another copy_from_user > 4. a use of the same field as tested in step 2 from the structure obtained > by the second copy_from_user or a function call with the structure as an > argument This looks pretty good! > In the case where the second copy_from_user stores the result in a > pointer, then a return with no reference of the tested field is also a > concern, unless, the pointer was already kfreed. I think sequence "2" above missing just looking at a direct value, like if instead of a field it was a u32. Also, should binop include "=="? And we need to add back in get_user() too... hmmm -Kees -- Kees Cook Nexus Security ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [RFC] coccicheck: add a test for repeat memory fetches
On Tue, Jan 10, 2017 at 11:30 AM, Julia Lawall <julia.law...@lip6.fr> wrote:
>
>
> On Tue, 10 Jan 2017, Kees Cook wrote:
>
>> On Tue, Jan 10, 2017 at 10:28 AM, Julia Lawall <julia.law...@lip6.fr> wrote:
>> >> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2159
>> >> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2257
>> >> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2302
>> >> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2342
>> >> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2365
>> >> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2406
>> >> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2439
>> >> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2491
>> >
>> > Do you want the above results? They have the form:
>> >
>> > if (copy_from_user(, useraddr, sizeof(t)))
>> >
>> > My reasoning was that there could be no problem here, because the size is
>> > the size of the destination structure. It doesn't depend on user level
>> > data.
>>
>> They're likely false positives, but it does follow the pattern of
>> reading the same userspace location twice:
>>
>> if (copy_from_user(, useraddr, sizeof(cmd)))
>> return -EFAULT;
>>
>> switch (cmd) {
>> case CHELSIO_SET_QSET_PARAMS:{
>> int i;
>> struct qset_params *q;
>> struct ch_qset_params t;
>> int q1 = pi->first_qset;
>> int nqsets = pi->nqsets;
>>
>> if (!capable(CAP_NET_ADMIN))
>> return -EPERM;
>> if (copy_from_user(, useraddr, sizeof(t)))
>> return -EFAULT;
>>
>> If there is any logic that examines cmd (u32) and operates on t
>> (struct ch_qset_params), there could be a flaw. It doesn't look like
>> it here, but a "correct" version of this would be:
>>
>> if (copy_from_user(, useraddr, sizeof(t)))
>> return -EFAULT;
>> t.cmd = cmd;
>
> OK, I'm fine with putting them all back.
>
> For another issue, what about code like the following:
>
> if (copy_from_user(_cmd, arg, sizeof(u_cmd)))
> return -EFAULT;
>
> if ((u_cmd.outsize > EC_MAX_MSG_BYTES) ||
> (u_cmd.insize > EC_MAX_MSG_BYTES))
> return -EINVAL;
>
> s_cmd = kmalloc(sizeof(*s_cmd) + max(u_cmd.outsize, u_cmd.insize),
> GFP_KERNEL);
> if (!s_cmd)
> return -ENOMEM;
>
> if (copy_from_user(s_cmd, arg, sizeof(*s_cmd) + u_cmd.outsize)) {
> ret = -EFAULT;
> goto exit;
> }
>
> It doesn't actually test sizeof(*s_cmd) + u_cmd.outsize, but it does test
> u_cmd.outsize > EC_MAX_MSG_BYTES, and presumably that test accounts for
> the extra sizeof(*s_cmd) value.
This is also bad since s_cmd now contains possibly new values for
outsize and insize, even though the old outsize was allocated. e.g.
past-end-of-buffer reads could happen for:
arg->outsize = 4
copy_from_user(_cmd, arg, ...)
s_cmd = kmalloc(... + 4)
arg->outsize = 1024
copy_from_user(s_cmd, arg, ...)
now s_cmd has only the 4 bytes allocated but anything operating on the
new outsize will expect 1024. This code needs the same sanity check:
if (s_cmd->outsize != u_cmd.outsize || s_cmd->insize != u_cmd.insize) {
ret = -EINVAL;
goto exit;
}
-Kees
--
Kees Cook
Nexus Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [RFC] coccicheck: add a test for repeat memory fetches
On Tue, Jan 10, 2017 at 11:23 AM, Kees Cook <keesc...@chromium.org> wrote:
> On Tue, Jan 10, 2017 at 10:28 AM, Julia Lawall <julia.law...@lip6.fr> wrote:
>>> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2159
>>> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2257
>>> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2302
>>> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2342
>>> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2365
>>> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2406
>>> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2439
>>> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2491
>>
>> Do you want the above results? They have the form:
>>
>> if (copy_from_user(, useraddr, sizeof(t)))
>>
>> My reasoning was that there could be no problem here, because the size is
>> the size of the destination structure. It doesn't depend on user level data.
>
> They're likely false positives, but it does follow the pattern of
> reading the same userspace location twice:
>
> if (copy_from_user(, useraddr, sizeof(cmd)))
> return -EFAULT;
>
> switch (cmd) {
> case CHELSIO_SET_QSET_PARAMS:{
> int i;
> struct qset_params *q;
> struct ch_qset_params t;
> int q1 = pi->first_qset;
> int nqsets = pi->nqsets;
>
> if (!capable(CAP_NET_ADMIN))
> return -EPERM;
> if (copy_from_user(, useraddr, sizeof(t)))
> return -EFAULT;
>
> If there is any logic that examines cmd (u32) and operates on t
> (struct ch_qset_params), there could be a flaw. It doesn't look like
> it here, but a "correct" version of this would be:
>
> if (copy_from_user(, useraddr, sizeof(t)))
> return -EFAULT;
> t.cmd = cmd;
Errr, no. Think-o. Should be "if (t.cmd != cmd) { freak out }"...
-Kees
--
Kees Cook
Nexus Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [RFC] coccicheck: add a test for repeat memory fetches
On Tue, Jan 10, 2017 at 10:28 AM, Julia Lawall <julia.law...@lip6.fr> wrote:
>> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2159
>> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2257
>> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2302
>> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2342
>> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2365
>> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2406
>> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2439
>> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2491
>
> Do you want the above results? They have the form:
>
> if (copy_from_user(, useraddr, sizeof(t)))
>
> My reasoning was that there could be no problem here, because the size is
> the size of the destination structure. It doesn't depend on user level data.
They're likely false positives, but it does follow the pattern of
reading the same userspace location twice:
if (copy_from_user(, useraddr, sizeof(cmd)))
return -EFAULT;
switch (cmd) {
case CHELSIO_SET_QSET_PARAMS:{
int i;
struct qset_params *q;
struct ch_qset_params t;
int q1 = pi->first_qset;
int nqsets = pi->nqsets;
if (!capable(CAP_NET_ADMIN))
return -EPERM;
if (copy_from_user(, useraddr, sizeof(t)))
return -EFAULT;
If there is any logic that examines cmd (u32) and operates on t
(struct ch_qset_params), there could be a flaw. It doesn't look like
it here, but a "correct" version of this would be:
if (copy_from_user(, useraddr, sizeof(t)))
return -EFAULT;
t.cmd = cmd;
-Kees
--
Kees Cook
Nexus Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user
On Tue, Jan 10, 2017 at 12:40 AM, Vaishali Thakkar
<vaishali.thak...@oracle.com> wrote:
> On Tuesday 10 January 2017 01:51 PM, Pengfei Wang wrote:
>>
>>
>>> 在 2017年1月10日，上午1:05，Vaishali Thakkar <vaishali.thak...@oracle.com> 写道：
>>>
>>> On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote:
>>>>
>>>> I totally dropped the ball on this. Many thanks to Vaishali for
>>>> resurrecting it.
>>>>
>>>> Some changes are suggested below.
>>>>
>>>> On Tue, 26 Apr 2016, Kees Cook wrote:
>>>>
>>>>> This is usually a sign of a resized request. This adds a check for
>>>>> potential races or confusions. The check isn't 100% accurate, so it
>>>>> needs some manual review.
>>>>>
>>>>> Signed-off-by: Kees Cook <keesc...@chromium.org>
>>>>> ---
>>>>> scripts/coccinelle/tests/reusercopy.cocci | 36
>>>>> +++
>>>>> 1 file changed, 36 insertions(+)
>>>>> create mode 100644 scripts/coccinelle/tests/reusercopy.cocci
>>>>>
>>>>> diff --git a/scripts/coccinelle/tests/reusercopy.cocci
>>>>> b/scripts/coccinelle/tests/reusercopy.cocci
>>>>> new file mode 100644
>>>>> index ..53645de8ae95
>>>>> --- /dev/null
>>>>> +++ b/scripts/coccinelle/tests/reusercopy.cocci
>>>>> @@ -0,0 +1,36 @@
>>>>> +/// Recopying from the same user buffer frequently indicates a pattern
>>>>> of
>>>>> +/// Reading a size header, allocating, and then re-reading an entire
>>>>> +/// structure. If the structure's size is not re-validated, this can
>>>>> lead
>>>>> +/// to structure or data size confusions.
>>>>> +///
>>>>> +// Confidence: Moderate
>>>>> +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2.
>>>>> +// URL: http://coccinelle.lip6.fr/
>>>>> +// Comments:
>>>>> +// Options: -no_includes -include_headers
>>>>
>>>>
>>>> The options could be: --no-include --include-headers
>>>>
>>>> Actually, Coccinelle supports both, but it only officially supports the
>>>> -- versions.
>>>>
>>>>> +
>>>>> +virtual report
>>>>> +virtual org
>>>>
>>>>
>>>> Add, the following for the *s:
>>>>
>>>> virtual context
>>>>
>>>> Then add the following rule:
>>>>
>>>> @ok@
>>>> position p;
>>>> expression src,dest;
>>>> @@
>>>>
>>>> copy_from_user@p(, src, sizeof(dest))
>>>>
>>>>> +
>>>>> +@cfu_twice@
>>>>> +position p;
>>>>
>>>>
>>>> Change this to:
>>>>
>>>> position p != ok.p;
>>>>
>>>>> +identifier src;
>>>>> +expression dest1, dest2, size1, size2, offset;
>>>>> +@@
>>>>> +
>>>>> +*copy_from_user(dest1, src, size1)
>>>>> + ... when != src = offset
>>>>> + when != src += offset
>>>
>>>
>>> Here, may be we should add few more lines from Pengfei's
>>> script to avoid th potential FPs.
>>>
>>>> Add the following lines:
>>>>
>>>> when != if (size2 > e1 || ...) { ... return ...; }
>>>> when != if (size2 > e1 || ...) { ... size2 = e2 ... }
>>>>
>>>> These changes drop cases where the last argument to copy_from_usr is the
>>>> size of the first argument, which seems safe enough, and where there is
>>>> a
>>>> test on the size value that can either update it or abort the function.
>>>> These changes only eliminate false positives, as far as I could tell.
>>>>
>>>> If it would be more convenient, I could just send the complete revised
>>>> patch, or whatever seems convenient.
>>>
>>>
>>> I was also thinking that probably we should also add other user space
>>> memory API functions. May be get_user and strncpy_from_user. Although I'm
>>> not sure how common it is to find such patterns for both of these functions.
>>
>>
>> I strongly recommend you adding get_user() API , which is used pervasively
>> within the kernel just like copy_from user().
>
>
> Sure. I have changed regetuser-wang.cocci from Kees's RFC patches to
> include everything in the pattern matching rule. I'll send that as well.
>
>> In many situations, there is a combination use, get_user() copies first
>> then
>> followed by a copy_from_user() copy. According to our investigation, this
>> typical
>> situation works by get_user() firstly copying a field of a specific struct
>> to check,
>> then copy_from_user() copies in the whole struct to use. Of course, the
>> struct
>> field is fetch twice.
>
>
> Do you mean that there is a problem when we have get_user() followed by
> copy_from_user()? Basically something like
> this:
>
> get_user(..., src.arg) //where src.arg = field of a structure
> ...
> copy_from_user(..., src, ...) //where src is a whole structure
>
> If that is the case then we would need to have one more new script
> or rule for such kind of combinational patterns. Disjunction can
> probably give FPs.
Yup, we need a single script: I just split them into three for comparisons.
-Kees
--
Kees Cook
Nexus Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user
On Tue, Jan 10, 2017 at 12:21 AM, Pengfei Wang <wpengfein...@gmail.com> wrote:
>
> 在 2017年1月10日，上午1:05，Vaishali Thakkar <vaishali.thak...@oracle.com> 写道：
>
> On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote:
>
> I totally dropped the ball on this. Many thanks to Vaishali for
> resurrecting it.
>
> Some changes are suggested below.
>
> On Tue, 26 Apr 2016, Kees Cook wrote:
>
> This is usually a sign of a resized request. This adds a check for
> potential races or confusions. The check isn't 100% accurate, so it
> needs some manual review.
>
> Signed-off-by: Kees Cook <keesc...@chromium.org>
> ---
> scripts/coccinelle/tests/reusercopy.cocci | 36
> +++
> 1 file changed, 36 insertions(+)
> create mode 100644 scripts/coccinelle/tests/reusercopy.cocci
>
> diff --git a/scripts/coccinelle/tests/reusercopy.cocci
> b/scripts/coccinelle/tests/reusercopy.cocci
> new file mode 100644
> index ..53645de8ae95
> --- /dev/null
> +++ b/scripts/coccinelle/tests/reusercopy.cocci
> @@ -0,0 +1,36 @@
> +/// Recopying from the same user buffer frequently indicates a pattern of
> +/// Reading a size header, allocating, and then re-reading an entire
> +/// structure. If the structure's size is not re-validated, this can lead
> +/// to structure or data size confusions.
> +///
> +// Confidence: Moderate
> +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2.
> +// URL: http://coccinelle.lip6.fr/
> +// Comments:
> +// Options: -no_includes -include_headers
>
>
> The options could be: --no-include --include-headers
>
> Actually, Coccinelle supports both, but it only officially supports the
> -- versions.
>
> +
> +virtual report
> +virtual org
>
>
> Add, the following for the *s:
>
> virtual context
>
> Then add the following rule:
>
> @ok@
> position p;
> expression src,dest;
> @@
>
> copy_from_user@p(, src, sizeof(dest))
>
> +
> +@cfu_twice@
> +position p;
>
>
> Change this to:
>
> position p != ok.p;
>
> +identifier src;
> +expression dest1, dest2, size1, size2, offset;
> +@@
> +
> +*copy_from_user(dest1, src, size1)
> + ... when != src = offset
> + when != src += offset
>
>
> Here, may be we should add few more lines from Pengfei's
> script to avoid th potential FPs.
>
> Add the following lines:
>
> when != if (size2 > e1 || ...) { ... return ...; }
> when != if (size2 > e1 || ...) { ... size2 = e2 ... }
>
> These changes drop cases where the last argument to copy_from_usr is the
> size of the first argument, which seems safe enough, and where there is a
> test on the size value that can either update it or abort the function.
> These changes only eliminate false positives, as far as I could tell.
>
> If it would be more convenient, I could just send the complete revised
> patch, or whatever seems convenient.
>
>
> I was also thinking that probably we should also add other user space memory
> API functions. May be get_user and strncpy_from_user. Although I'm not sure
> how common it is to find such patterns for both of these functions.
>
>
> I strongly recommend you adding get_user() API , which is used pervasively
> within the kernel just like copy_from user().
>
> In many situations, there is a combination use, get_user() copies first then
> followed by a copy_from_user() copy. According to our investigation, this
> typical
> situation works by get_user() firstly copying a field of a specific struct
> to check,
> then copy_from_user() copies in the whole struct to use. Of course, the
> struct
> field is fetch twice.
For sure, yes. I just split it out initially so we could compare some
of the pieces the two scripts do. Getting the size check into the test
was important to reduce false positives, so I think we need to just
expand the rules a bit more to include the size checks.
-Kees
--
Kees Cook
Nexus Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user
On Mon, Jan 9, 2017 at 12:56 PM, Kees Cook <keesc...@chromium.org> wrote: > On Mon, Jan 9, 2017 at 11:08 AM, Julia Lawall <julia.law...@lip6.fr> wrote: >> >> On Mon, 9 Jan 2017, Vaishali Thakkar wrote: >> >>> Here, may be we should add few more lines from Pengfei's >>> script to avoid th potential FPs. >> >> Which lines (I don't have it handy)? > > I'm going to compare > https://github.com/wpengfei/double_fetch_cocci/blob/master/pattern_match_linux.cocci > to my original one, add your improvements and see what I get... Okay, I finally had time to look at this. Pengfei added two other logical cases that should be checked for, IIUC: 1) destination alias checking (with assignment either before or after the first copy_from_user): struct thing object; struct thing *pointer = copy_from_user(..., ); ... copy_from_user(..., pointer); 2) field writes (via . or ->, instead of short writes): struct thing object; copy_from_user(..., ); ... copy_from_user(..., ); It'd probably better to convert Pengfei's into being able to run under the coccicheck target. -Kees -- Kees Cook Nexus Security ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user
On Mon, Jan 9, 2017 at 11:08 AM, Julia Lawall <julia.law...@lip6.fr> wrote:
>
>
> On Mon, 9 Jan 2017, Vaishali Thakkar wrote:
>
>> On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote:
>> > I totally dropped the ball on this. Many thanks to Vaishali for
>> > resurrecting it.
>> >
>> > Some changes are suggested below.
>> >
>> > On Tue, 26 Apr 2016, Kees Cook wrote:
>> >
>> > > This is usually a sign of a resized request. This adds a check for
>> > > potential races or confusions. The check isn't 100% accurate, so it
>> > > needs some manual review.
>> > >
>> > > Signed-off-by: Kees Cook <keesc...@chromium.org>
>> > > ---
>> > > scripts/coccinelle/tests/reusercopy.cocci | 36
>> > > +++
>> > > 1 file changed, 36 insertions(+)
>> > > create mode 100644 scripts/coccinelle/tests/reusercopy.cocci
>> > >
>> > > diff --git a/scripts/coccinelle/tests/reusercopy.cocci
>> > > b/scripts/coccinelle/tests/reusercopy.cocci
>> > > new file mode 100644
>> > > index ..53645de8ae95
>> > > --- /dev/null
>> > > +++ b/scripts/coccinelle/tests/reusercopy.cocci
>> > > @@ -0,0 +1,36 @@
>> > > +/// Recopying from the same user buffer frequently indicates a pattern
>> > > of
>> > > +/// Reading a size header, allocating, and then re-reading an entire
>> > > +/// structure. If the structure's size is not re-validated, this can
>> > > lead
>> > > +/// to structure or data size confusions.
>> > > +///
>> > > +// Confidence: Moderate
>> > > +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2.
>> > > +// URL: http://coccinelle.lip6.fr/
>> > > +// Comments:
>> > > +// Options: -no_includes -include_headers
>> >
>> > The options could be: --no-include --include-headers
>> >
>> > Actually, Coccinelle supports both, but it only officially supports the
>> > -- versions.
>> >
>> > > +
>> > > +virtual report
>> > > +virtual org
>> >
>> > Add, the following for the *s:
>> >
>> > virtual context
>> >
>> > Then add the following rule:
>> >
>> > @ok@
>> > position p;
>> > expression src,dest;
>> > @@
>> >
>> > copy_from_user@p(, src, sizeof(dest))
>> >
>> > > +
>> > > +@cfu_twice@
>> > > +position p;
>> >
>> > Change this to:
>> >
>> > position p != ok.p;
>> >
>> > > +identifier src;
>> > > +expression dest1, dest2, size1, size2, offset;
>> > > +@@
>> > > +
>> > > +*copy_from_user(dest1, src, size1)
>> > > + ... when != src = offset
>> > > + when != src += offset
>>
>> Here, may be we should add few more lines from Pengfei's
>> script to avoid th potential FPs.
>
> Which lines (I don't have it handy)?
I'm going to compare
https://github.com/wpengfei/double_fetch_cocci/blob/master/pattern_match_linux.cocci
to my original one, add your improvements and see what I get...
-Kees
>
> julia
>
>>
>> > Add the following lines:
>> >
>> > when != if (size2 > e1 || ...) { ... return ...; }
>> > when != if (size2 > e1 || ...) { ... size2 = e2 ... }
>> >
>> > These changes drop cases where the last argument to copy_from_usr is the
>> > size of the first argument, which seems safe enough, and where there is a
>> > test on the size value that can either update it or abort the function.
>> > These changes only eliminate false positives, as far as I could tell.
>> >
>> > If it would be more convenient, I could just send the complete revised
>> > patch, or whatever seems convenient.
>>
>> I was also thinking that probably we should also add other user space memory
>> API functions. May be get_user and strncpy_from_user. Although I'm not sure
>> how common it is to find such patterns for both of these functions.
>>
>> > thanks,
>> > julia
>> >
>> > > +*copy_from_user@p(dest2, src, size2)
>> > > +
>> > > +@script:python depends on org@
>> > > +p << cfu_twice.p;
>> > > +@@
>> > > +
>> > > +cocci.print_main("potentially dangerous second copy_from_user()",p)
>> > > +
>> > > +@script:python depends on report@
>> > > +p << cfu_twice.p;
>> > > +@@
>> > > +
>> > > +coccilib.report.print_report(p[0],"potentially dangerous second
>> > > copy_from_user()")
>> > > --
>> > > 2.6.3
>> > >
>> > >
>> > > --
>> > > Kees Cook
>> > > Chrome OS & Brillo Security
>> > >
>> > ___
>> > Cocci mailing list
>> > Cocci@systeme.lip6.fr
>> > https://systeme.lip6.fr/mailman/listinfo/cocci
>> >
>>
>>
--
Kees Cook
Nexus Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] [PATCH] coccicheck: Fix missing 0 index in kill loop
By default, "seq" counts from 1, but processes were starting counting
from 0, so when interrupted, coccicheck would leave the 0th process
running.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
scripts/coccicheck | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/coccicheck b/scripts/coccicheck
index b2d758188f2f..dd85a455b2ba 100755
--- a/scripts/coccicheck
+++ b/scripts/coccicheck
@@ -98,7 +98,7 @@ run_cmd() {
}
kill_running() {
- for i in $(seq $(( NPROC - 1 )) ); do
+ for i in $(seq 0 $(( NPROC - 1 )) ); do
if [ $VERBOSE -eq 2 ] ; then
echo "Killing ${SPATCH_PID[$i]}"
fi
--
2.6.3
--
Kees Cook
Chrome OS & Brillo Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user
On Tue, Apr 26, 2016 at 3:24 PM, Kees Cook <keesc...@chromium.org> wrote:
> This is usually a sign of a resized request. This adds a check for
> potential races or confusions. The check isn't 100% accurate, so it
> needs some manual review.
>
> Signed-off-by: Kees Cook <keesc...@chromium.org>
> ---
> scripts/coccinelle/tests/reusercopy.cocci | 36
> +++
> 1 file changed, 36 insertions(+)
> create mode 100644 scripts/coccinelle/tests/reusercopy.cocci
>
> diff --git a/scripts/coccinelle/tests/reusercopy.cocci
> b/scripts/coccinelle/tests/reusercopy.cocci
> new file mode 100644
> index ..53645de8ae95
> --- /dev/null
> +++ b/scripts/coccinelle/tests/reusercopy.cocci
> @@ -0,0 +1,36 @@
> +/// Recopying from the same user buffer frequently indicates a pattern of
> +/// Reading a size header, allocating, and then re-reading an entire
> +/// structure. If the structure's size is not re-validated, this can lead
> +/// to structure or data size confusions.
> +///
> +// Confidence: Moderate
> +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2.
> +// URL: http://coccinelle.lip6.fr/
> +// Comments:
> +// Options: -no_includes -include_headers
> +
> +virtual report
> +virtual org
> +
> +@cfu_twice@
> +position p;
> +identifier src;
> +expression dest1, dest2, size1, size2, offset;
> +@@
> +
> +*copy_from_user(dest1, src, size1)
> + ... when != src = offset
> + when != src += offset
> +*copy_from_user@p(dest2, src, size2)
> +
> +@script:python depends on org@
> +p << cfu_twice.p;
> +@@
> +
> +cocci.print_main("potentially dangerous second copy_from_user()",p)
> +
> +@script:python depends on report@
> +p << cfu_twice.p;
> +@@
> +
> +coccilib.report.print_report(p[0],"potentially dangerous second
> copy_from_user()")
> --
> 2.6.3
>
>
> --
> Kees Cook
> Chrome OS & Brillo Security
And here's the current (noisy) output:
./arch/powerpc/platforms/powernv/opal-prd.c:248:6-20: potentially
dangerous second copy_from_user()
./sound/isa/sb/sb16_csp.c:391:7-21: potentially dangerous second
copy_from_user()
./drivers/gpu/drm/tegra/drm.c:361:6-20: potentially dangerous second
copy_from_user()
./fs/exec.c:537:7-21: potentially dangerous second copy_from_user()
./drivers/char/lp.c:387:7-21: potentially dangerous second copy_from_user()
./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2149:6-20:
potentially dangerous second copy_from_user()
./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2247:6-20:
potentially dangerous second copy_from_user()
./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2292:6-20:
potentially dangerous second copy_from_user()
./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2332:6-20:
potentially dangerous second copy_from_user()
./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2355:6-20:
potentially dangerous second copy_from_user()
./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2396:6-20:
potentially dangerous second copy_from_user()
./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2429:6-20:
potentially dangerous second copy_from_user()
./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2481:6-20:
potentially dangerous second copy_from_user()
./arch/ia64/kernel/perfmon.c:4833:11-25: potentially dangerous second
copy_from_user()
./drivers/staging/i4l/icn/icn.c:1048:7-21: potentially dangerous
second copy_from_user()
./drivers/misc/mic/vop/vop_vringh.c:775:9-23: potentially dangerous
second copy_from_user()
./drivers/misc/mic/vop/vop_vringh.c:944:6-20: potentially dangerous
second copy_from_user()
./fs/coda/psdev.c:128:6-20: potentially dangerous second copy_from_user()
./fs/coda/psdev.c:174:12-26: potentially dangerous second copy_from_user()
./drivers/hid/hid-picolcd_debugfs.c:283:6-20: potentially dangerous
second copy_from_user()
./fs/aio.c:1610:15-29: potentially dangerous second copy_from_user()
./fs/splice.c:1459:6-20: potentially dangerous second copy_from_user()
./kernel/kexec_core.c:815:12-26: potentially dangerous second copy_from_user()
./kernel/kexec_core.c:752:12-26: potentially dangerous second copy_from_user()
./drivers/scsi/3w-9xxx.c:691:5-19: potentially dangerous second copy_from_user()
./drivers/scsi/3w-.c:921:5-19: potentially dangerous second copy_from_user()
./drivers/platform/chrome/cros_ec_dev.c:145:5-19: potentially
dangerous second copy_from_user()
./sound/drivers/opl3/opl3_synth.c:204:6-20: potentially dangerous
second copy_from_user()
./drivers/scsi/megaraid.c:3465:5-19: potentially dangerous second
copy_from_user()
./drivers/scsi/aacraid/commctrl.c:116:5-19: potentially dangerous
second copy_from_user()
./drivers/staging/lustre/lustre/obdclass/linux/linux-module.c:116:5-19:
potentially dangerous second copy_from_user()
./drivers/scsi/dpt_i2o.c:1847:6-20: potentially dangerous sec
[Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user
This is usually a sign of a resized request. This adds a check for
potential races or confusions. The check isn't 100% accurate, so it
needs some manual review.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
scripts/coccinelle/tests/reusercopy.cocci | 36 +++
1 file changed, 36 insertions(+)
create mode 100644 scripts/coccinelle/tests/reusercopy.cocci
diff --git a/scripts/coccinelle/tests/reusercopy.cocci
b/scripts/coccinelle/tests/reusercopy.cocci
new file mode 100644
index ..53645de8ae95
--- /dev/null
+++ b/scripts/coccinelle/tests/reusercopy.cocci
@@ -0,0 +1,36 @@
+/// Recopying from the same user buffer frequently indicates a pattern of
+/// Reading a size header, allocating, and then re-reading an entire
+/// structure. If the structure's size is not re-validated, this can lead
+/// to structure or data size confusions.
+///
+// Confidence: Moderate
+// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2.
+// URL: http://coccinelle.lip6.fr/
+// Comments:
+// Options: -no_includes -include_headers
+
+virtual report
+virtual org
+
+@cfu_twice@
+position p;
+identifier src;
+expression dest1, dest2, size1, size2, offset;
+@@
+
+*copy_from_user(dest1, src, size1)
+ ... when != src = offset
+ when != src += offset
+*copy_from_user@p(dest2, src, size2)
+
+@script:python depends on org@
+p << cfu_twice.p;
+@@
+
+cocci.print_main("potentially dangerous second copy_from_user()",p)
+
+@script:python depends on report@
+p << cfu_twice.p;
+@@
+
+coccilib.report.print_report(p[0],"potentially dangerous second
copy_from_user()")
--
2.6.3
--
Kees Cook
Chrome OS & Brillo Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
