[alsa-devel] [PATCH] ALSA: compress: avoid integer overflow for long duration offload playback
From: Banajit Goswami bgoswami@codeaurora.org
Currently a 32 bit variable is used for storing number of bytes copied to DSP, which can overflow when playback continues for a long duration. Change data type for this variable to __u64 to prevent overflow.
Signed-off-by: Dhananjay Kumar dhakumar@codeaurora.org Signed-off-by: Banajit Goswami bgoswami@codeaurora.org --- include/uapi/sound/compress_offload.h | 2 +- sound/core/compress_offload.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h index 56d9567..db5edf3 100644 --- a/include/uapi/sound/compress_offload.h +++ b/include/uapi/sound/compress_offload.h @@ -67,7 +67,7 @@ struct snd_compr_params { */ struct snd_compr_tstamp { __u32 byte_offset; - __u32 copied_total; + __u64 copied_total; __u32 pcm_frames; __u32 pcm_io_frames; __u32 sampling_rate; diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index a1a6fd7..2d0a709 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -184,7 +184,7 @@ static int snd_compr_update_tstamp(struct snd_compr_stream *stream, if (!stream->ops->pointer) return -ENOTSUPP; stream->ops->pointer(stream, tstamp); - pr_debug("dsp consumed till %d total %d bytes\n", + pr_debug("dsp consumed till %d total %llu bytes\n", tstamp->byte_offset, tstamp->copied_total); if (stream->direction == SND_COMPRESS_PLAYBACK) stream->runtime->total_bytes_transferred = tstamp->copied_total;
On 6/18/19 9:44 AM, bgoswami@codeaurora.org wrote:
From: Banajit Goswami bgoswami@codeaurora.org
Currently a 32 bit variable is used for storing number of bytes copied to DSP, which can overflow when playback continues for a long duration. Change data type for this variable to __u64 to prevent overflow.
This clearly looks like a bug, the number of bytes transferred is stored as u64 in the runtime. I have no memories of this being intentional.
That said, it seems odd to me that you have an overflow on the number of bytes but not on the number of PCM frames. Shouldn't both the pcm_frames and pcm_io_frames fields also be changed to u64 while we are at it?
And the second issue is that this may impact apps. This is a ABI change, isn't it?
Signed-off-by: Dhananjay Kumar dhakumar@codeaurora.org Signed-off-by: Banajit Goswami bgoswami@codeaurora.org
include/uapi/sound/compress_offload.h | 2 +- sound/core/compress_offload.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h index 56d9567..db5edf3 100644 --- a/include/uapi/sound/compress_offload.h +++ b/include/uapi/sound/compress_offload.h @@ -67,7 +67,7 @@ struct snd_compr_params { */ struct snd_compr_tstamp { __u32 byte_offset;
- __u32 copied_total;
- __u64 copied_total; __u32 pcm_frames; __u32 pcm_io_frames; __u32 sampling_rate;
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index a1a6fd7..2d0a709 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -184,7 +184,7 @@ static int snd_compr_update_tstamp(struct snd_compr_stream *stream, if (!stream->ops->pointer) return -ENOTSUPP; stream->ops->pointer(stream, tstamp);
- pr_debug("dsp consumed till %d total %d bytes\n",
- pr_debug("dsp consumed till %d total %llu bytes\n", tstamp->byte_offset, tstamp->copied_total); if (stream->direction == SND_COMPRESS_PLAYBACK) stream->runtime->total_bytes_transferred = tstamp->copied_total;
On Tue, 18 Jun 2019 10:02:59 +0200, Pierre-Louis Bossart wrote:
On 6/18/19 9:44 AM, bgoswami@codeaurora.org wrote:
From: Banajit Goswami bgoswami@codeaurora.org
Currently a 32 bit variable is used for storing number of bytes copied to DSP, which can overflow when playback continues for a long duration. Change data type for this variable to __u64 to prevent overflow.
This clearly looks like a bug, the number of bytes transferred is stored as u64 in the runtime. I have no memories of this being intentional.
That said, it seems odd to me that you have an overflow on the number of bytes but not on the number of PCM frames. Shouldn't both the pcm_frames and pcm_io_frames fields also be changed to u64 while we are at it?
And the second issue is that this may impact apps. This is a ABI change, isn't it?
Yes, it breaks ABI, and we can't take this as is.
If we need 64bit values, a new ioctl must be provided while keeping the old one still working.
thanks,
Takashi
Signed-off-by: Dhananjay Kumar dhakumar@codeaurora.org Signed-off-by: Banajit Goswami bgoswami@codeaurora.org
include/uapi/sound/compress_offload.h | 2 +- sound/core/compress_offload.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h index 56d9567..db5edf3 100644 --- a/include/uapi/sound/compress_offload.h +++ b/include/uapi/sound/compress_offload.h @@ -67,7 +67,7 @@ struct snd_compr_params { */ struct snd_compr_tstamp { __u32 byte_offset;
- __u32 copied_total;
- __u64 copied_total; __u32 pcm_frames; __u32 pcm_io_frames; __u32 sampling_rate;
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index a1a6fd7..2d0a709 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -184,7 +184,7 @@ static int snd_compr_update_tstamp(struct snd_compr_stream *stream, if (!stream->ops->pointer) return -ENOTSUPP; stream->ops->pointer(stream, tstamp);
- pr_debug("dsp consumed till %d total %d bytes\n",
- pr_debug("dsp consumed till %d total %llu bytes\n", tstamp->byte_offset, tstamp->copied_total); if (stream->direction == SND_COMPRESS_PLAYBACK) stream->runtime->total_bytes_transferred = tstamp->copied_total;
Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
participants (3)
-
bgoswamiï¼ codeaurora.org -
Pierre-Louis Bossart -
Takashi Iwai