[alsa-devel] [PATCH v2 2/2] ASoC: STI: Fix null ptr deference in IRQ handler

[Takashi Iwai]

On Mon, 27 Mar 2017 18:15:20 +0200,
Arnaud Pouliquen wrote:
> 
> 
> Hello Takashi
> 
> Thanks for your comments,
> Please find my answers below
> 
> Regards Arnaud
> 
> On 03/23/2017 08:02 PM, Takashi Iwai wrote:
> > On Thu, 23 Mar 2017 19:39:55 +0100,
> > Arnaud Pouliquen wrote:
> >>
> >> With RTlinux a race condition has been found that leads to NULL ptr crash:
> >> - On CPU 0: uni_player_irq_handler is called to treat XRUN
> >>  "(player->state == UNIPERIF_STATE_STOPPED)" is FALSE so status is checked,
> >>  dev_err(player->dev, "FIFO underflow error detected") is printed
> >> and then snd_pcm_stream_lock should be called to lock stream for stopping.
> >> - On CPU 1: application stop and close the stream.
> >> Issue is that the stop and shutdown functions are executed while
> >> "FIFO underflow error detected" is printed.
> >> So when CPU 0 calls snd_pcm_stream_lock, player->substream is already null.
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen at st.com>
> >>
> >> ---
> >> V2: Add spinlock to protect player/reader->substream
> >> ---
> >>  sound/soc/sti/uniperif.h        |  1 +
> >>  sound/soc/sti/uniperif_player.c | 34 +++++++++++++++++++++++-----------
> >>  sound/soc/sti/uniperif_reader.c | 24 ++++++++++++++++++++----
> >>  3 files changed, 44 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/sound/soc/sti/uniperif.h b/sound/soc/sti/uniperif.h
> >> index d487dd2..cfcb0ea 100644
> >> --- a/sound/soc/sti/uniperif.h
> >> +++ b/sound/soc/sti/uniperif.h
> >> @@ -1299,6 +1299,7 @@ struct uniperif {
> >>  	int ver; /* IP version, used by register access macros */
> >>  	struct regmap_field *clk_sel;
> >>  	struct regmap_field *valid_sel;
> >> +	spinlock_t irq_lock; /* used to prevent race condition with IRQ */
> >>  
> >>  	/* capabilities */
> >>  	const struct snd_pcm_hardware *hw;
> >> diff --git a/sound/soc/sti/uniperif_player.c b/sound/soc/sti/uniperif_player.c
> >> index 60ae31a..8f3a582 100644
> >> --- a/sound/soc/sti/uniperif_player.c
> >> +++ b/sound/soc/sti/uniperif_player.c
> >> @@ -65,10 +65,13 @@ static irqreturn_t uni_player_irq_handler(int irq, void *dev_id)
> >>  	unsigned int status;
> >>  	unsigned int tmp;
> >>  
> >> -	if (player->state == UNIPERIF_STATE_STOPPED) {
> >> -		/* Unexpected IRQ: do nothing */
> >> -		return IRQ_NONE;
> >> -	}
> >> +	spin_lock(&player->irq_lock);
> >> +	if (!player->substream)
> >> +		goto IRQ_END;
> > 
> > This will be NULL-dereference, as it calls snd_pcm_stream_unlock() there.
> > Also, use lower letters for labels.
> Beginner's fault...
> > 
> > 
> >> +
> >> +	snd_pcm_stream_lock(player->substream);
> > 
> > Actually we don't need to take this lock here any longer since we have
> > irq_lock.  Better to keep the stream lock only around the stop.
> 
> Needed to protect "player->state".
> The irq_lock does not ensure that application calls a PCM stop in
> parallel on another CPU. That means that i need also to protect
> uni_player_stop and uni_player_start to protect layer->state.
> But in this case i will have a double lock when call snd_pcm_stop in IRQ
> handler.

Fair enough.  It's not ideal to use the stream lock to protect the
other stuff, but it should work fine.


thanks,

Takashi