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

[Arnaud Pouliquen]

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.

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