ASoC: soc-pcm: fix handling of NULL be_substream by kv2019i · Pull Request #4085 · thesofproject/linux

kv2019i · 2022-12-08T12:23:33Z

Fix a few regressions related to handling cases where there are multiple BEs but one of the BE substreams is NULL.

This happens e.g. with echo reference PCM when it is opened with the playback stream not running. FW will return -ENODATA in this case, but with curent kernel, the kernel with hit an OOPS.

ujfalusi · 2022-12-08T13:47:34Z

 		return -EINVAL;
 	}
-	if (fe_substream->pcm->nonatomic && !be_substream->pcm->nonatomic) {
+	if (be_substream && fe_substream->pcm->nonatomic && !be_substream->pcm->nonatomic) {


interesting. In this case we are creating a loophole that misaligned 'atomicity' can slip through?

plbossart

I don't understand how this configuration might work @kv2019i, with no be_substream there would be no trigger. This is super-weird.

ghost · 2022-12-09T19:58:53Z

@kv2019i see suggested extension in plbossart@3998038 and configuration changes in #4092

Fix a NULL dereference in dpcm_process_paths() in a case where there are multiple BEs, but one of them does not have a substream. [ 94.483796] RIP: 0010:dpcm_process_paths+0x1c4/0x585 ... [ 94.483882] dpcm_fe_dai_open+0x8f/0x791 [ 94.483901] snd_pcm_open_substream+0x554/0x7f2 [ 94.483911] snd_pcm_open+0xd2/0x1fa Fixes: bbf7d3b ("ASoC: soc-pcm: align BE 'atomicity' with that of the FE") Link: thesofproject#4083 Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

Fix a NULL dereference in snd_soc_dpcm_stream_lock_irqsave_nested() in a case where there are multiple BEs, but one of them does not have a substream. Fixes: b2ae806 ("ASoC: soc-pcm: serialize BE triggers") Link: thesofproject#4083 Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

When performing actions on BEs using for_each_dpcm_be(), there are multiple places where the BE substream is assumed to be non-NULL. It is possible however to define such topologies in machine drivers and the soc_pcm functions should handle such errors gracefully without kernel oops on NULL dereference. Link: thesofproject#4083 Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

ghost · 2022-12-14T18:25:36Z

@plbossart wrote:

@kv2019i see suggested extension in plbossart@3998038 and configuration changes in #4092

V2 uploaded with a patch added.

Given the original oops is fixed with other means, I'm now wondering whether I should squash all three patches and remove the Fixes-tag. And just keep a link to the SOF issue for reference. That might be more appropriate now.

ghost

One case that remains questionable.
I'd keep the fixes tag, it's fine IMHO.

ghost · 2022-12-14T18:44:53Z

-		be_substream->runtime = NULL;
+		if (be_substream) {
+			__soc_pcm_close(be, be_substream);
+			be_substream->runtime = NULL;


Line 1569 is still using the be_substream. Not clear to me why you chose to protect the close() but not the hw_free()?

actually you do protect the hw_free in the other case line 1946 now, so it's inconsistent.

@plbossart Ran out of time, but I now realized some of the checks are superfluous as "be->dpcm[stream].state" can only be in limited states when BE substream is NULL. Need to go through all the additions. E.g. I think the case of L1946 is not needed.

In dpcm_be_dai_stop(), be->dpcm[stream].users will be NULL as the BE is skipped in dai_startup(), so this check is not needed.

ghost

@plbossart I went through all cases and it seems it was no accident we only hit a NULL deref in the two places (covered by first two patches) in soc-pcm.c. The other code locations where be_substream is used, are already protected via either stream-state or users-count (see comments inline). So I'm thinking of going back to the original two patches.

ghost · 2022-12-16T07:44:51Z

-		be_substream->runtime = NULL;
+		if (be_substream) {
+			__soc_pcm_close(be, be_substream);
+			be_substream->runtime = NULL;


In dpcm_be_dai_stop(), be->dpcm[stream].users will be NULL as the BE is skipped in dai_startup(), so this check is not needed.

ghost · 2022-12-16T07:47:17Z


-		__soc_pcm_hw_free(be, be_substream);
+		if (be_substream)
+			__soc_pcm_hw_free(be, be_substream);


Here checks on "be->dpcm[stream].state" at L1935 protect against NULL be_substream usage.

ghost · 2022-12-16T07:48:14Z


+		if (!be_substream)
+			continue;
+


Same here, checks on stream state on L2013 help to avoid NULL usage.

ghost · 2022-12-16T07:49:22Z


+		if (!be_substream)
+			continue;
+


Ditto here, checks on L2050.

ghost · 2022-12-16T07:49:50Z


+		if (!be_substream)
+			continue;
+


And here likewise, L2418.

ghost · 2022-12-16T15:27:16Z

@plbossart I went through all cases and it seems it was no accident we only hit a NULL deref in the two places (covered by first two patches) in soc-pcm.c. The other code locations where be_substream is used, are already protected via either stream-state or users-count (see comments inline). So I'm thinking of going back to the original two patches.

I don't have the time today to look into this is details, so will trust your analysis @kv2019i. Maybe just add a comment in the commit message that all other cases are already protected via either stream-state or users-count.

ghost · 2023-04-17T20:27:26Z

@kv2019i what do you want to do with this PR? Merge it as it and send upstream, or close it?

ghost · 2023-05-09T09:11:48Z

Closing until I have bandwidth to work on this.

kv2019i requested review from ajitkupandey, bardliao, dbaluta, kuanhsuncheng, lgirdwood, plbossart, ranj063, ujfalusi and yaochunhung as code owners December 8, 2022 12:23

kv2019i mentioned this pull request Dec 8, 2022

[BUG][ADL-N] Kernel panic when echo reference stream is opend #4083

Closed

ujfalusi reviewed Dec 8, 2022

View reviewed changes

plbossart reviewed Dec 8, 2022

View reviewed changes

Comment thread sound/soc/soc-pcm.c

Comment thread sound/soc/soc-pcm.c

plbossart mentioned this pull request Dec 9, 2022

ASoC: Intel: boards: fix SOF-generated echo reference #4092

Merged

Kai Vehmanen added 3 commits December 14, 2022 18:44

kv2019i force-pushed the 202212-fix-echo-ref branch from b78b280 to 80afc9e Compare December 14, 2022 18:21

plbossart reviewed Dec 14, 2022

View reviewed changes

kv2019i commented Dec 16, 2022

View reviewed changes

kv2019i closed this May 9, 2023

Conversation

kv2019i commented Dec 8, 2022

Uh oh!

ujfalusi Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ghost commented Dec 9, 2022

Uh oh!

ghost commented Dec 14, 2022

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

ghost Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

ghost Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

ghost Dec 16, 2022

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost Dec 16, 2022

Choose a reason for hiding this comment

Uh oh!

ghost Dec 16, 2022

Choose a reason for hiding this comment

Uh oh!

ghost Dec 16, 2022

Choose a reason for hiding this comment

Uh oh!

ghost Dec 16, 2022

Choose a reason for hiding this comment

Uh oh!

ghost Dec 16, 2022

Choose a reason for hiding this comment

Uh oh!

ghost commented Dec 16, 2022

Uh oh!

ghost commented Apr 17, 2023

Uh oh!

ghost commented May 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants