Received-SPF: pass (google.com: domain of linux-kernel+bounces-117283-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 25 Mar 2024 15:44:26 +0100
Message-ID: <874jcuxtf9.wl-tiwai@suse.de>
From: Takashi Iwai <tiwai@suse.de>
To: duoming@zju.edu.cn
Cc: "Takashi Iwai" <tiwai@suse.de>,
	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
In-Reply-To: <43e102f3.61dc.18e7601a2f2.Coremail.duoming@zju.edu.cn>
References: <20240325033946.47052-1-duoming@zju.edu.cn>
	<871q7yybeb.wl-tiwai@suse.de>
	<43e102f3.61dc.18e7601a2f2.Coremail.duoming@zju.edu.cn>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0
Precedence: bulk
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII

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