[PATCH V0 0/1] ASoC: msm: fix integer overflow for long duration compress offload playback
32 bit variable is used for storing number of bytes copied to DSP, which can overflow when playback duration goes beyond 24 hours. Change data type for this variable to uint64_t to prevent overflow and related playback anomaly.
below are the steps used to reproduce Steps:- 1. play all clips from music app with Repeat All Songs enabled 2. one of clip is 1hr duration - 5.1_16bit_192khz_1hr.m4a 3. ACTUAL BEHAVIOUR:- there is no audio playback & progress bar was at end of stream, but time stamp continues to update till 31hrs for 1hr clip
Raghu Bankapur (1): ASoC: msm: fix integer overflow for long duration offload playback
include/uapi/sound/compress_offload.h | 2 +- sound/core/compress_offload.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
From: Raghu Bankapur quic_rbankapu@quiinc.com
32 bit variable is used for storing number of bytes copied to DSP, which can overflow when playback duration goes beyond 24 hours. Change data type for this variable to uint64_t to prevent overflow and related playback anomaly.
Signed-off-by: Raghu Bankapur quic_rbankapu@quicinc.com --- 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 9555f31c8425..57d24d89b2f4 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 de514ec8c83d..068376b586be 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -169,7 +169,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 3/9/2022 3:22 PM, Raghu Bankapur wrote:
From: Raghu Bankapur quic_rbankapu@quiinc.com
32 bit variable is used for storing number of bytes copied to DSP, which can overflow when playback duration goes beyond 24 hours. Change data type for this variable to uint64_t to prevent overflow and related playback anomaly.
Signed-off-by: Raghu Bankapur quic_rbankapu@quicinc.com
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 9555f31c8425..57d24d89b2f4 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;
I don't think this patch should be merged as is. It changes UAPI header, most likely breaking existing user space tools. Can't overflow be handled somehow instead?
Although having looked around, it makes a bit of sense, as snd_compr_update_tstamp() copies tstamp->copied_total to 64 bit variables...
Perhaps raise protocol version? ( https://elixir.bootlin.com/linux/latest/source/include/uapi/sound/compress_o... )
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index de514ec8c83d..068376b586be 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -169,7 +169,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 3/9/22 08:51, Amadeusz Sławiński wrote:
On 3/9/2022 3:22 PM, Raghu Bankapur wrote:
From: Raghu Bankapur quic_rbankapu@quiinc.com
32 bit variable is used for storing number of bytes copied to DSP, which can overflow when playback duration goes beyond 24 hours. Change data type for this variable to uint64_t to prevent overflow and related playback anomaly.
Signed-off-by: Raghu Bankapur quic_rbankapu@quicinc.com
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 9555f31c8425..57d24d89b2f4 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;
I don't think this patch should be merged as is. It changes UAPI header, most likely breaking existing user space tools. Can't overflow be handled somehow instead?
Although having looked around, it makes a bit of sense, as snd_compr_update_tstamp() copies tstamp->copied_total to 64 bit variables...
Perhaps raise protocol version? ( https://elixir.bootlin.com/linux/latest/source/include/uapi/sound/compress_o... )
This change sounded familiar, and sure enough we've already discussed this in 2019. I thought we did change things but unfortunately no, the problem is still there.
https://lore.kernel.org/alsa-devel/1560843846-4631-1-git-send-email-bgoswami...
we really want to change all the copied_total, pcm_frames and pcm_io_frames to u64 and add a new IOCTL.
Hi Raghu,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tiwai-sound/for-next] [also build test WARNING on vkoul-dmaengine/next broonie-sound/for-next v5.17-rc7 next-20220309] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Raghu-Bankapur/ASoC-msm-fix-integer... base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next config: x86_64-randconfig-a014 (https://download.01.org/0day-ci/archive/20220310/202203100248.RVGW6JZh-lkp@i...) compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 276ca87382b8f16a65bddac700202924228982f6) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/9020c5c2e38ba210a8d822d20e32bed85a4f... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Raghu-Bankapur/ASoC-msm-fix-integer-overflow-for-long-duration-compress-offload-playback/20220309-222520 git checkout 9020c5c2e38ba210a8d822d20e32bed85a4ffcab # save the config file to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash sound/soc/intel/atom/sst/
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
sound/soc/intel/atom/sst/sst_drv_interface.c:370:11: warning: format specifies type 'int' but the argument has type '__u64' (aka 'unsigned long long') [-Wformat]
str_id, tstamp->copied_total, tstamp->pcm_frames); ^~~~~~~~~~~~~~~~~~~~ include/linux/dev_printk.h:155:39: note: expanded from macro 'dev_dbg' dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ include/linux/dynamic_debug.h:167:19: note: expanded from macro 'dynamic_dev_dbg' dev, fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ include/linux/dynamic_debug.h:152:56: note: expanded from macro '_dynamic_func_call' __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__) ^~~~~~~~~~~ include/linux/dynamic_debug.h:134:15: note: expanded from macro '__dynamic_func_call' func(&id, ##__VA_ARGS__); \ ^~~~~~~~~~~ 1 warning generated.
vim +370 sound/soc/intel/atom/sst/sst_drv_interface.c
7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 343 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 344 static int sst_cdev_tstamp(struct device *dev, unsigned int str_id, 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 345 struct snd_compr_tstamp *tstamp) 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 346 { 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 347 struct snd_sst_tstamp fw_tstamp = {0,}; 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 348 struct stream_info *stream; 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 349 struct intel_sst_drv *ctx = dev_get_drvdata(dev); ce1cfe295abaa7 sound/soc/intel/atom/sst/sst_drv_interface.c Pierre-Louis Bossart 2018-07-24 350 void __iomem *addr; ce1cfe295abaa7 sound/soc/intel/atom/sst/sst_drv_interface.c Pierre-Louis Bossart 2018-07-24 351 ce1cfe295abaa7 sound/soc/intel/atom/sst/sst_drv_interface.c Pierre-Louis Bossart 2018-07-24 352 addr = (void __iomem *)(ctx->mailbox + ctx->tstamp) + ce1cfe295abaa7 sound/soc/intel/atom/sst/sst_drv_interface.c Pierre-Louis Bossart 2018-07-24 353 (str_id * sizeof(fw_tstamp)); 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 354 ce1cfe295abaa7 sound/soc/intel/atom/sst/sst_drv_interface.c Pierre-Louis Bossart 2018-07-24 355 memcpy_fromio(&fw_tstamp, addr, sizeof(fw_tstamp)); 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 356 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 357 stream = get_stream_info(ctx, str_id); 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 358 if (!stream) 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 359 return -EINVAL; 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 360 dev_dbg(dev, "rb_counter %llu in bytes\n", fw_tstamp.ring_buffer_counter); 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 361 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 362 tstamp->copied_total = fw_tstamp.ring_buffer_counter; 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 363 tstamp->pcm_frames = fw_tstamp.frames_decoded; 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 364 tstamp->pcm_io_frames = div_u64(fw_tstamp.hardware_counter, 75afbd052b3675 sound/soc/intel/atom/sst/sst_drv_interface.c Dan Carpenter 2015-04-09 365 (u64)stream->num_ch * SST_GET_BYTES_PER_SAMPLE(24)); 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 366 tstamp->sampling_rate = fw_tstamp.sampling_frequency; 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 367 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 368 dev_dbg(dev, "PCM = %u\n", tstamp->pcm_io_frames); 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 369 dev_dbg(dev, "Ptr Query on strid = %d copied_total %d, decodec %d\n", 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 @370 str_id, tstamp->copied_total, tstamp->pcm_frames); 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 371 dev_dbg(dev, "rendered %d\n", tstamp->pcm_io_frames); 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 372 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 373 return 0; 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 374 } 7adab122a57c5a sound/soc/intel/sst/sst_drv_interface.c Vinod Koul 2014-10-30 375
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Raghu,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tiwai-sound/for-next] [also build test ERROR on vkoul-dmaengine/next broonie-sound/for-next v5.17-rc7 next-20220309] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Raghu-Bankapur/ASoC-msm-fix-integer... base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next config: i386-randconfig-c001 (https://download.01.org/0day-ci/archive/20220310/202203100601.sczPjPL5-lkp@i...) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/9020c5c2e38ba210a8d822d20e32bed85a4f... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Raghu-Bankapur/ASoC-msm-fix-integer-overflow-for-long-duration-compress-offload-playback/20220309-222520 git checkout 9020c5c2e38ba210a8d822d20e32bed85a4ffcab # save the config file to linux build tree mkdir build_dir make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All error/warnings (new ones prefixed by >>, old ones prefixed by <<):
In file included from include/linux/printk.h:555, from include/linux/kernel.h:29, from arch/x86/include/asm/percpu.h:27, from arch/x86/include/asm/current.h:6, from include/linux/sched.h:12, from include/linux/delay.h:23, from sound/soc/intel/atom/sst/sst_drv_interface.c:13: sound/soc/intel/atom/sst/sst_drv_interface.c: In function 'sst_cdev_tstamp':
sound/soc/intel/atom/sst/sst_drv_interface.c:369:15: warning: format '%d' expects argument of type 'int', but argument 5 has type '__u64' {aka 'long long unsigned int'} [-Wformat=]
369 | dev_dbg(dev, "Ptr Query on strid = %d copied_total %d, decodec %dn", | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/dynamic_debug.h:134:15: note: in definition of macro '__dynamic_func_call' 134 | func(&id, ##__VA_ARGS__); | ^~~~~~~~~~~ include/linux/dynamic_debug.h:166:2: note: in expansion of macro '_dynamic_func_call' 166 | _dynamic_func_call(fmt,__dynamic_dev_dbg, | ^~~~~~~~~~~~~~~~~~ include/linux/dev_printk.h:155:2: note: in expansion of macro 'dynamic_dev_dbg' 155 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__) | ^~~~~~~~~~~~~~~ include/linux/dev_printk.h:155:23: note: in expansion of macro 'dev_fmt' 155 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__) | ^~~~~~~ sound/soc/intel/atom/sst/sst_drv_interface.c:369:2: note: in expansion of macro 'dev_dbg' 369 | dev_dbg(dev, "Ptr Query on strid = %d copied_total %d, decodec %dn", | ^~~~~~~ sound/soc/intel/atom/sst/sst_drv_interface.c:369:55: note: format string is defined here 369 | dev_dbg(dev, "Ptr Query on strid = %d copied_total %d, decodec %dn", | ~^ | | | int | %lld --
ERROR: modpost: "__umoddi3" [sound/soc/intel/atom/snd-soc-sst-atom-hifi2-platform.ko] undefined!
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
participants (4)
-
Amadeusz Sławiński -
kernel test robot -
Pierre-Louis Bossart -
Raghu Bankapur