Skip to contentSkip to conversation

ALSA: compress: info leak in snd_compr_get_caps()

PATCH Applied Dan Carpenter 4 participants 5 messages Apr 21, 2013 – Apr 22, 2013

Subsystems (3)
Sound Kernel janitors Sound - compressed audio
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 ALSA: compress: info leak in snd_compr_get_caps()

If the ->get_caps() function doesn't clear the buffer then there would stack information leaked to userspace. For example, soc_compr_get_caps() can return success without clearing the buffer.

Signed-off-by Dan Carpenter <[email protected]>

---
Perhaps the soc_compr_get_caps() function should return an error code if the platform->driver->compr_ops is NULL. I'm not sure about that, and it's a separate issue anyway.

sound/core/compress_offload.c +1 -0
index c84abc8..8d3190a 100644
@@ -375,6 +375,7 @@ snd_compr_get_caps(struct snd_compr_stream *stream, unsigned long arg)
 	if (!stream->ops->get_caps)
 		return -ENXIO;
 
+	memset(&caps, 0, sizeof(caps));
 	retval = stream->ops->get_caps(stream, &caps);
 	if (retval)
 		goto out;
#1 lore raw

Am 21.04.2013 13:07, schrieb Dan Carpenter:

Quoted 15 lines

If the ->get_caps() function doesn't clear the buffer then there would stack information leaked to userspace. For example, soc_compr_get_caps() can return success without clearing the buffer.

Signed-off-by Dan Carpenter <[email protected]>

---
Perhaps the soc_compr_get_caps() function should return an error code if the platform->driver->compr_ops is NULL. I'm not sure about that, and it's a separate issue anyway.

sound/core/compress_offload.c +0 -0
index c84abc8..8d3190a 100644

sound/core/compress_offload.c snd_compr_get_caps(struct snd_compr_stream *stream, unsigned long arg)

Quoted 8 lines
sound/core/compress_offload.c +1 -1
@@ -375,6 +375,7 @@ snd_compr_get_caps(struct snd_compr_stream *stream, unsigned long arg)
 	if (!stream->ops->get_caps)
 		return -ENXIO;
 
+	memset(&caps, 0, sizeof(caps));
 	retval = stream->ops->get_caps(stream, &caps);
 	if (retval)
 		goto out;
--

IMHO this should be done in get_caps() as it will manipulate the entries. or is there a special reason to have it here ?

re,
wh

author
#2 lore raw
Quoted 21 lines · walter harms

Am 21.04.2013 13:07, schrieb Dan Carpenter:

Quoted 18 lines

If the ->get_caps() function doesn't clear the buffer then there would stack information leaked to userspace. For example, soc_compr_get_caps() can return success without clearing the buffer.

Signed-off-by Dan Carpenter <[email protected]>

---
Perhaps the soc_compr_get_caps() function should return an error code if the platform->driver->compr_ops is NULL. I'm not sure about that, and it's a separate issue anyway.

sound/core/compress_offload.c +0 -0
index c84abc8..8d3190a 100644
@@ -375,6 +375,7 @@ snd_compr_get_caps(struct snd_compr_stream *stream, unsigned long arg)
 	if (!stream->ops->get_caps)
 		return -ENXIO;
Quoted 8 lines

sound/core/compress_offload.c snd_compr_get_caps(struct snd_compr_stream *stream, unsigned long arg)

Quoted 5 lines
sound/core/compress_offload.c +1 -1
+	memset(&caps, 0, sizeof(caps));
 	retval = stream->ops->get_caps(stream, &caps);
 	if (retval)
 		goto out;
--

IMHO this should be done in get_caps() as it will manipulate the entries. or is there a special reason to have it here ?

There isn't a get_caps() function. It's a pointer to many functions. It's safer to do it here where it can be audited by static analisys. It's cleaner to do it once instead of many times.

regards,
dan carpenter

Applied
#3 lore raw
In reply to Dan Carpenter

At Sun, 21 Apr 2013 14:07:29 +0300,

Quoted 4 lines · Dan Carpenter

If the ->get_caps() function doesn't clear the buffer then there would stack information leaked to userspace. For example, soc_compr_get_caps() can return success without clearing the buffer.

Thanks, applied now.

I found that we have also a similar problem in other ioctls (snd_compr_get_codec_caps() and snd_compr_get_params()), so applied the following patch in addition.

Takashi

---
From: Takashi Iwai <[email protected]>
Subject: [PATCH] ALSA: compress: Use kzalloc() for ioctls writing back data

Like the previous patch by Dan, we should clear the data to be returned from certain compress ioctls, namely, snd_compr_get_codec_caps() and snd_compr_get_params(). This time, we can simply replace kmalloc() with kzalloc().

Signed-off-by Takashi Iwai <[email protected]>
---
 sound/core/compress_offload.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
sound/core/compress_offload.c +2 -2
index 664c693..a0bc47f 100644
@@ -428,7 +428,7 @@ snd_compr_get_codec_caps(struct snd_compr_stream *stream, unsigned long arg)
 	if (!stream->ops->get_codec_caps)
 		return -ENXIO;
 
-	caps = kmalloc(sizeof(*caps), GFP_KERNEL);
+	caps = kzalloc(sizeof(*caps), GFP_KERNEL);
 	if (!caps)
 		return -ENOMEM;
 

@@ -545,7 +545,7 @@ snd_compr_get_params(struct snd_compr_stream *stream, unsigned long arg)
 	if (!stream->ops->get_params)
 		return -EBADFD;
 
-	params = kmalloc(sizeof(*params), GFP_KERNEL);
+	params = kzalloc(sizeof(*params), GFP_KERNEL);
 	if (!params)
 		return -ENOMEM;
 	retval = stream->ops->get_params(stream, params);
Signature · 1 line
1.8.2.1
#4 lore raw
Quoted 20 lines · Takashi Iwai

At Sun, 21 Apr 2013 14:07:29 +0300,

Quoted 4 lines · Dan Carpenter

If the ->get_caps() function doesn't clear the buffer then there would stack information leaked to userspace. For example, soc_compr_get_caps() can return success without clearing the buffer.

Thanks, applied now.

I found that we have also a similar problem in other ioctls (snd_compr_get_codec_caps() and snd_compr_get_params()), so applied the following patch in addition.

Takashi

---
From: Takashi Iwai <[email protected]>
Subject: [PATCH] ALSA: compress: Use kzalloc() for ioctls writing back data

Quoted 6 lines

Like the previous patch by Dan, we should clear the data to be returned from certain compress ioctls, namely, snd_compr_get_codec_caps() and snd_compr_get_params(). This time, we can simply replace kmalloc() with kzalloc().

Signed-off-by Takashi Iwai <[email protected]>

If not too late..
Both Acked-by: Vinod Koul <[email protected]>

Quoted 29 lines
---
 sound/core/compress_offload.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
sound/core/compress_offload.c +2 -2
index 664c693..a0bc47f 100644
@@ -428,7 +428,7 @@ snd_compr_get_codec_caps(struct snd_compr_stream *stream, unsigned long arg)
 	if (!stream->ops->get_codec_caps)
 		return -ENXIO;
 
-	caps = kmalloc(sizeof(*caps), GFP_KERNEL);
+	caps = kzalloc(sizeof(*caps), GFP_KERNEL);
 	if (!caps)
 		return -ENOMEM;
 

@@ -545,7 +545,7 @@ snd_compr_get_params(struct snd_compr_stream *stream, unsigned long arg)
 	if (!stream->ops->get_params)
 		return -EBADFD;
 
-	params = kmalloc(sizeof(*params), GFP_KERNEL);
+	params = kzalloc(sizeof(*params), GFP_KERNEL);
 	if (!params)
 		return -ENOMEM;
 	retval = stream->ops->get_params(stream, params);
Signature · 1 line
1.8.2.1