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

[Pierre-Louis Bossart]

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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxxx/

we really want to change all the copied_total, pcm_frames and 
pcm_io_frames to u64 and add a new IOCTL.