* [PATCH] ALSA: core: don't return uninitialized snd_compr_tstamp
@ 2013-02-11 13:44 Mark Brown
2013-02-11 14:05 ` Takashi Iwai
0 siblings, 1 reply; 6+ messages in thread
From: Mark Brown @ 2013-02-11 13:44 UTC (permalink / raw)
To: Jaroslav Kysela, Takashi Iwai
Cc: Vinod Koul, Mark Brown, Richard Fitzgerald, alsa-devel
From: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
The snd_compr_update_tstamp() can only fill in the snd_compr_tstamp
if the codec implements the pointer() function. If that happened
the code was previously returning uninitialized garbage in the
tstamp because it wasn't initialized anywhere.
This change zero-fills the tstamp in the two places it is used
before calling snd_compr_update_tstamp(), and also has
snd_compr_update_tstamp() return an error indication if it
can't provide a tstamp. For the case of snd_compr_calc_avail()
it ignores this error because we still need to return info on
the available buffer space even if we can't provide tstamp
info - when the tstamp is not valid all fields are now
guaranteed to be zero.
Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Acked-by: Vinod Koul <vinod.koul@intel.com>
---
sound/core/compress_offload.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index ad11dc9..2d62068 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -144,16 +144,17 @@ static int snd_compr_free(struct inode *inode, struct file *f)
return 0;
}
-static void snd_compr_update_tstamp(struct snd_compr_stream *stream,
+static int snd_compr_update_tstamp(struct snd_compr_stream *stream,
struct snd_compr_tstamp *tstamp)
{
if (!stream->ops->pointer)
- return;
+ return -ENOTSUPP;
stream->ops->pointer(stream, tstamp);
pr_debug("dsp consumed till %d total %d bytes\n",
tstamp->byte_offset, tstamp->copied_total);
stream->runtime->hw_pointer = tstamp->byte_offset;
stream->runtime->total_bytes_transferred = tstamp->copied_total;
+ return 0;
}
static size_t snd_compr_calc_avail(struct snd_compr_stream *stream,
@@ -161,7 +162,9 @@ static size_t snd_compr_calc_avail(struct snd_compr_stream *stream,
{
long avail_calc; /*this needs to be signed variable */
+ memset(avail, 0, sizeof(*avail));
snd_compr_update_tstamp(stream, &avail->tstamp);
+ /* Still need to return avail even if tstamp can't be filled in */
/* FIXME: This needs to be different for capture stream,
available is # of compressed data, for playback it's
@@ -517,11 +520,14 @@ out:
static inline int
snd_compr_tstamp(struct snd_compr_stream *stream, unsigned long arg)
{
- struct snd_compr_tstamp tstamp;
+ struct snd_compr_tstamp tstamp = {0};
+ int ret;
- snd_compr_update_tstamp(stream, &tstamp);
- return copy_to_user((struct snd_compr_tstamp __user *)arg,
- &tstamp, sizeof(tstamp)) ? -EFAULT : 0;
+ ret = snd_compr_update_tstamp(stream, &tstamp);
+ if (ret == 0)
+ ret = copy_to_user((struct snd_compr_tstamp __user *)arg,
+ &tstamp, sizeof(tstamp)) ? -EFAULT : 0;
+ return ret;
}
static int snd_compr_pause(struct snd_compr_stream *stream)
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] ALSA: core: don't return uninitialized snd_compr_tstamp
2013-02-11 13:44 [PATCH] ALSA: core: don't return uninitialized snd_compr_tstamp Mark Brown
@ 2013-02-11 14:05 ` Takashi Iwai
0 siblings, 0 replies; 6+ messages in thread
From: Takashi Iwai @ 2013-02-11 14:05 UTC (permalink / raw)
To: Mark Brown; +Cc: Vinod Koul, Richard Fitzgerald, alsa-devel
At Mon, 11 Feb 2013 13:44:53 +0000,
Mark Brown wrote:
>
> From: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
>
> The snd_compr_update_tstamp() can only fill in the snd_compr_tstamp
> if the codec implements the pointer() function. If that happened
> the code was previously returning uninitialized garbage in the
> tstamp because it wasn't initialized anywhere.
>
> This change zero-fills the tstamp in the two places it is used
> before calling snd_compr_update_tstamp(), and also has
> snd_compr_update_tstamp() return an error indication if it
> can't provide a tstamp. For the case of snd_compr_calc_avail()
> it ignores this error because we still need to return info on
> the available buffer space even if we can't provide tstamp
> info - when the tstamp is not valid all fields are now
> guaranteed to be zero.
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Acked-by: Vinod Koul <vinod.koul@intel.com>
Thanks, applied.
Takashi
> ---
> sound/core/compress_offload.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index ad11dc9..2d62068 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -144,16 +144,17 @@ static int snd_compr_free(struct inode *inode, struct file *f)
> return 0;
> }
>
> -static void snd_compr_update_tstamp(struct snd_compr_stream *stream,
> +static int snd_compr_update_tstamp(struct snd_compr_stream *stream,
> struct snd_compr_tstamp *tstamp)
> {
> if (!stream->ops->pointer)
> - return;
> + return -ENOTSUPP;
> stream->ops->pointer(stream, tstamp);
> pr_debug("dsp consumed till %d total %d bytes\n",
> tstamp->byte_offset, tstamp->copied_total);
> stream->runtime->hw_pointer = tstamp->byte_offset;
> stream->runtime->total_bytes_transferred = tstamp->copied_total;
> + return 0;
> }
>
> static size_t snd_compr_calc_avail(struct snd_compr_stream *stream,
> @@ -161,7 +162,9 @@ static size_t snd_compr_calc_avail(struct snd_compr_stream *stream,
> {
> long avail_calc; /*this needs to be signed variable */
>
> + memset(avail, 0, sizeof(*avail));
> snd_compr_update_tstamp(stream, &avail->tstamp);
> + /* Still need to return avail even if tstamp can't be filled in */
>
> /* FIXME: This needs to be different for capture stream,
> available is # of compressed data, for playback it's
> @@ -517,11 +520,14 @@ out:
> static inline int
> snd_compr_tstamp(struct snd_compr_stream *stream, unsigned long arg)
> {
> - struct snd_compr_tstamp tstamp;
> + struct snd_compr_tstamp tstamp = {0};
> + int ret;
>
> - snd_compr_update_tstamp(stream, &tstamp);
> - return copy_to_user((struct snd_compr_tstamp __user *)arg,
> - &tstamp, sizeof(tstamp)) ? -EFAULT : 0;
> + ret = snd_compr_update_tstamp(stream, &tstamp);
> + if (ret == 0)
> + ret = copy_to_user((struct snd_compr_tstamp __user *)arg,
> + &tstamp, sizeof(tstamp)) ? -EFAULT : 0;
> + return ret;
> }
>
> static int snd_compr_pause(struct snd_compr_stream *stream)
> --
> 1.7.10.4
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ALSA: core: don't return uninitialized snd_compr_tstamp
@ 2013-01-31 2:37 Mark Brown
2013-01-31 2:53 ` Vinod Koul
2013-02-04 14:23 ` Pierre-Louis Bossart
0 siblings, 2 replies; 6+ messages in thread
From: Mark Brown @ 2013-01-31 2:37 UTC (permalink / raw)
To: Vinod Koul, Takashi Iwai, Jaroslav Kysela
Cc: alsa-devel, Mark Brown, Richard Fitzgerald
From: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
The snd_compr_update_tstamp() can only fill in the snd_compr_tstamp
if the codec implements the pointer() function. If that happened
the code was previously returning uninitialized garbage in the
tstamp because it wasn't initialized anywhere.
This change zero-fills the tstamp in the two places it is used
before calling snd_compr_update_tstamp(), and also has
snd_compr_update_tstamp() return an error indication if it
can't provide a tstamp. For the case of snd_compr_calc_avail()
it ignores this error because we still need to return info on
the available buffer space even if we can't provide tstamp
info - when the tstamp is not valid all fields are now
guaranteed to be zero.
Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
sound/core/compress_offload.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index ad11dc9..2d62068 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -144,16 +144,17 @@ static int snd_compr_free(struct inode *inode, struct file *f)
return 0;
}
-static void snd_compr_update_tstamp(struct snd_compr_stream *stream,
+static int snd_compr_update_tstamp(struct snd_compr_stream *stream,
struct snd_compr_tstamp *tstamp)
{
if (!stream->ops->pointer)
- return;
+ return -ENOTSUPP;
stream->ops->pointer(stream, tstamp);
pr_debug("dsp consumed till %d total %d bytes\n",
tstamp->byte_offset, tstamp->copied_total);
stream->runtime->hw_pointer = tstamp->byte_offset;
stream->runtime->total_bytes_transferred = tstamp->copied_total;
+ return 0;
}
static size_t snd_compr_calc_avail(struct snd_compr_stream *stream,
@@ -161,7 +162,9 @@ static size_t snd_compr_calc_avail(struct snd_compr_stream *stream,
{
long avail_calc; /*this needs to be signed variable */
+ memset(avail, 0, sizeof(*avail));
snd_compr_update_tstamp(stream, &avail->tstamp);
+ /* Still need to return avail even if tstamp can't be filled in */
/* FIXME: This needs to be different for capture stream,
available is # of compressed data, for playback it's
@@ -517,11 +520,14 @@ out:
static inline int
snd_compr_tstamp(struct snd_compr_stream *stream, unsigned long arg)
{
- struct snd_compr_tstamp tstamp;
+ struct snd_compr_tstamp tstamp = {0};
+ int ret;
- snd_compr_update_tstamp(stream, &tstamp);
- return copy_to_user((struct snd_compr_tstamp __user *)arg,
- &tstamp, sizeof(tstamp)) ? -EFAULT : 0;
+ ret = snd_compr_update_tstamp(stream, &tstamp);
+ if (ret == 0)
+ ret = copy_to_user((struct snd_compr_tstamp __user *)arg,
+ &tstamp, sizeof(tstamp)) ? -EFAULT : 0;
+ return ret;
}
static int snd_compr_pause(struct snd_compr_stream *stream)
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] ALSA: core: don't return uninitialized snd_compr_tstamp
2013-01-31 2:37 Mark Brown
@ 2013-01-31 2:53 ` Vinod Koul
2013-02-04 14:23 ` Pierre-Louis Bossart
1 sibling, 0 replies; 6+ messages in thread
From: Vinod Koul @ 2013-01-31 2:53 UTC (permalink / raw)
To: Mark Brown; +Cc: Takashi Iwai, alsa-devel, Richard Fitzgerald, Vinod Koul
On Thu, Jan 31, 2013 at 10:37:12AM +0800, Mark Brown wrote:
> From: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
>
> The snd_compr_update_tstamp() can only fill in the snd_compr_tstamp
> if the codec implements the pointer() function. If that happened
> the code was previously returning uninitialized garbage in the
> tstamp because it wasn't initialized anywhere.
>
> This change zero-fills the tstamp in the two places it is used
> before calling snd_compr_update_tstamp(), and also has
> snd_compr_update_tstamp() return an error indication if it
> can't provide a tstamp. For the case of snd_compr_calc_avail()
> it ignores this error because we still need to return info on
> the available buffer space even if we can't provide tstamp
> info - when the tstamp is not valid all fields are now
> guaranteed to be zero.
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Acked-by: Vinod Koul <vinod.koul@intel.com>
--
~Vinod
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: core: don't return uninitialized snd_compr_tstamp
2013-01-31 2:37 Mark Brown
2013-01-31 2:53 ` Vinod Koul
@ 2013-02-04 14:23 ` Pierre-Louis Bossart
2013-02-04 14:43 ` Takashi Iwai
1 sibling, 1 reply; 6+ messages in thread
From: Pierre-Louis Bossart @ 2013-02-04 14:23 UTC (permalink / raw)
To: Mark Brown; +Cc: Takashi Iwai, Vinod Koul, Richard Fitzgerald, alsa-devel
> The snd_compr_update_tstamp() can only fill in the snd_compr_tstamp
> if the codec implements the pointer() function. If that happened
> the code was previously returning uninitialized garbage in the
> tstamp because it wasn't initialized anywhere.
>
> This change zero-fills the tstamp in the two places it is used
> before calling snd_compr_update_tstamp(), and also has
> snd_compr_update_tstamp() return an error indication if it
> can't provide a tstamp. For the case of snd_compr_calc_avail()
> it ignores this error because we still need to return info on
> the available buffer space even if we can't provide tstamp
> info - when the tstamp is not valid all fields are now
> guaranteed to be zero.
I think I am missing something here. For compressed data, the DSP really
needs to return a timestamp or the application will not be able to track
the audio time, it'll only be able to refill buffers. Since with
variable-rate compression we cannot translate bytes to time, isn't it a
requirement that timestamps be supported for this API? If you don't
support the .pointer, how will you provide this information to
user-space levels?
Thanks,
-Pierre
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: core: don't return uninitialized snd_compr_tstamp
2013-02-04 14:23 ` Pierre-Louis Bossart
@ 2013-02-04 14:43 ` Takashi Iwai
0 siblings, 0 replies; 6+ messages in thread
From: Takashi Iwai @ 2013-02-04 14:43 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: Vinod Koul, Mark Brown, Richard Fitzgerald, alsa-devel
At Mon, 04 Feb 2013 08:23:14 -0600,
Pierre-Louis Bossart wrote:
>
>
> > The snd_compr_update_tstamp() can only fill in the snd_compr_tstamp
> > if the codec implements the pointer() function. If that happened
> > the code was previously returning uninitialized garbage in the
> > tstamp because it wasn't initialized anywhere.
> >
> > This change zero-fills the tstamp in the two places it is used
> > before calling snd_compr_update_tstamp(), and also has
> > snd_compr_update_tstamp() return an error indication if it
> > can't provide a tstamp. For the case of snd_compr_calc_avail()
> > it ignores this error because we still need to return info on
> > the available buffer space even if we can't provide tstamp
> > info - when the tstamp is not valid all fields are now
> > guaranteed to be zero.
>
> I think I am missing something here. For compressed data, the DSP really
> needs to return a timestamp or the application will not be able to track
> the audio time, it'll only be able to refill buffers. Since with
> variable-rate compression we cannot translate bytes to time, isn't it a
> requirement that timestamps be supported for this API? If you don't
> support the .pointer, how will you provide this information to
> user-space levels?
It's hypothetically allowed to have a driver without pointer ops, so
we need to cover the leak of uninitialized kernel space anyway.
Whether the driver without pointer ops is practically useful or not
is another question...
Takashi
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-02-11 14:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-02-11 13:44 [PATCH] ALSA: core: don't return uninitialized snd_compr_tstamp Mark Brown
2013-02-11 14:05 ` Takashi Iwai
-- strict thread matches above, loose matches on Subject: below --
2013-01-31 2:37 Mark Brown
2013-01-31 2:53 ` Vinod Koul
2013-02-04 14:23 ` Pierre-Louis Bossart
2013-02-04 14:43 ` Takashi Iwai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).