[PATCH] ASoC: SOF: Intel: Fix stream cleanup on pcm close

[PATCH] ASoC: SOF: Intel: Fix stream cleanup on pcm close

[Cezary Rojewski]

Field "substream" gets assigned during stream setup in
hda_dsp_pcm_hw_params() but it is never cleared afterwards during
hda_dsp_pcm_close(). Now, any non-pcm operation e.g.: compress can
mistakenly make use of that pointer as it's bypassing all
"if (s->substream)" checks.

Nulling the pointer during close operation ensures no wild pointers are
left behind.

Fixes: cdae3b9a47aa ("ASoC: SOF: Intel: Add Intel specific HDA PCM operations")
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/sof/intel/hda-pcm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/soc/sof/intel/hda-pcm.c b/sound/soc/sof/intel/hda-pcm.c
index a46a6baa1c3f..4b3a89cf20e7 100644
--- a/sound/soc/sof/intel/hda-pcm.c
+++ b/sound/soc/sof/intel/hda-pcm.c
@@ -246,5 +246,6 @@ int hda_dsp_pcm_close(struct snd_sof_dev *sdev,
 
 	/* unbinding pcm substream to hda stream */
 	substream->runtime->private_data = NULL;
+	hstream->substream = NULL;
 	return 0;
 }
-- 
2.17.1

Re: [PATCH] ASoC: SOF: Intel: Fix stream cleanup on pcm close

[Pierre-Louis Bossart]

On 2/18/20 8:10 AM, Cezary Rojewski wrote:
> Field "substream" gets assigned during stream setup in
> hda_dsp_pcm_hw_params() but it is never cleared afterwards during
> hda_dsp_pcm_close(). Now, any non-pcm operation e.g.: compress can
> mistakenly make use of that pointer as it's bypassing all
> "if (s->substream)" checks.
> 
> Nulling the pointer during close operation ensures no wild pointers are
> left behind.
> 
> Fixes: cdae3b9a47aa ("ASoC: SOF: Intel: Add Intel specific HDA PCM operations")
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>   sound/soc/sof/intel/hda-pcm.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/sound/soc/sof/intel/hda-pcm.c b/sound/soc/sof/intel/hda-pcm.c
> index a46a6baa1c3f..4b3a89cf20e7 100644
> --- a/sound/soc/sof/intel/hda-pcm.c
> +++ b/sound/soc/sof/intel/hda-pcm.c
> @@ -246,5 +246,6 @@ int hda_dsp_pcm_close(struct snd_sof_dev *sdev,
>   
>   	/* unbinding pcm substream to hda stream */
>   	substream->runtime->private_data = NULL;
> +	hstream->substream = NULL;
>   	return 0;
>   }


Humm, yes we should clean this, but wondering if the close() operation 
is the right place. Doing this is hda_dsp_stream_hw_free() sounds more 
logical to me?

Re: [PATCH] ASoC: SOF: Intel: Fix stream cleanup on pcm close

[Cezary Rojewski]

On 2020-02-18 17:45, Pierre-Louis Bossart wrote:
> On 2/18/20 8:10 AM, Cezary Rojewski wrote:
>> Field "substream" gets assigned during stream setup in
>> hda_dsp_pcm_hw_params() but it is never cleared afterwards during
>> hda_dsp_pcm_close(). Now, any non-pcm operation e.g.: compress can
>> mistakenly make use of that pointer as it's bypassing all
>> "if (s->substream)" checks.
>>
>> Nulling the pointer during close operation ensures no wild pointers are
>> left behind.
>>
>> Fixes: cdae3b9a47aa ("ASoC: SOF: Intel: Add Intel specific HDA PCM 
>> operations")
>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>> ---
>>   sound/soc/sof/intel/hda-pcm.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/sound/soc/sof/intel/hda-pcm.c 
>> b/sound/soc/sof/intel/hda-pcm.c
>> index a46a6baa1c3f..4b3a89cf20e7 100644
>> --- a/sound/soc/sof/intel/hda-pcm.c
>> +++ b/sound/soc/sof/intel/hda-pcm.c
>> @@ -246,5 +246,6 @@ int hda_dsp_pcm_close(struct snd_sof_dev *sdev,
>>       /* unbinding pcm substream to hda stream */
>>       substream->runtime->private_data = NULL;
>> +    hstream->substream = NULL;
>>       return 0;
>>   }
> 
> 
> Humm, yes we should clean this, but wondering if the close() operation 
> is the right place. Doing this is hda_dsp_stream_hw_free() sounds more 
> logical to me?

Ain't hda-pcm.c the best place for it as "hstream->substream = 
substream" happens there too? If the cleanup is to be done in 
_hw_free(), then I'd expect the same to happen to the original 
assignments. Doubt we want to do the later so.. _close() for the win?

In general the existing hstream->substream initialization looks kinda 
disconnected from the actual stream assignment code - _stream_get() - as 
if the duties of the state machine were shared.

Czarek

Re: [PATCH] ASoC: SOF: Intel: Fix stream cleanup on pcm close

[Pierre-Louis Bossart]

Hi Cezary,

>>> diff --git a/sound/soc/sof/intel/hda-pcm.c 
>>> b/sound/soc/sof/intel/hda-pcm.c
>>> index a46a6baa1c3f..4b3a89cf20e7 100644
>>> --- a/sound/soc/sof/intel/hda-pcm.c
>>> +++ b/sound/soc/sof/intel/hda-pcm.c
>>> @@ -246,5 +246,6 @@ int hda_dsp_pcm_close(struct snd_sof_dev *sdev,
>>>       /* unbinding pcm substream to hda stream */
>>>       substream->runtime->private_data = NULL;
>>> +    hstream->substream = NULL;
>>>       return 0;
>>>   }
>>
>>
>> Humm, yes we should clean this, but wondering if the close() operation 
>> is the right place. Doing this is hda_dsp_stream_hw_free() sounds more 
>> logical to me?
> 
> Ain't hda-pcm.c the best place for it as "hstream->substream = 
> substream" happens there too? If the cleanup is to be done in 
> _hw_free(), then I'd expect the same to happen to the original 
> assignments. Doubt we want to do the later so.. _close() for the win?
> 
> In general the existing hstream->substream initialization looks kinda 
> disconnected from the actual stream assignment code - _stream_get() - as 
> if the duties of the state machine were shared.

I am having difficulties interpreting your answer, i.e. I don't know 
what the last sentence refers to.

Currently open() and close() are perfectly symmetrical, I don't really 
see why you'd want to change and add an imbalanced set of operations, 
unless you moved

hstream->substream = substream;

to the open() instead of hw_params().

Or alternatively add a hw_free() in hda-pcm.c to mirror what's done in 
hw_params.

Re: [PATCH] ASoC: SOF: Intel: Fix stream cleanup on pcm close

[Cezary Rojewski]

On 2020-02-18 20:05, Pierre-Louis Bossart wrote:
>>
>> Ain't hda-pcm.c the best place for it as "hstream->substream = 
>> substream" happens there too? If the cleanup is to be done in 
>> _hw_free(), then I'd expect the same to happen to the original 
>> assignments. Doubt we want to do the later so.. _close() for the win?
>>
>> In general the existing hstream->substream initialization looks kinda 
>> disconnected from the actual stream assignment code - _stream_get() - 
>> as if the duties of the state machine were shared.
> 
> I am having difficulties interpreting your answer, i.e. I don't know 
> what the last sentence refers to.

It's just safer to tie substream assignment directly with _stream_get() 
so problems such as this don't stay hidden and arise later during the 
development process. _stream_assign() in /hda/ext/hdac_ext_stream/ & 
_stream_release in /hda/hdac_stream do exactly that.

> Currently open() and close() are perfectly symmetrical, I don't really 
> see why you'd want to change and add an imbalanced set of operations, 
> unless you moved
> 
> hstream->substream = substream;
> 
> to the open() instead of hw_params().
> 
> Or alternatively add a hw_free() in hda-pcm.c to mirror what's done in 
> hw_params.

Relocated to _hw_free(). Thanks.