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

[Arnaud Pouliquen]

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.

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