| Message ID | 20240326094238.95442-1-duoming@zju.edu.cn (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 051e0840ffa8ab25554d6b14b62c9ab9e4901457 |
| Headers | show |
| Series | [v2] ALSA: sh: aica: reorder cleanup operations to avoid UAF bugs | expand |
On Tue, 26 Mar 2024 10:42:38 +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 snd_pcm_substream is closing, the aica_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 and other possible corner cases, > call mod_timer() conditionally in run_spu_dma(), then implement > PCM sync_stop op to cancel both the timer and worker. The sync_stop > op will be called from PCM core appropriately when needed. > > Fixes: 198de43d758c ("[ALSA] Add ALSA support for the SEGA Dreamcast PCM device") > Suggested-by: Takashi Iwai <tiwai@suse.de> > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn> > --- > Changes in v2: > - call mod_timer() conditionally and implement PCM sync_stop op. Thanks, applied now. Takashi
diff --git a/sound/sh/aica.c b/sound/sh/aica.c index 320ac792c7f..3182c634464 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,19 @@ 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); dreamcastcard->substream = NULL; kfree(dreamcastcard->channel); spu_disable(); @@ -401,6 +411,7 @@ static const struct snd_pcm_ops snd_aicapcm_playback_ops = { .prepare = snd_aicapcm_pcm_prepare, .trigger = snd_aicapcm_pcm_trigger, .pointer = snd_aicapcm_pcm_pointer, + .sync_stop = snd_aicapcm_pcm_sync_stop, }; /* TO DO: set up to handle more than one pcm instance */
The dreamcastcard->timer could schedule the spu_dma_work and the spu_dma_work could also arm the dreamcastcard->timer. When the snd_pcm_substream is closing, the aica_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 and other possible corner cases, call mod_timer() conditionally in run_spu_dma(), then implement PCM sync_stop op to cancel both the timer and worker. The sync_stop op will be called from PCM core appropriately when needed. Fixes: 198de43d758c ("[ALSA] Add ALSA support for the SEGA Dreamcast PCM device") Suggested-by: Takashi Iwai <tiwai@suse.de> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn> --- Changes in v2: - call mod_timer() conditionally and implement PCM sync_stop op. sound/sh/aica.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)