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

[duoming]

On Mon, 25 Mar 2024 09:16:12 +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:

diff --git a/sound/sh/aica.c b/sound/sh/aica.c
index 320ac792c7fe..dab005eda7f0 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 (!dreamcastcard->shutdown)
+                       mod_timer(&dreamcastcard->timer, jiffies + 1);
        }
 }

@@ -347,6 +348,7 @@ static int snd_aicapcm_pcm_open(struct snd_pcm_substream
        dreamcastcard->clicks = 0;
        dreamcastcard->current_period = 0;
        dreamcastcard->dma_check = 0;
+       dreamcastcard->shutdown = false;
        return 0;
 }

@@ -354,8 +356,9 @@ static int snd_aicapcm_pcm_close(struct snd_pcm_substream
                                 *substream)
 {
        struct snd_card_aica *dreamcastcard = substream->pcm->private_data;
+       dreamcastcard->shutdown = true;
+       del_timer_sync(&dreamcastcard->timer);
        flush_work(&(dreamcastcard->spu_dma_work));
-       del_timer(&dreamcastcard->timer);
        dreamcastcard->substream = NULL;
        kfree(dreamcastcard->channel);
        spu_disable();
diff --git a/sound/sh/aica.h b/sound/sh/aica.h
index 021b132e088e..59a9deaf3dd3 100644
--- a/sound/sh/aica.h
+++ b/sound/sh/aica.h
@@ -65,4 +65,5 @@ struct snd_card_aica {
        struct timer_list timer;
        int master_volume;
        int dma_check;
+       bool shutdown;
 };

I would be very appreciate, if you could give me some suggestions
about the above solution.

Best regards,
Duoming Zhou