[alsa-devel] [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:
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?
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=a... 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
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=a... 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
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), ...)
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
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.
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:
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
participants (3)
-
Joe Perches -
Julia Lawall -
Nicolas Iooss