[PATCH v2 1/3] ALSA: pcm: introduce INFO_NO_REWINDS flag

[Takashi Iwai]

On Tue, 12 Oct 2021 18:41:19 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> 
> On 10/12/21 10:27 AM, Takashi Iwai wrote:
> > On Tue, 12 Oct 2021 17:15:56 +0200,
> > Pierre-Louis Bossart wrote:
> >>
> >>> In either way, the new_ptr has to be validated beforehand that it's
> >>> within 0..boundary-1.  (old_ptr is assumed to be valid.)
> >>
> >> In the 3 of the calls to pcm_lib_apply_appl_ptr(), the check is done
> >> already prior to calling that function
> >> 	if (appl_ptr >= runtime->boundary)
> >> 		appl_ptr -= runtime->boundary;
> >> 	err = pcm_lib_apply_appl_ptr(substream, appl_ptr);
> >>
> >>
> >> it's rather unclear to me why the same check is not done for sync_ptr, e.g.
> >>
> >> if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
> >> 	err = pcm_lib_apply_appl_ptr(substream,	
> >> 			     sync_ptr.c.control.appl_ptr);
> >>
> >> if (!(sflags & SNDRV_PCM_SYNC_PTR_APPL)) {
> >> 	err = pcm_lib_apply_appl_ptr(substream,
> >> 				scontrol.appl_ptr);
> >>
> >> Should I add a check there, or add a check inside of
> >> pcm_lib_apply_appl_ptr() which would be a duplicate in the majority of
> >> cases?
> > 
> > I guess adding in pcm_lib_appl_appl_ptr() would be easier and safer.
> > There is even one more place that is calling pcm_lib_apply_appl_ptr()
> > in the very latest commit (a fix for a buggy 32bit compat ioctl).
> 
> ok, here's the code I'll start testing. Thanks a lot Takashi for your help.
> 
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index a144a3f68e9e..e839459916ca 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -2127,11 +2127,30 @@ int pcm_lib_apply_appl_ptr(struct
> snd_pcm_substream *substream,
>  {
>         struct snd_pcm_runtime *runtime = substream->runtime;
>         snd_pcm_uframes_t old_appl_ptr = runtime->control->appl_ptr;
> +       snd_pcm_sframes_t diff;
>         int ret;
> 
>         if (old_appl_ptr == appl_ptr)
>                 return 0;
> 
> +       /*
> +        * check if a rewind is requested by the application, after
> +        * verifying the new appl_ptr is in the 0..boundary range
> +        */
> +       if (substream->runtime->info & SNDRV_PCM_INFO_NO_REWINDS) {
> +               if (appl_ptr >= runtime->boundary)
> +                       appl_ptr -= runtime->boundary;

The boundary check can (or should) be done unconditionally.
It was too naive to assume a sane appl_ptr passed always.
And, it can rather return an error.  So,

	if (appl_ptr >= runtime->boundary)
		return -EINVAL;

	/* check if a rewind is requested by the application */
	if (substream->runtime->info & SNDRV_PCM_INFO_NO_REWINDS) {
		diff = appl_ptr - old_appl_ptr;
		....

> +               if (diff >= 0) {
> +                       if (diff > runtime->buffer_size)
> +                               return 0;
> +               } else {
> +                       if (runtime->boundary + diff > runtime->buffer_size)
> +                               return 0;

I'm not sure whether we should return 0 here.  In snd_pcm_rewind() it
returns 0 due to application breakage, though.


thanks,

Takashi