Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1471416 > unrolled thread

[PATCH 1/1] ASoC: Intel: Atom: add a missing star in a memcpy call

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/1] ASoC: Intel: Atom: add a missing star in a memcpy call Nicolas Iooss <nicolas.iooss_linux@m4x.org> - 2016-08-28 19:50 +0200
    Misuses of ** ? (was Re: [PATCH 1/1] ASoC: Intel: Atom: add a  missing star in a memcpy call) Joe Perches <joe@perches.com> - 2016-08-28 20:00 +0200
      Re: Misuses of ** ? (was Re: [PATCH 1/1] ASoC: Intel: Atom: add a  missing star in a memcpy call) Nicolas Iooss <nicolas.iooss_linux@m4x.org> - 2016-08-28 21:00 +0200
        Re: Misuses of ** ? (was Re: [PATCH 1/1] ASoC: Intel: Atom: add a  missing star in a memcpy call) Julia Lawall <julia.lawall@lip6.fr> - 2016-08-28 21:40 +0200
          Re: Misuses of ** ? (was Re: [PATCH 1/1] ASoC: Intel: Atom: add a  missing star in a memcpy call) Joe Perches <joe@perches.com> - 2016-08-28 22:40 +0200
            Re: Misuses of ** ? (was Re: [PATCH 1/1] ASoC: Intel: Atom: add a  missing star in a memcpy call) Julia Lawall <julia.lawall@lip6.fr> - 2016-08-28 23:50 +0200
              Re: Misuses of ** ? (was Re: [PATCH 1/1] ASoC: Intel: Atom: add a  missing star in a memcpy call) Joe Perches <joe@perches.com> - 2016-08-29 00:00 +0200
    Re: [PATCH 1/1] ASoC: Intel: Atom: add a missing star in a memcpy  call Joe Perches <joe@perches.com> - 2016-08-28 20:20 +0200
      Re: [PATCH 1/1] ASoC: Intel: Atom: add a missing star in a memcpy  call Nicolas Iooss <nicolas.iooss_linux@m4x.org> - 2016-08-28 20:40 +0200

#1471416 — [PATCH 1/1] ASoC: Intel: Atom: add a missing star in a memcpy call

In sst_prepare_and_post_msg(), when a response is received in "block",
the following code gets executed:

    *data = kzalloc(block->size, GFP_KERNEL);
    memcpy(data, (void *) block->data, block->size);

The memcpy() call overwrites the content of the *data pointer instead of
filling the newly-allocated memory (which pointer is hold by *data).
Fix this by using *data in the memcpy() call.

Fixes: 60dc8dbacb00 ("ASoC: Intel: sst: Add some helper functions")
Cc: stable@vger.kernel.org # 3.19.x
Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
---
 sound/soc/intel/atom/sst/sst_pvt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/intel/atom/sst/sst_pvt.c b/sound/soc/intel/atom/sst/sst_pvt.c
index adb32fefd693..7c398b7c9d4b 100644
--- a/sound/soc/intel/atom/sst/sst_pvt.c
+++ b/sound/soc/intel/atom/sst/sst_pvt.c
@@ -289,7 +289,7 @@ int sst_prepare_and_post_msg(struct intel_sst_drv *sst,
 				ret = -ENOMEM;
 				goto out;
 			} else
-				memcpy(data, (void *) block->data, block->size);
+				memcpy(*data, (void *) block->data, block->size);
 		}
 	}
 out:
-- 
2.9.3

[toc] | [next] | [standalone]

#1471425 — Misuses of ** ? (was Re: [PATCH 1/1] ASoC: Intel: Atom: add a missing star in a memcpy call)

On Sun, 2016-08-28 at 19:39 +0200, Nicolas Iooss wrote:
> In sst_prepare_and_post_msg(), when a response is received in "block",
> the following code gets executed:
> 
>     *data = kzalloc(block->size, GFP_KERNEL);
>     memcpy(data, (void *) block->data, block->size);

Yuck, thanks.

Julia, Dan, could cocci or smatch help find any other
similar misuses here?

[toc] | [prev] | [next] | [standalone]

#1471439 — Re: Misuses of ** ? (was Re: [PATCH 1/1] ASoC: Intel: Atom: add a missing star in a memcpy call)

On 28/08/16 19:50, Joe Perches wrote:
> On Sun, 2016-08-28 at 19:39 +0200, Nicolas Iooss wrote:
>> In sst_prepare_and_post_msg(), when a response is received in "block",
>> the following code gets executed:
>>
>>     *data = kzalloc(block->size, GFP_KERNEL);
>>     memcpy(data, (void *) block->data, block->size);
> 
> Yuck, thanks.
> 
> Julia, Dan, could cocci or smatch help find any other
> similar misuses here?

In fact I have found this bug with a GCC plugin I have written after I
discovered an issue with a printf format string in brcmfmac driver
(https://lkml.org/lkml/2016/8/23/193 fixes this one). This GCC plugin
uses an approach which has many false positives but it helped me detect
real bugs such as the one you replied to, and
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=ae6c33ba6e37eea3012fe2640b22400ef3f2d0f3
a few days ago.

In case you are curious about what the plugin looks like (it is very
dirty but might be useful for future work I won't have time to do), I
published it on
https://gist.github.com/anonymous/36dd40dcbeeb83964e66b65be7a96136 .
This huge patch contains the plugin code in
scripts/gcc-plugins/deref_checker_plugin.c, many dirty work-arounds to
filter false positive matches, a really-dirty way of handling memcpy
optimisations done by gcc, and fixes to possible bugs (which can be
found by searching "/* BUG? */", I have not yet had time to find out
whether they are real bugs or false positives too).

I hope this will help in the work of eliminating bugs in the kernel :)

-- Nicolas

[toc] | [prev] | [next] | [standalone]

#1471445 — Re: Misuses of ** ? (was Re: [PATCH 1/1] ASoC: Intel: Atom: add a missing star in a memcpy call)

On Sun, 28 Aug 2016, Nicolas Iooss wrote:

> On 28/08/16 19:50, Joe Perches wrote:
> > On Sun, 2016-08-28 at 19:39 +0200, Nicolas Iooss wrote:
> >> In sst_prepare_and_post_msg(), when a response is received in "block",
> >> the following code gets executed:
> >>
> >>     *data = kzalloc(block->size, GFP_KERNEL);
> >>     memcpy(data, (void *) block->data, block->size);
> >
> > Yuck, thanks.
> >
> > Julia, Dan, could cocci or smatch help find any other
> > similar misuses here?
>
> In fact I have found this bug with a GCC plugin I have written after I
> discovered an issue with a printf format string in brcmfmac driver
> (https://lkml.org/lkml/2016/8/23/193 fixes this one). This GCC plugin
> uses an approach which has many false positives but it helped me detect
> real bugs such as the one you replied to, and
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=ae6c33ba6e37eea3012fe2640b22400ef3f2d0f3
> a few days ago.
>
> In case you are curious about what the plugin looks like (it is very
> dirty but might be useful for future work I won't have time to do), I
> published it on
> https://gist.github.com/anonymous/36dd40dcbeeb83964e66b65be7a96136 .
> This huge patch contains the plugin code in
> scripts/gcc-plugins/deref_checker_plugin.c, many dirty work-arounds to
> filter false positive matches, a really-dirty way of handling memcpy
> optimisations done by gcc, and fixes to possible bugs (which can be
> found by searching "/* BUG? */", I have not yet had time to find out
> whether they are real bugs or false positives too).
>
> I hope this will help in the work of eliminating bugs in the kernel :)

I tried the following semantic patch, that is quite general, and the fixed
issue was the only report.

@@
expression x,y,sz;
identifier f,g;
@@

* *x = f(sz,...);
  ...
* g(x,y,sz);

julia

[toc] | [prev] | [next] | [standalone]

#1471450 — Re: Misuses of ** ? (was Re: [PATCH 1/1] ASoC: Intel: Atom: add a missing star in a memcpy call)

On Sun, 2016-08-28 at 21:38 +0200, Julia Lawall wrote:
> On Sun, 28 Aug 2016, Nicolas Iooss wrote:
> > On 28/08/16 19:50, Joe Perches wrote:
> > > On Sun, 2016-08-28 at 19:39 +0200, Nicolas Iooss wrote:
> > >> In sst_prepare_and_post_msg(), when a response is received in "block",
> > >> the following code gets executed:
> > >>
> > >>     *data = kzalloc(block->size, GFP_KERNEL);
> > >>     memcpy(data, (void *) block->data, block->size);
> > >
> > > Yuck, thanks.
> > >
> > > Julia, Dan, could cocci or smatch help find any other
> > > similar misuses here?
[]
> I tried the following semantic patch, that is quite general, and the fixed
> issue was the only report.
> 
> @@
> expression x,y,sz;
> identifier f,g;
> @@
> 
> * *x = f(sz,...);
>   ...
> * g(x,y,sz);

Hi Julia,

This would find exactly the same form, but I think
the question is are there assignments of a **pp
that should have been *pp

Something like:

@@
type P;
P **pp;
@@

* pp = <alloc>\|<copy>\|<access>(..., sizeof(P), ...)

[toc] | [prev] | [next] | [standalone]

#1471476 — Re: Misuses of ** ? (was Re: [PATCH 1/1] ASoC: Intel: Atom: add a missing star in a memcpy call)

[Multipart message — attachments visible in raw view] — view raw

On Sun, 28 Aug 2016, Joe Perches wrote:

> On Sun, 2016-08-28 at 21:38 +0200, Julia Lawall wrote:
> > On Sun, 28 Aug 2016, Nicolas Iooss wrote:
> > > On 28/08/16 19:50, Joe Perches wrote:
> > > > On Sun, 2016-08-28 at 19:39 +0200, Nicolas Iooss wrote:
> > > >> In sst_prepare_and_post_msg(), when a response is received in "block",
> > > >> the following code gets executed:
> > > >>
> > > >>     *data = kzalloc(block->size, GFP_KERNEL);
> > > >>     memcpy(data, (void *) block->data, block->size);
> > > >
> > > > Yuck, thanks.
> > > >
> > > > Julia, Dan, could cocci or smatch help find any other
> > > > similar misuses here?
> []
> > I tried the following semantic patch, that is quite general, and the fixed
> > issue was the only report.
> >
> > @@
> > expression x,y,sz;
> > identifier f,g;
> > @@
> >
> > * *x = f(sz,...);
> >   ...
> > * g(x,y,sz);
>
> Hi Julia,
>
> This would find exactly the same form, but I think
> the question is are there assignments of a **pp
> that should have been *pp
>
> Something like:
>
> @@
> type P;
> P **pp;
> @@
>
> * pp = <alloc>\|<copy>\|<access>(..., sizeof(P), ...)

I didn't get anything for this.  Did you mean for the left hand side of
the assignment to be pp or *pp?  Is the issue that the type is wrong?

julia

[toc] | [prev] | [next] | [standalone]

#1471480 — Re: Misuses of ** ? (was Re: [PATCH 1/1] ASoC: Intel: Atom: add a missing star in a memcpy call)

On Sun, 2016-08-28 at 23:40 +0200, Julia Lawall wrote:
> On Sun, 28 Aug 2016, Joe Perches wrote:
> > On Sun, 2016-08-28 at 21:38 +0200, Julia Lawall wrote:
> > > On Sun, 28 Aug 2016, Nicolas Iooss wrote:
> > > > On 28/08/16 19:50, Joe Perches wrote:
> > > > > On Sun, 2016-08-28 at 19:39 +0200, Nicolas Iooss wrote:
> > > > >> In sst_prepare_and_post_msg(), when a response is received in "block",
> > > > >> the following code gets executed:
> > > > >>
> > > > >>     *data = kzalloc(block->size, GFP_KERNEL);
> > > > >>     memcpy(data, (void *) block->data, block->size);
> > > > >
> > > > > Yuck, thanks.
> > > > >
> > > > > Julia, Dan, could cocci or smatch help find any other
> > > > > similar misuses here?
> > []
> > > I tried the following semantic patch, that is quite general, and the fixed
> > > issue was the only report.
> > >
> > > @@
> > > expression x,y,sz;
> > > identifier f,g;
> > > @@
> > >
> > > * *x = f(sz,...);
> > >   ...
> > > * g(x,y,sz);
> >
> > Hi Julia,
> >
> > This would find exactly the same form, but I think
> > the question is are there assignments of a **pp
> > that should have been *pp
> >
> > Something like:
> >
> > @@
> > type P;
> > P **pp;
> > @@
> >
> > * pp = \|\|(..., sizeof(P), ...)

> I didn't get anything for this.  Did you mean for the left hand side of
> the assignment to be pp or *pp?  Is the issue that the type is wrong?

Yes, the issue here is the type may be wrong.

A function passed a ** and assigned like:

    type function foo(type **bar)
    {
    	    ...
    	    bar = baz();
    	    ...
    }

bar is rarely correct and *bar is generally correct.

I suppose the example would have been clearer with something

-	pp = foo;
+	*pp = foo;

Also, any function that calls another function with
implicit casts to void * from a specific type **pp
after an assignment to *pp could be suspect.

[toc] | [prev] | [next] | [standalone]

#1471432 — Re: [PATCH 1/1] ASoC: Intel: Atom: add a missing star in a memcpy call

On Sun, 2016-08-28 at 19:39 +0200, Nicolas Iooss wrote:
> In sst_prepare_and_post_msg(), when a response is received in "block",
> the following code gets executed:
> 
>     *data = kzalloc(block->size, GFP_KERNEL);
>     memcpy(data, (void *) block->data, block->size);
> 
> The memcpy() call overwrites the content of the *data pointer instead of
> filling the newly-allocated memory (which pointer is hold by *data).
> Fix this by using *data in the memcpy() call.
> 
> Fixes: 60dc8dbacb00 ("ASoC: Intel: sst: Add some helper functions")
> Cc: stable@vger.kernel.org # 3.19.x
> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
> ---
>  sound/soc/intel/atom/sst/sst_pvt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/soc/intel/atom/sst/sst_pvt.c b/sound/soc/intel/atom/sst/sst_pvt.c
> index adb32fefd693..7c398b7c9d4b 100644
> --- a/sound/soc/intel/atom/sst/sst_pvt.c
> +++ b/sound/soc/intel/atom/sst/sst_pvt.c
> @@ -289,7 +289,7 @@ int sst_prepare_and_post_msg(struct intel_sst_drv *sst,
>  				ret = -ENOMEM;
>  				goto out;
>  			} else
> -				memcpy(data, (void *) block->data, block->size);
> +				memcpy(*data, (void *) block->data, block->size);
>  		}
>  	}
>  out:

Perhaps this would be nicer using kmemdup too
---
 sound/soc/intel/atom/sst/sst_pvt.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/sound/soc/intel/atom/sst/sst_pvt.c b/sound/soc/intel/atom/sst/sst_pvt.c
index adb32fe..b1e6b8f 100644
--- a/sound/soc/intel/atom/sst/sst_pvt.c
+++ b/sound/soc/intel/atom/sst/sst_pvt.c
@@ -279,17 +279,15 @@ int sst_prepare_and_post_msg(struct intel_sst_drv *sst,
 
 	if (response) {
 		ret = sst_wait_timeout(sst, block);
-		if (ret < 0) {
+		if (ret < 0)
 			goto out;
-		} else if(block->data) {
-			if (!data)
-				goto out;
-			*data = kzalloc(block->size, GFP_KERNEL);
-			if (!(*data)) {
+
+		if (data && block->data) {
+			*data = kmemdup(block->data, block->size, GFP_KERNEL);
+			if (!*data) {
 				ret = -ENOMEM;
 				goto out;
-			} else
-				memcpy(data, (void *) block->data, block->size);
+			}
 		}
 	}
 out:

[toc] | [prev] | [next] | [standalone]

#1471437 — Re: [PATCH 1/1] ASoC: Intel: Atom: add a missing star in a memcpy call

On 28/08/16 20:17, Joe Perches wrote:
> On Sun, 2016-08-28 at 19:39 +0200, Nicolas Iooss wrote:
>> In sst_prepare_and_post_msg(), when a response is received in "block",
>> the following code gets executed:
>>
>>     *data = kzalloc(block->size, GFP_KERNEL);
>>     memcpy(data, (void *) block->data, block->size);
>>
>> The memcpy() call overwrites the content of the *data pointer instead of
>> filling the newly-allocated memory (which pointer is hold by *data).
>> Fix this by using *data in the memcpy() call.
>>
>> Fixes: 60dc8dbacb00 ("ASoC: Intel: sst: Add some helper functions")
>> Cc: stable@vger.kernel.org # 3.19.x
>> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
>> ---
>>  sound/soc/intel/atom/sst/sst_pvt.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/intel/atom/sst/sst_pvt.c b/sound/soc/intel/atom/sst/sst_pvt.c
>> index adb32fefd693..7c398b7c9d4b 100644
>> --- a/sound/soc/intel/atom/sst/sst_pvt.c
>> +++ b/sound/soc/intel/atom/sst/sst_pvt.c
>> @@ -289,7 +289,7 @@ int sst_prepare_and_post_msg(struct intel_sst_drv *sst,
>>  				ret = -ENOMEM;
>>  				goto out;
>>  			} else
>> -				memcpy(data, (void *) block->data, block->size);
>> +				memcpy(*data, (void *) block->data, block->size);
>>  		}
>>  	}
>>  out:
> 
> Perhaps this would be nicer using kmemdup too

Thanks for your quick reply! I agree using kmemdup looks nicer here and
your patch looks good. I will send a v2 once I compile-tested it.

Nicolas

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web