diff mbox series

[V0,1/1] ASoC: msm: fix integer overflow for long duration offload playback

Message ID b906dbaf772d0152a3af52d639b090d15fe8c362.1646835508.git.quic_rbankapu@quicinc.com (mailing list archive)
State New, archived
Headers show
Series ASoC: msm: fix integer overflow for long duration compress offload playback | expand

Commit Message

Raghu Ballappa Bankapur March 9, 2022, 2:22 p.m. UTC 
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(-)

Comments

Amadeusz Sławiński March 9, 2022, 2:51 p.m. UTC | #1 
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;
Pierre-Louis Bossart March 9, 2022, 3:15 p.m. UTC | #2 
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.
kernel test robot March 9, 2022, 6:23 p.m. UTC | #3 
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
kernel test robot March 9, 2022, 10:17 p.m. UTC | #4 
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
diff mbox series

Patch

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;