* [patch] ALSA: compress: info leak in snd_compr_get_caps() @ 2013-04-21 11:07 Dan Carpenter 2013-04-21 11:40 ` walter harms 2013-04-22 8:42 ` Takashi Iwai 0 siblings, 2 replies; 5+ 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] 5+ messages in thread
* Re: [patch] ALSA: compress: info leak in snd_compr_get_caps() 2013-04-21 11:07 [patch] ALSA: compress: info leak in snd_compr_get_caps() Dan Carpenter @ 2013-04-21 11:40 ` walter harms 2013-04-21 16:39 ` Dan Carpenter 2013-04-22 8:42 ` Takashi Iwai 1 sibling, 1 reply; 5+ 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] 5+ 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 0 siblings, 0 replies; 5+ 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] 5+ messages in thread
* Re: [patch] ALSA: compress: info leak in snd_compr_get_caps() 2013-04-21 11:07 [patch] ALSA: compress: info leak in snd_compr_get_caps() Dan Carpenter 2013-04-21 11:40 ` walter harms @ 2013-04-22 8:42 ` Takashi Iwai 2013-04-22 8:50 ` Vinod Koul 1 sibling, 1 reply; 5+ 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] 5+ 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 0 siblings, 0 replies; 5+ 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] 5+ messages in thread
end of thread, other threads:[~2013-04-22 8:50 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-04-21 11:07 [patch] ALSA: compress: info leak in snd_compr_get_caps() Dan Carpenter 2013-04-21 11:40 ` walter harms 2013-04-21 16:39 ` Dan Carpenter 2013-04-22 8:42 ` Takashi Iwai 2013-04-22 8:50 ` Vinod Koul
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox