Re: [PATCH] ALSA: sh: aica: reorder cleanup operations to avoid UAF bug

[duoming]

On Tue, 26 Mar 2024 09:25:26 +0100 Takashi Iwai wrote:
> > > > > > > > The dreamcastcard->timer could schedule the spu_dma_work and the
> > > > > > > > spu_dma_work could also arm the dreamcastcard->timer.
> > > > > > > > 
> > > > > > > > When the Yamaha AICA card is closing, the dreamcastcard->channel
> > > > > > > > will be deallocated. But it could still be dereferenced in the
> > > > > > > > worker thread. The reason is that del_timer() will return directly
> > > > > > > > regardless of whether the timer handler is running or not and the
> > > > > > > > worker could be rescheduled in the timer handler. As a result, the
> > > > > > > > UAF bug will happen. The racy situation is shown below:
> > > > > > > > 
> > > > > > > >       (Thread 1)                 |      (Thread 2)
> > > > > > > > snd_aicapcm_pcm_close()          |
> > > > > > > >  ...                             |  run_spu_dma() //worker
> > > > > > > >                                  |    mod_timer()
> > > > > > > >   flush_work()                   |
> > > > > > > >   del_timer()                    |  aica_period_elapsed() //timer
> > > > > > > >   kfree(dreamcastcard->channel)  |    schedule_work()
> > > > > > > >                                  |  run_spu_dma() //worker
> > > > > > > >   ...                            |    dreamcastcard->channel-> //USE
> > > > > > > > 
> > > > > > > > In order to mitigate this bug, use timer_shutdown_sync() to shutdown
> > > > > > > > the timer and then use flush_work() to cancel the worker.
> > > > > > > > 
> > > > > > > > Fixes: 198de43d758c ("[ALSA] Add ALSA support for the SEGA Dreamcast PCM device")
> > > > > > > > Signed-off-by: Duoming Zhou <duoming@xxxxxxxxxx>
> > > > > > > > ---
> > > > > > > >  sound/sh/aica.c | 2 +-
> > > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > > 
> > > > > > > > diff --git a/sound/sh/aica.c b/sound/sh/aica.c
> > > > > > > > index 320ac792c7f..bc68a3903f2 100644
> > > > > > > > --- a/sound/sh/aica.c
> > > > > > > > +++ b/sound/sh/aica.c
> > > > > > > > @@ -354,8 +354,8 @@ static int snd_aicapcm_pcm_close(struct snd_pcm_substream
> > > > > > > >  				 *substream)
> > > > > > > >  {
> > > > > > > >  	struct snd_card_aica *dreamcastcard = substream->pcm->private_data;
> > > > > > > > +	timer_shutdown_sync(&dreamcastcard->timer);
> > > > > > > 
> > > > > > > I thought this call invalidates the timer object, hence it can't be
> > > > > > > used again; i.e. it breaks when the stream is re-opened, I suppose?
> > > > > > >
> > > > > > > In general timer_shutdown*() is used for the code path to clean up the
> > > > > > > driver (or the object the timer belongs to).  The PCM close is only
> > > > > > > about the PCM stream, and it's not the place.
> > > > > > > 
> > > > > > > A proper fix would be rather to implement two things:
> > > > > > > - Call mod_timer() conditionally in run_spu_dma()
> > > > > > > - Implement PCM sync_stop op to cancel/flush the work
> > > > > > > 
> > > > > > > The former alone should suffice to fix the UAF in your scenario,
> > > > > > > though.  The latter will cover other possible corner cases.
> > > > > > 
> > > > > > Thank you for your time and reply! I know using timer_shutdown_sync()
> > > > > > is not proper. In order to solve the problem, I add a shutdown flag 
> > > > > > in the struct snd_card_aica and set the flag to true when the PCM 
> > > > > > stream is closing. Then call mod_timer() conditionally in run_spu_dma().
> > > > > > What's more, use del_timer_sync() to stop the timer and put it before 
> > > > > > flush_work(). As a result, both timer and worker could be stopped safely. 
> > > > > > The detail is shown below:
> > > > > 
> > > > > You can use the existing API to check the PCM running state, e.g.
> > > > > 
> > > > > --- a/sound/sh/aica.c
> > > > > +++ b/sound/sh/aica.c
> > > > > @@ -278,7 +278,8 @@ static void run_spu_dma(struct work_struct *work)
> > > > >  		dreamcastcard->clicks++;
> > > > >  		if (unlikely(dreamcastcard->clicks >= AICA_PERIOD_NUMBER))
> > > > >  			dreamcastcard->clicks %= AICA_PERIOD_NUMBER;
> > > > > -		mod_timer(&dreamcastcard->timer, jiffies + 1);
> > > > > +		if (snd_pcm_running(dreamcastcard->substream))
> > > > > +			mod_timer(&dreamcastcard->timer, jiffies + 1);
> > > > >  	}
> > > > >  }
> > > > 
> > > > Thank you for your suggestions, The following is a new plan using the
> > > > existing API to mitigate the bug.
> > > > 
> > > > diff --git a/sound/sh/aica.c b/sound/sh/aica.c
> > > > index 320ac792c7fe..bc003dd91a82 100644
> > > > --- a/sound/sh/aica.c
> > > > +++ b/sound/sh/aica.c
> > > > @@ -278,7 +278,8 @@ static void run_spu_dma(struct work_struct *work)
> > > >                 dreamcastcard->clicks++;
> > > >                 if (unlikely(dreamcastcard->clicks >= AICA_PERIOD_NUMBER))
> > > >                         dreamcastcard->clicks %= AICA_PERIOD_NUMBER;
> > > > -               mod_timer(&dreamcastcard->timer, jiffies + 1);
> > > > +               if (snd_pcm_running(dreamcastcard->substream))
> > > > +                       mod_timer(&dreamcastcard->timer, jiffies + 1);
> > > >         }
> > > >  }
> > > > 
> > > > @@ -316,6 +317,7 @@ static void spu_begin_dma(struct snd_pcm_substream *substream)
> > > >         struct snd_pcm_runtime *runtime;
> > > >         runtime = substream->runtime;
> > > >         dreamcastcard = substream->pcm->private_data;
> > > > +       __snd_pcm_set_state(runtime, SNDRV_PCM_STATE_RUNNING);
> > > 
> > > Such an explicit state change isn't needed, rather wrong.
> > > The above condition check is performed only when kicked off from the
> > > timer handler, and that's always after the stream started.
> > > 
> > > > @@ -354,8 +357,9 @@ static int snd_aicapcm_pcm_close(struct snd_pcm_substream
> > > >                                  *substream)
> > > >  {
> > > >         struct snd_card_aica *dreamcastcard = substream->pcm->private_data;
> > > > +       __snd_pcm_set_state(substream->runtime, SNDRV_PCM_STATE_DISCONNECTED);
> > > 
> > > This breaks things again!  You don't disconnect the device at closing
> > > the stream at all.  And the state change is handled in PCM core side,
> > > not in the driver side.
> > > 
> > > > +       del_timer_sync(&dreamcastcard->timer);
> > > >         flush_work(&(dreamcastcard->spu_dma_work));
> > > > -       del_timer(&dreamcastcard->timer);
> > > 
> > > I'd leave this unchanged.  The UAF itself is covered by the stream
> > > state check.  And, if any, we can change more properly:
> > 
> > I think if we leave the cleanup operations unchanged, the UAF could 
> > still happen. The scenario is shown below:
> > 
> >       (Thread 1)                 |      (Thread 2)
> >                                  |  run_spu_dma() //worker
> >                                  |    if (snd_pcm_running(dreamcastcard->substream))
> > snd_aicapcm_pcm_close()          |
> >  ...                             |  
> >                                  |    mod_timer()
> >   flush_work()                   |
> >   del_timer()                    |  aica_period_elapsed() //timer
> >   kfree(dreamcastcard->channel)  |    schedule_work()
> >                                  |  run_spu_dma() //worker
> >   ...                            |    dreamcastcard->channel-> //USE
> > 
> > So we should implement PCM sync_stop ops like below.
> > 
> > > - Add the same PCM state check at the beginning of
> > >   aica_period_elapsed(), and bail out immediately if not running
> > > 
> > > - Implement PCM sync_stop ops:
> > >   it should have like
> > > 
> > > static int snd_aicapcm_pcm_sync_stop(struct snd_pcm_substream *substream)
> > > {
> > > 	struct snd_card_aica *dreamcastcard = substream->pcm->private_data;
> > > 
> > > 	del_timer_sync(&dreamcastcard->timer);
> > > 	cancel_work_sync(&dreamcastcard->spu_dma_work);
> > > 	return 0;
> > > }
> > > 
> > >   and get rid of the corresponding calls from snd_aicapcm_pcm_close()
> > 
> > Thank you for your suggestions, the improved plan is shown below:
> > 
> > diff --git a/sound/sh/aica.c b/sound/sh/aica.c
> > index 320ac792c7f..2989407606f 100644
> > --- a/sound/sh/aica.c
> > +++ b/sound/sh/aica.c
> > @@ -278,7 +278,8 @@ static void run_spu_dma(struct work_struct *work)
> >                 dreamcastcard->clicks++;
> >                 if (unlikely(dreamcastcard->clicks >= AICA_PERIOD_NUMBER))
> >                         dreamcastcard->clicks %= AICA_PERIOD_NUMBER;
> > -               mod_timer(&dreamcastcard->timer, jiffies + 1);
> > +               if (snd_pcm_running(dreamcastcard->substream))
> > +                       mod_timer(&dreamcastcard->timer, jiffies + 1);
> >         }
> >  }
> > 
> > @@ -290,6 +291,8 @@ static void aica_period_elapsed(struct timer_list *t)
> >         /*timer function - so cannot sleep */
> >         int play_period;
> >         struct snd_pcm_runtime *runtime;
> > +       if (!snd_pcm_running(substream))
> > +               return;
> >         runtime = substream->runtime;
> >         dreamcastcard = substream->pcm->private_data;
> >         /* Have we played out an additional period? */
> > @@ -350,12 +353,20 @@ static int snd_aicapcm_pcm_open(struct snd_pcm_substream
> >         return 0;
> >  }
> > 
> > +static int snd_aicapcm_pcm_sync_stop(struct snd_pcm_substream *substream)
> > +{
> > +       struct snd_card_aica *dreamcastcard = substream->pcm->private_data;
> > +
> > +       del_timer_sync(&dreamcastcard->timer);
> > +       cancel_work_sync(&dreamcastcard->spu_dma_work);
> > +       return 0;
> > +}
> > +
> >  static int snd_aicapcm_pcm_close(struct snd_pcm_substream
> >                                  *substream)
> >  {
> >         struct snd_card_aica *dreamcastcard = substream->pcm->private_data;
> > -       flush_work(&(dreamcastcard->spu_dma_work));
> > -       del_timer(&dreamcastcard->timer);
> > +       snd_aicapcm_pcm_sync_stop(substream);
> >         dreamcastcard->substream = NULL;
> >         kfree(dreamcastcard->channel);
> >         spu_disable();
> 
> Don't call it here explicitly.  sync_stop is a PCM op, i.e. add
> snd_aicapcm_pcm_sync_stop to snd_aicapcm_playback_ops definition,
> instead.
> Then it'll be called from PCM core appropriately when needed
> (e.g. before calling the close callback via hw_free call).

Thank you for your suggestions! I have sent the v2 that adds 
snd_aicapcm_pcm_sync_stop to snd_aicapcm_playback_ops definition.

https://patchwork.kernel.org/project/alsa-devel/patch/20240326094238.95442-1-duoming@xxxxxxxxxx/

Best regards,
Duoming Zhou