| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <75af0c57.6e39.18e796e478f.Coremail.duoming@zju.edu.cn>
Date: Tue, 26 Mar 2024 14:24:14 +0800 (GMT+08:00)
From: duoming@....edu.cn
To: "Takashi Iwai" <tiwai@...e.de>
Cc: linux-sound@...r.kernel.org, linux-kernel@...r.kernel.org,
tiwai@...e.com, perex@...ex.cz
Subject: Re: [PATCH] ALSA: sh: aica: reorder cleanup operations to avoid UAF
bug
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@....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
Powered by blists - more mailing lists