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

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

[Duoming Zhou]

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@zju.edu.cn>
---
 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);
 	flush_work(&(dreamcastcard->spu_dma_work));
-	del_timer(&dreamcastcard->timer);
 	dreamcastcard->substream = NULL;
 	kfree(dreamcastcard->channel);
 	spu_disable();
-- 
2.17.1

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

[Takashi Iwai]

On Mon, 25 Mar 2024 04:39:46 +0100,
Duoming Zhou 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@zju.edu.cn>
> ---
>  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.


thanks,

Takashi


>  	flush_work(&(dreamcastcard->spu_dma_work));
> -	del_timer(&dreamcastcard->timer);
>  	dreamcastcard->substream = NULL;
>  	kfree(dreamcastcard->channel);
>  	spu_disable();
> -- 
> 2.17.1
> 

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@zju.edu.cn>
> > ---
> >  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

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

[Takashi Iwai]

On Mon, 25 Mar 2024 15:26:42 +0100,
duoming@zju.edu.cn wrote:
> 
> 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@zju.edu.cn>
> > > ---
> > >  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);
 	}
 }
 

HTH,

Takashi

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

[duoming]

On Mon, 25 Mar 2024 15:44: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@zju.edu.cn>
> > > > ---
> > > >  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);
        /*get the queue to do the work */
        schedule_work(&(dreamcastcard->spu_dma_work));
        mod_timer(&dreamcastcard->timer, jiffies + 4);
@@ -347,6 +349,7 @@ static int snd_aicapcm_pcm_open(struct snd_pcm_substream
        dreamcastcard->clicks = 0;
        dreamcastcard->current_period = 0;
        dreamcastcard->dma_check = 0;
+       __snd_pcm_set_state(runtime, SNDRV_PCM_STATE_OPEN);
        return 0;
 }

@@ -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);
+       del_timer_sync(&dreamcastcard->timer);
        flush_work(&(dreamcastcard->spu_dma_work));
-       del_timer(&dreamcastcard->timer);
        dreamcastcard->substream = NULL;
        kfree(dreamcastcard->channel);
        spu_disable();

Do you think this plan is proper?

Best regards,
Duoming Zhou

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

[Takashi Iwai]

On Tue, 26 Mar 2024 07:24:14 +0100,
duoming@zju.edu.cn wrote:
> 
> On Mon, 25 Mar 2024 15:44: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@zju.edu.cn>
> > > > > ---
> > > > >  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:

- 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()


thanks,

Takashi

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

[duoming]

On Tue, 26 Mar 2024 08:08:29 +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@zju.edu.cn>
> > > > > > ---
> > > > > >  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();

If you think the above plan is proper, I will send the v2.

Best regards,
Duoming Zhou

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

[Takashi Iwai]

On Tue, 26 Mar 2024 09:06:15 +0100,
duoming@zju.edu.cn wrote:
> 
> On Tue, 26 Mar 2024 08:08:29 +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@zju.edu.cn>
> > > > > > > ---
> > > > > > >  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).


thanks,

Takashi

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@zju.edu.cn>
> > > > > > > > ---
> > > > > > > >  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@zju.edu.cn/

Best regards,
Duoming Zhou