[PATCH 0/2] Update memdup_user.cocci

[PATCH 0/2] Update memdup_user.cocci

[Denis Efremov]

Add GFP_USER to the allocation flags and handle vmemdup_user().

Denis Efremov (2):
  Coccinelle: extend memdup_user transformation with GFP_USER
  Coccinelle: extend memdup_user rule with vmemdup_user()

 scripts/coccinelle/api/memdup_user.cocci | 53 ++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 4 deletions(-)

-- 
2.26.2

[PATCH 1/2] Coccinelle: extend memdup_user transformation with GFP_USER

[Denis Efremov]

Match GFP_USER allocations with memdup_user.cocci rule.
Commit 6c2c97a24f09 ("memdup_user(): switch to GFP_USER") switched
memdup_user() from GFP_KERNEL to GFP_USER. In most cases it is still
a good idea to use memdup_user() for GFP_KERNEL allocations. The
motivation behind altering memdup_user() to GFP_USER is here:
https://lkml.org/lkml/2018/1/6/333

Signed-off-by: Denis Efremov <efremov@linux.com>
---
 scripts/coccinelle/api/memdup_user.cocci | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/coccinelle/api/memdup_user.cocci b/scripts/coccinelle/api/memdup_user.cocci
index c809ab10bbce..49f487e6a5c8 100644
--- a/scripts/coccinelle/api/memdup_user.cocci
+++ b/scripts/coccinelle/api/memdup_user.cocci
@@ -20,7 +20,7 @@ expression from,to,size;
 identifier l1,l2;
 @@
 
--  to = \(kmalloc\|kzalloc\)(size,GFP_KERNEL);
+-  to = \(kmalloc\|kzalloc\)(size,\(GFP_KERNEL\|GFP_USER\));
 +  to = memdup_user(from,size);
    if (
 -      to==NULL
@@ -43,7 +43,7 @@ position p;
 statement S1,S2;
 @@
 
-*  to = \(kmalloc@p\|kzalloc@p\)(size,GFP_KERNEL);
+*  to = \(kmalloc@p\|kzalloc@p\)(size,\(GFP_KERNEL\|GFP_USER\));
    if (to==NULL || ...) S1
    if (copy_from_user(to, from, size) != 0)
    S2
-- 
2.26.2

[PATCH 2/2] Coccinelle: extend memdup_user rule with vmemdup_user()

[Denis Efremov]

Add vmemdup_user() transformations to the memdup_user.cocci rule.
Commit 50fd2f298bef ("new primitive: vmemdup_user()") introduced
vmemdup_user(). The function uses kvmalloc with GPF_USER flag.

Signed-off-by: Denis Efremov <efremov@linux.com>
---
 scripts/coccinelle/api/memdup_user.cocci | 49 +++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 2 deletions(-)

diff --git a/scripts/coccinelle/api/memdup_user.cocci b/scripts/coccinelle/api/memdup_user.cocci
index 49f487e6a5c8..a50def35136e 100644
--- a/scripts/coccinelle/api/memdup_user.cocci
+++ b/scripts/coccinelle/api/memdup_user.cocci
@@ -37,6 +37,28 @@ identifier l1,l2;
 -    ...+>
 -  }
 
+@depends on patch@
+expression from,to,size;
+identifier l1,l2;
+@@
+
+-  to = \(kvmalloc\|kvzalloc\)(size,\(GFP_KERNEL\|GFP_USER\));
++  to = vmemdup_user(from,size);
+   if (
+-      to==NULL
++      IS_ERR(to)
+                 || ...) {
+   <+... when != goto l1;
+-  -ENOMEM
++  PTR_ERR(to)
+   ...+>
+   }
+-  if (copy_from_user(to, from, size) != 0) {
+-    <+... when != goto l2;
+-    -EFAULT
+-    ...+>
+-  }
+
 @r depends on !patch@
 expression from,to,size;
 position p;
@@ -48,14 +70,37 @@ statement S1,S2;
    if (copy_from_user(to, from, size) != 0)
    S2
 
-@script:python depends on org@
+@rv depends on !patch@
+expression from,to,size;
+position p;
+statement S1,S2;
+@@
+
+*  to = \(kvmalloc@p\|kvzalloc@p\)(size,\(GFP_KERNEL\|GFP_USER\));
+   if (to==NULL || ...) S1
+   if (copy_from_user(to, from, size) != 0)
+   S2
+
+@script:python depends on org && r@
 p << r.p;
 @@
 
 coccilib.org.print_todo(p[0], "WARNING opportunity for memdup_user")
 
-@script:python depends on report@
+@script:python depends on report && r@
 p << r.p;
 @@
 
 coccilib.report.print_report(p[0], "WARNING opportunity for memdup_user")
+
+@script:python depends on org && rv@
+p << rv.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING opportunity for vmemdup_user")
+
+@script:python depends on report && rv@
+p << rv.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING opportunity for vmemdup_user")
-- 
2.26.2

Re: [Cocci] [PATCH 1/2] Coccinelle: extend memdup_user transformation with GFP_USER

[Julia Lawall]

On Sat, 30 May 2020, Denis Efremov wrote:

> Match GFP_USER allocations with memdup_user.cocci rule.
> Commit 6c2c97a24f09 ("memdup_user(): switch to GFP_USER") switched
> memdup_user() from GFP_KERNEL to GFP_USER. In most cases it is still
> a good idea to use memdup_user() for GFP_KERNEL allocations. The
> motivation behind altering memdup_user() to GFP_USER is here:
> https://lkml.org/lkml/2018/1/6/333

Thanks for the patch series.  I will test them and try to push them to
Linus shortly.

julia


>
> Signed-off-by: Denis Efremov <efremov@linux.com>
> ---
>  scripts/coccinelle/api/memdup_user.cocci | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/coccinelle/api/memdup_user.cocci b/scripts/coccinelle/api/memdup_user.cocci
> index c809ab10bbce..49f487e6a5c8 100644
> --- a/scripts/coccinelle/api/memdup_user.cocci
> +++ b/scripts/coccinelle/api/memdup_user.cocci
> @@ -20,7 +20,7 @@ expression from,to,size;
>  identifier l1,l2;
>  @@
>
> --  to = \(kmalloc\|kzalloc\)(size,GFP_KERNEL);
> +-  to = \(kmalloc\|kzalloc\)(size,\(GFP_KERNEL\|GFP_USER\));
>  +  to = memdup_user(from,size);
>     if (
>  -      to==NULL
> @@ -43,7 +43,7 @@ position p;
>  statement S1,S2;
>  @@
>
> -*  to = \(kmalloc@p\|kzalloc@p\)(size,GFP_KERNEL);
> +*  to = \(kmalloc@p\|kzalloc@p\)(size,\(GFP_KERNEL\|GFP_USER\));
>     if (to==NULL || ...) S1
>     if (copy_from_user(to, from, size) != 0)
>     S2
> --
> 2.26.2
>
> _______________________________________________
> Cocci mailing list
> Cocci@systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
>

Re: [Cocci] [PATCH 1/2] Coccinelle: extend memdup_user transformation with GFP_USER

[Julia Lawall]

On Sat, 30 May 2020, Denis Efremov wrote:

> Match GFP_USER allocations with memdup_user.cocci rule.
> Commit 6c2c97a24f09 ("memdup_user(): switch to GFP_USER") switched
> memdup_user() from GFP_KERNEL to GFP_USER. In most cases it is still
> a good idea to use memdup_user() for GFP_KERNEL allocations. The
> motivation behind altering memdup_user() to GFP_USER is here:
> https://lkml.org/lkml/2018/1/6/333

Should the rule somehow document the cases in which memdup_user should now
not be used?

julia


>
> Signed-off-by: Denis Efremov <efremov@linux.com>
> ---
>  scripts/coccinelle/api/memdup_user.cocci | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/coccinelle/api/memdup_user.cocci b/scripts/coccinelle/api/memdup_user.cocci
> index c809ab10bbce..49f487e6a5c8 100644
> --- a/scripts/coccinelle/api/memdup_user.cocci
> +++ b/scripts/coccinelle/api/memdup_user.cocci
> @@ -20,7 +20,7 @@ expression from,to,size;
>  identifier l1,l2;
>  @@
>
> --  to = \(kmalloc\|kzalloc\)(size,GFP_KERNEL);
> +-  to = \(kmalloc\|kzalloc\)(size,\(GFP_KERNEL\|GFP_USER\));
>  +  to = memdup_user(from,size);
>     if (
>  -      to==NULL
> @@ -43,7 +43,7 @@ position p;
>  statement S1,S2;
>  @@
>
> -*  to = \(kmalloc@p\|kzalloc@p\)(size,GFP_KERNEL);
> +*  to = \(kmalloc@p\|kzalloc@p\)(size,\(GFP_KERNEL\|GFP_USER\));
>     if (to==NULL || ...) S1
>     if (copy_from_user(to, from, size) != 0)
>     S2
> --
> 2.26.2
>
> _______________________________________________
> Cocci mailing list
> Cocci@systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
>

Re: [Cocci] [PATCH 2/2] Coccinelle: extend memdup_user rule with vmemdup_user()

[Julia Lawall]

On Sat, 30 May 2020, Denis Efremov wrote:

> Add vmemdup_user() transformations to the memdup_user.cocci rule.
> Commit 50fd2f298bef ("new primitive: vmemdup_user()") introduced
> vmemdup_user(). The function uses kvmalloc with GPF_USER flag.
>
> Signed-off-by: Denis Efremov <efremov@linux.com>
> ---
>  scripts/coccinelle/api/memdup_user.cocci | 49 +++++++++++++++++++++++-
>  1 file changed, 47 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/coccinelle/api/memdup_user.cocci b/scripts/coccinelle/api/memdup_user.cocci
> index 49f487e6a5c8..a50def35136e 100644
> --- a/scripts/coccinelle/api/memdup_user.cocci
> +++ b/scripts/coccinelle/api/memdup_user.cocci
> @@ -37,6 +37,28 @@ identifier l1,l2;
>  -    ...+>
>  -  }
>
> +@depends on patch@
> +expression from,to,size;
> +identifier l1,l2;
> +@@
> +
> +-  to = \(kvmalloc\|kvzalloc\)(size,\(GFP_KERNEL\|GFP_USER\));
> ++  to = vmemdup_user(from,size);
> +   if (
> +-      to==NULL
> ++      IS_ERR(to)
> +                 || ...) {
> +   <+... when != goto l1;
> +-  -ENOMEM
> ++  PTR_ERR(to)
> +   ...+>
> +   }
> +-  if (copy_from_user(to, from, size) != 0) {
> +-    <+... when != goto l2;
> +-    -EFAULT
> +-    ...+>
> +-  }
> +

This could protect against modifying vmemdup_user.  Probably the original
rule should protect against modifying memdup_user as well.

julia




>  @r depends on !patch@
>  expression from,to,size;
>  position p;
> @@ -48,14 +70,37 @@ statement S1,S2;
>     if (copy_from_user(to, from, size) != 0)
>     S2
>
> -@script:python depends on org@
> +@rv depends on !patch@
> +expression from,to,size;
> +position p;
> +statement S1,S2;
> +@@
> +
> +*  to = \(kvmalloc@p\|kvzalloc@p\)(size,\(GFP_KERNEL\|GFP_USER\));
> +   if (to==NULL || ...) S1
> +   if (copy_from_user(to, from, size) != 0)
> +   S2
> +
> +@script:python depends on org && r@
>  p << r.p;
>  @@
>
>  coccilib.org.print_todo(p[0], "WARNING opportunity for memdup_user")
>
> -@script:python depends on report@
> +@script:python depends on report && r@
>  p << r.p;
>  @@
>
>  coccilib.report.print_report(p[0], "WARNING opportunity for memdup_user")
> +
> +@script:python depends on org && rv@
> +p << rv.p;
> +@@
> +
> +coccilib.org.print_todo(p[0], "WARNING opportunity for vmemdup_user")
> +
> +@script:python depends on report && rv@
> +p << rv.p;
> +@@
> +
> +coccilib.report.print_report(p[0], "WARNING opportunity for vmemdup_user")
> --
> 2.26.2
>
> _______________________________________________
> Cocci mailing list
> Cocci@systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
>

Re: [Cocci] [PATCH 1/2] Coccinelle: extend memdup_user transformation with GFP_USER

[Denis Efremov]

On 6/6/20 11:24 AM, Julia Lawall wrote:
> 
> 
> On Sat, 30 May 2020, Denis Efremov wrote:
> 
>> Match GFP_USER allocations with memdup_user.cocci rule.
>> Commit 6c2c97a24f09 ("memdup_user(): switch to GFP_USER") switched
>> memdup_user() from GFP_KERNEL to GFP_USER. In most cases it is still
>> a good idea to use memdup_user() for GFP_KERNEL allocations. The
>> motivation behind altering memdup_user() to GFP_USER is here:
>> https://lkml.org/lkml/2018/1/6/333
> 
> Should the rule somehow document the cases in which memdup_user should now
> not be used?

As for now, I can't provide a counterexample. GPF_USER is more permissive than
GFP_KERNEL. It's completely ok to use GPF_USER with copy_from_user. Given that
memdup_user() was "silently" switched to GPF_USER from GPF_KERNEL with no callside
fixes, I think it's ok to recommend to use memdup_user for GPF_KERNEL matches with
no additional restrictions.

Thanks,
Denis

Re: [Cocci] [PATCH 1/2] Coccinelle: extend memdup_user transformation with GFP_USER

[Denis Efremov]

On 6/6/20 11:24 AM, Julia Lawall wrote:
> 
> 
> On Sat, 30 May 2020, Denis Efremov wrote:
> 
>> Match GFP_USER allocations with memdup_user.cocci rule.
>> Commit 6c2c97a24f09 ("memdup_user(): switch to GFP_USER") switched
>> memdup_user() from GFP_KERNEL to GFP_USER. In most cases it is still
>> a good idea to use memdup_user() for GFP_KERNEL allocations. The
>> motivation behind altering memdup_user() to GFP_USER is here:
>> https://lkml.org/lkml/2018/1/6/333
> 
> Should the rule somehow document the cases in which memdup_user should now
> not be used?
> 
> julia
> 
> 
>>
>> Signed-off-by: Denis Efremov <efremov@linux.com>
>> ---
>>  scripts/coccinelle/api/memdup_user.cocci | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/scripts/coccinelle/api/memdup_user.cocci b/scripts/coccinelle/api/memdup_user.cocci
>> index c809ab10bbce..49f487e6a5c8 100644
>> --- a/scripts/coccinelle/api/memdup_user.cocci
>> +++ b/scripts/coccinelle/api/memdup_user.cocci
>> @@ -20,7 +20,7 @@ expression from,to,size;
>>  identifier l1,l2;
>>  @@
>>
>> --  to = \(kmalloc\|kzalloc\)(size,GFP_KERNEL);
>> +-  to = \(kmalloc\|kzalloc\)(size,\(GFP_KERNEL\|GFP_USER\));

Actually, we can add optional __GFP_NOWARN here to match such cases as:
GFP_KERNEL | __GFP_NOWARN

However, I don't know how to express it in elegant way. Something like?
(
-  to = \(kmalloc\|kzalloc\)(size,\(GFP_KERNEL\|GFP_USER\));
|
-  to = \(kmalloc\|kzalloc\)(size, GFP_KERNEL|__GFP_NOWARN);
|
-  to = \(kmalloc\|kzalloc\)(size, GFP_USER|__GFP_NOWARN);
)

Thanks,
Denis

Re: [Cocci] [PATCH 1/2] Coccinelle: extend memdup_user transformation with GFP_USER

[Julia Lawall]

On Sat, 6 Jun 2020, Denis Efremov wrote:

>
>
> On 6/6/20 11:24 AM, Julia Lawall wrote:
> >
> >
> > On Sat, 30 May 2020, Denis Efremov wrote:
> >
> >> Match GFP_USER allocations with memdup_user.cocci rule.
> >> Commit 6c2c97a24f09 ("memdup_user(): switch to GFP_USER") switched
> >> memdup_user() from GFP_KERNEL to GFP_USER. In most cases it is still
> >> a good idea to use memdup_user() for GFP_KERNEL allocations. The
> >> motivation behind altering memdup_user() to GFP_USER is here:
> >> https://lkml.org/lkml/2018/1/6/333
> >
> > Should the rule somehow document the cases in which memdup_user should now
> > not be used?
> >
> > julia
> >
> >
> >>
> >> Signed-off-by: Denis Efremov <efremov@linux.com>
> >> ---
> >>  scripts/coccinelle/api/memdup_user.cocci | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/scripts/coccinelle/api/memdup_user.cocci b/scripts/coccinelle/api/memdup_user.cocci
> >> index c809ab10bbce..49f487e6a5c8 100644
> >> --- a/scripts/coccinelle/api/memdup_user.cocci
> >> +++ b/scripts/coccinelle/api/memdup_user.cocci
> >> @@ -20,7 +20,7 @@ expression from,to,size;
> >>  identifier l1,l2;
> >>  @@
> >>
> >> --  to = \(kmalloc\|kzalloc\)(size,GFP_KERNEL);
> >> +-  to = \(kmalloc\|kzalloc\)(size,\(GFP_KERNEL\|GFP_USER\));
>
> Actually, we can add optional __GFP_NOWARN here to match such cases as:
> GFP_KERNEL | __GFP_NOWARN
>
> However, I don't know how to express it in elegant way. Something like?
> (
> -  to = \(kmalloc\|kzalloc\)(size,\(GFP_KERNEL\|GFP_USER\));
> |
> -  to = \(kmalloc\|kzalloc\)(size, GFP_KERNEL|__GFP_NOWARN);
> |
> -  to = \(kmalloc\|kzalloc\)(size, GFP_USER|__GFP_NOWARN);
> )

I guess you could do:

\(GFP_KERNEL\|GFP_USER\|\(GFP_KERNEL\|GFP_USER\)|__GFP_NOWARN\)

julia