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: <37933410-1bc7-3e27-50e8-3da03e8b5603@linux.intel.com>
Date:   Wed, 9 Mar 2022 15:51:10 +0100
From:   Amadeusz Sławiński 
        <amadeuszx.slawinski@...ux.intel.com>
To:     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/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 
)

> 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;