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

[Takashi Iwai]

On Tue, 21 Mar 2017 11:32:51 +0100,
Arnaud Pouliquen wrote:
> 
> 
> 
> On 03/20/2017 06:42 PM, Takashi Iwai wrote:
> > On Mon, 20 Mar 2017 18:09:15 +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 call snd_pcm_stream_lock is player->substream is already null.
> > 
> > Hrm, how the first call of snd_pcm_stream_lock() with
> > player->substream to be guaranteed to be non-NULL?
> 
> Not sure that it is possible to be 100% save, but player->substream
> should not be null...
> 
>  Use case at risk: Stop request occurs just before IRQ.
> 1) IRQ handler locked on snd_pcm_stream_lock(player->substream)
> 2) uni_player_stop clear IRQ flags and set player->state ==
> UNIPERIF_STATE_STOPPED. So no more IRQs after UNIPERIF_STATE_STOPPED state.
> 3) As stream is cleared after close, i guess that between stop and close
> following IRQ handler code should be executed
> 
>   	if (player->state == UNIPERIF_STATE_STOPPED) {
>   		/* Unexpected IRQ: do nothing */
>  		snd_pcm_stream_unlock(player->substream);
>   		return IRQ_NONE;
>   	}
> 
> Anyway adding a check of player->substream in IRQ handler before locking
> seems more save.

Well, if the PCM stop happens right after the call of
uni_player_irq_handler() but just before the spinlock call, you may
still hit NULL dereference.

That is, it's not safe to refer to player->substream unless we ensure
non-NULL in other way.


thanks,

Takashi

> 
> I will add it in a V2
> 
> Thanks
> 
> Arnaud
> 
> > 
> > 
> > Takashi
> > 
> > 
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen at st.com>
> >> ---
> >>  sound/soc/sti/uniperif_player.c | 9 +++------
> >>  sound/soc/sti/uniperif_reader.c | 8 +++++---
> >>  2 files changed, 8 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/sound/soc/sti/uniperif_player.c b/sound/soc/sti/uniperif_player.c
> >> index 60ae31a..6fa9eee 100644
> >> --- a/sound/soc/sti/uniperif_player.c
> >> +++ b/sound/soc/sti/uniperif_player.c
> >> @@ -65,8 +65,10 @@ static irqreturn_t uni_player_irq_handler(int irq, void *dev_id)
> >>  	unsigned int status;
> >>  	unsigned int tmp;
> >>  
> >> +	snd_pcm_stream_lock(player->substream);
> >>  	if (player->state == UNIPERIF_STATE_STOPPED) {
> >>  		/* Unexpected IRQ: do nothing */
> >> +		snd_pcm_stream_unlock(player->substream);
> >>  		return IRQ_NONE;
> >>  	}
> >>  
> >> @@ -88,9 +90,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);
> >>  		}
> >>  
> >>  		ret = IRQ_HANDLED;
> >> @@ -104,9 +104,7 @@ static irqreturn_t uni_player_irq_handler(int irq, void *dev_id)
> >>  		SET_UNIPERIF_ITM_BCLR_DMA_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);
> >>  
> >>  		ret = IRQ_HANDLED;
> >>  	}
> >> @@ -138,13 +136,12 @@ static irqreturn_t uni_player_irq_handler(int irq, void *dev_id)
> >>  		dev_err(player->dev, "Underflow recovery failed\n");
> >>  
> >>  		/* Stop the player */
> >> -		snd_pcm_stream_lock(player->substream);
> >>  		snd_pcm_stop(player->substream, SNDRV_PCM_STATE_XRUN);
> >> -		snd_pcm_stream_unlock(player->substream);
> >>  
> >>  		ret = IRQ_HANDLED;
> >>  	}
> >>  
> >> +	snd_pcm_stream_unlock(player->substream);
> >>  	return ret;
> >>  }
> >>  
> >> diff --git a/sound/soc/sti/uniperif_reader.c b/sound/soc/sti/uniperif_reader.c
> >> index 5992c6a..9273f59 100644
> >> --- a/sound/soc/sti/uniperif_reader.c
> >> +++ b/sound/soc/sti/uniperif_reader.c
> >> @@ -46,9 +46,11 @@ static irqreturn_t uni_reader_irq_handler(int irq, void *dev_id)
> >>  	struct uniperif *reader = dev_id;
> >>  	unsigned int status;
> >>  
> >> +	snd_pcm_stream_lock(reader->substream);
> >>  	if (reader->state == UNIPERIF_STATE_STOPPED) {
> >>  		/* Unexpected IRQ: do nothing */
> >>  		dev_warn(reader->dev, "unexpected IRQ\n");
> >> +		snd_pcm_stream_unlock(reader->substream);
> >>  		return IRQ_HANDLED;
> >>  	}
> >>  
> >> @@ -60,13 +62,13 @@ static irqreturn_t uni_reader_irq_handler(int irq, void *dev_id)
> >>  	if (unlikely(status & UNIPERIF_ITS_FIFO_ERROR_MASK(reader))) {
> >>  		dev_err(reader->dev, "FIFO error detected\n");
> >>  
> >> -		snd_pcm_stream_lock(reader->substream);
> >>  		snd_pcm_stop(reader->substream, SNDRV_PCM_STATE_XRUN);
> >> -		snd_pcm_stream_unlock(reader->substream);
> >>  
> >> -		return IRQ_HANDLED;
> >> +		ret = IRQ_HANDLED;
> >>  	}
> >>  
> >> +	snd_pcm_stream_unlock(reader->substream);
> >> +
> >>  	return ret;
> >>  }
> >>  
> >> -- 
> >> 1.9.1
> >>
>