* Re: [PATCH V0 1/1] ASoC: msm: fix integer overflow for long duration offload playback
2022-03-09 14:22 ` [PATCH V0 1/1] ASoC: msm: fix integer overflow for long duration " Raghu Bankapur
@ 2022-03-09 14:51 ` Amadeusz Sławiński
2022-03-09 15:15 ` Pierre-Louis Bossart
2022-03-09 18:23 ` kernel test robot
2022-03-09 22:17 ` kernel test robot
2 siblings, 1 reply; 6+ messages in thread
From: Amadeusz Sławiński @ 2022-03-09 14:51 UTC (permalink / raw)
To: Raghu Bankapur, Vinod Koul, Jaroslav Kysela, Takashi Iwai,
alsa-devel, linux-kernel
Cc: Krishna Jha, Raghu Bankapur
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_offload.h#L34
)
> 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;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH V0 1/1] ASoC: msm: fix integer overflow for long duration offload playback
2022-03-09 14:51 ` Amadeusz Sławiński
@ 2022-03-09 15:15 ` Pierre-Louis Bossart
0 siblings, 0 replies; 6+ messages in thread
From: Pierre-Louis Bossart @ 2022-03-09 15:15 UTC (permalink / raw)
To: Amadeusz Sławiński, Raghu Bankapur, Vinod Koul,
Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-kernel
Cc: Krishna Jha, Raghu Bankapur
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_offload.h#L34
> )
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@codeaurora.org/
we really want to change all the copied_total, pcm_frames and
pcm_io_frames to u64 and add a new IOCTL.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V0 1/1] ASoC: msm: fix integer overflow for long duration offload playback
2022-03-09 14:22 ` [PATCH V0 1/1] ASoC: msm: fix integer overflow for long duration " Raghu Bankapur
2022-03-09 14:51 ` Amadeusz Sławiński
@ 2022-03-09 18:23 ` kernel test robot
2022-03-09 22:17 ` kernel test robot
2 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2022-03-09 18:23 UTC (permalink / raw)
To: Raghu Bankapur, Vinod Koul, Jaroslav Kysela, Takashi Iwai,
alsa-devel, linux-kernel
Cc: llvm, kbuild-all, Krishna Jha
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-overflow-for-long-duration-compress-offload-playback/20220309-222520
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@intel.com/config)
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/9020c5c2e38ba210a8d822d20e32bed85a4ffcab
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH V0 1/1] ASoC: msm: fix integer overflow for long duration offload playback
2022-03-09 14:22 ` [PATCH V0 1/1] ASoC: msm: fix integer overflow for long duration " Raghu Bankapur
2022-03-09 14:51 ` Amadeusz Sławiński
2022-03-09 18:23 ` kernel test robot
@ 2022-03-09 22:17 ` kernel test robot
2 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2022-03-09 22:17 UTC (permalink / raw)
To: Raghu Bankapur, Vinod Koul, Jaroslav Kysela, Takashi Iwai,
alsa-devel, linux-kernel
Cc: kbuild-all, Krishna Jha
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-overflow-for-long-duration-compress-offload-playback/20220309-222520
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@intel.com/config)
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/9020c5c2e38ba210a8d822d20e32bed85a4ffcab
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
^ permalink raw reply [flat|nested] 6+ messages in thread