2024-03-26 09:47:19

by Duoming Zhou

Subject: [PATCH v2] ALSA: sh: aica: reorder cleanup operations to avoid UAF bugs

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 <[email protected]>
Signed-off-by: Duoming Zhou <[email protected]>
---
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(-)

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 */
--
2.17.1

2024-03-26 11:20:20

by Takashi Iwai

Subject: Re: [PATCH v2] ALSA: sh: aica: reorder cleanup operations to avoid UAF bugs

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 <[email protected]>
> Signed-off-by: Duoming Zhou <[email protected]>
> ---
> Changes in v2:
> - call mod_timer() conditionally and implement PCM sync_stop op.

Thanks, applied now.


Takashi