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

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

[Nicolas Iooss]

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

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

[Joe Perches]

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?

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

[Nicolas Iooss]

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

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

[Julia Lawall]

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

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

[Joe Perches]

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), ...)

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

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1268 bytes --]



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

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

[Joe Perches]

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.

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

[Joe Perches]

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:

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

[Nicolas Iooss]

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