From: duoming@zju.edu.cn
To: "Takashi Iwai" <tiwai@suse.de>
Cc: linux-sound@vger.kernel.org, linux-kernel@vger.kernel.org,
tiwai@suse.com, perex@perex.cz
Subject: Re: [PATCH] ALSA: sh: aica: reorder cleanup operations to avoid UAF bug
Date: Tue, 26 Mar 2024 14:24:14 +0800 (GMT+08:00) [thread overview]
Message-ID: <75af0c57.6e39.18e796e478f.Coremail.duoming@zju.edu.cn> (raw)
In-Reply-To: <874jcuxtf9.wl-tiwai@suse.de>
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
next prev parent reply other threads:[~2024-03-26 6:24 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-25 3:39 [PATCH] ALSA: sh: aica: reorder cleanup operations to avoid UAF bug Duoming Zhou
2024-03-25 8:16 ` Takashi Iwai
2024-03-25 14:26 ` duoming
2024-03-25 14:44 ` Takashi Iwai
2024-03-26 6:24 ` duoming [this message]
2024-03-26 7:08 ` Takashi Iwai
2024-03-26 8:06 ` duoming
2024-03-26 8:25 ` Takashi Iwai
2024-03-26 9:50 ` duoming
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=75af0c57.6e39.18e796e478f.Coremail.duoming@zju.edu.cn \
--to=duoming@zju.edu.cn \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-sound@vger.kernel.org \
--cc=perex@perex.cz \
--cc=tiwai@suse.com \
--cc=tiwai@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.