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

[Takashi Iwai]

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.


> +
> +	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.


> +	if (player->state == UNIPERIF_STATE_STOPPED)
> +		goto IRQ_END;
>  
>  	/* Get interrupt status & clear them immediately */
>  	status = GET_UNIPERIF_ITS(player);
> @@ -88,9 +91,7 @@ static irqreturn_t uni_player_irq_handler(int irq, void *dev_id)
>  			SET_UNIPERIF_ITM_BCLR_FIFO_ERROR(player);
>  
>  			/* Stop the player */
> -			snd_pcm_stream_lock(player->substream);
>  			snd_pcm_stop(player->substream, SNDRV_PCM_STATE_XRUN);
> -			snd_pcm_stream_unlock(player->substream);

BTW, these three calls can be simplified with snd_pcm_stop_xrun().


Takashi