* [patch] ALSA: compress: info leak in snd_compr_get_caps()
@ 2013-04-21 11:07 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2013-04-21 11:07 UTC (permalink / raw)
To: Jaroslav Kysela
Cc: alsa-devel, Takashi Iwai, kernel-janitors, Vinod Koul, Jeeja KP,
Namarta Kohli
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 <dan.carpenter@oracle.com>
---
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.
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index c84abc8..8d3190a 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -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;
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [patch] ALSA: compress: info leak in snd_compr_get_caps()
2013-04-21 11:07 ` Dan Carpenter
@ 2013-04-21 11:40 ` walter harms
-1 siblings, 0 replies; 10+ messages in thread
From: walter harms @ 2013-04-21 11:40 UTC (permalink / raw)
To: Dan Carpenter
Cc: alsa-devel, Takashi Iwai, kernel-janitors, Vinod Koul, Jeeja KP,
Namarta Kohli
Am 21.04.2013 13:07, schrieb 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.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> 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.
>
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index c84abc8..8d3190a 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] ALSA: compress: info leak in snd_compr_get_caps()
@ 2013-04-21 11:40 ` walter harms
0 siblings, 0 replies; 10+ messages in thread
From: walter harms @ 2013-04-21 11:40 UTC (permalink / raw)
To: Dan Carpenter
Cc: alsa-devel, Takashi Iwai, kernel-janitors, Vinod Koul, Jeeja KP,
Namarta Kohli
Am 21.04.2013 13:07, schrieb 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.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> 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.
>
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index c84abc8..8d3190a 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] ALSA: compress: info leak in snd_compr_get_caps()
2013-04-21 11:40 ` walter harms
@ 2013-04-21 16:39 ` Dan Carpenter
-1 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2013-04-21 16:39 UTC (permalink / raw)
To: walter harms
Cc: alsa-devel, Takashi Iwai, kernel-janitors, Vinod Koul, Jeeja KP,
Namarta Kohli
On Sun, Apr 21, 2013 at 01:40:25PM +0200, walter harms wrote:
>
>
> Am 21.04.2013 13:07, schrieb 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.
> >
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > 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.
> >
> > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> > index c84abc8..8d3190a 100644
> > --- a/sound/core/compress_offload.c
> > +++ b/sound/core/compress_offload.c
> > @@ -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 ?
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] ALSA: compress: info leak in snd_compr_get_caps()
@ 2013-04-21 16:39 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2013-04-21 16:39 UTC (permalink / raw)
To: walter harms
Cc: alsa-devel, Takashi Iwai, kernel-janitors, Vinod Koul, Jeeja KP,
Namarta Kohli
On Sun, Apr 21, 2013 at 01:40:25PM +0200, walter harms wrote:
>
>
> Am 21.04.2013 13:07, schrieb 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.
> >
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > 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.
> >
> > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> > index c84abc8..8d3190a 100644
> > --- a/sound/core/compress_offload.c
> > +++ b/sound/core/compress_offload.c
> > @@ -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 ?
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] ALSA: compress: info leak in snd_compr_get_caps()
2013-04-21 11:07 ` Dan Carpenter
@ 2013-04-22 8:42 ` Takashi Iwai
-1 siblings, 0 replies; 10+ messages in thread
From: Takashi Iwai @ 2013-04-22 8:42 UTC (permalink / raw)
To: Dan Carpenter
Cc: alsa-devel, Vinod Koul, kernel-janitors, Jeeja KP, Namarta Kohli
At Sun, 21 Apr 2013 14:07:29 +0300,
Dan Carpenter wrote:
>
> 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 <tiwai@suse.de>
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 <tiwai@suse.de>
---
sound/core/compress_offload.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 664c693..a0bc47f 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -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);
--
1.8.2.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [patch] ALSA: compress: info leak in snd_compr_get_caps()
@ 2013-04-22 8:42 ` Takashi Iwai
0 siblings, 0 replies; 10+ messages in thread
From: Takashi Iwai @ 2013-04-22 8:42 UTC (permalink / raw)
To: Dan Carpenter
Cc: alsa-devel, Vinod Koul, kernel-janitors, Jeeja KP, Namarta Kohli
At Sun, 21 Apr 2013 14:07:29 +0300,
Dan Carpenter wrote:
>
> 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 <tiwai@suse.de>
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 <tiwai@suse.de>
---
sound/core/compress_offload.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 664c693..a0bc47f 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -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);
--
1.8.2.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [patch] ALSA: compress: info leak in snd_compr_get_caps()
2013-04-22 8:42 ` Takashi Iwai
@ 2013-04-22 8:50 ` Vinod Koul
-1 siblings, 0 replies; 10+ messages in thread
From: Vinod Koul @ 2013-04-22 8:38 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, kernel-janitors, Jeeja KP, Namarta Kohli,
Dan Carpenter
On Mon, Apr 22, 2013 at 10:42:25AM +0200, Takashi Iwai wrote:
> At Sun, 21 Apr 2013 14:07:29 +0300,
> Dan Carpenter wrote:
> >
> > 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 <tiwai@suse.de>
> 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 <tiwai@suse.de>
If not too late..
Both Acked-by: Vinod Koul <vinod.koul@intel.com>
> ---
> sound/core/compress_offload.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index 664c693..a0bc47f 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -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);
> --
> 1.8.2.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] ALSA: compress: info leak in snd_compr_get_caps()
@ 2013-04-22 8:50 ` Vinod Koul
0 siblings, 0 replies; 10+ messages in thread
From: Vinod Koul @ 2013-04-22 8:50 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, kernel-janitors, Jeeja KP, Namarta Kohli,
Dan Carpenter
On Mon, Apr 22, 2013 at 10:42:25AM +0200, Takashi Iwai wrote:
> At Sun, 21 Apr 2013 14:07:29 +0300,
> Dan Carpenter wrote:
> >
> > 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 <tiwai@suse.de>
> 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 <tiwai@suse.de>
If not too late..
Both Acked-by: Vinod Koul <vinod.koul@intel.com>
> ---
> sound/core/compress_offload.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index 664c693..a0bc47f 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -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);
> --
> 1.8.2.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread