linux-kernel - Re: [PATCH V0 1/1] ASoC: msm: fix integer overflow for long duration offload playback

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2e6b7905-4b9f-8615-a7f2-6b3d66e6386e@linux.intel.com>
Date:   Wed, 9 Mar 2022 09:15:31 -0600
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Amadeusz Sławiński 
        <amadeuszx.slawinski@...ux.intel.com>,
        Raghu Bankapur <quic_rbankapu@...cinc.com>,
        Vinod Koul <vkoul@...nel.org>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, alsa-devel@...a-project.org,
        linux-kernel@...r.kernel.org
Cc:     Krishna Jha <quic_kkishorj@...cinc.com>,
        Raghu Bankapur <quic_rbankapu@...inc.com>
Subject: Re: [PATCH V0 1/1] ASoC: msm: fix integer overflow for long duration
 offload playback



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@...inc.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@...cinc.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.