Skip to content

Conversation

@ujfalusi
Copy link
Collaborator

Other, non DAI copier widgets could have the same stream name (sname) as the ALH copier and in that case the copier->data is NULL, no alh_data is attached, which could lead to NULL pointer dereference. We could check for this NULL pointer in sof_ipc4_prepare_copier_module() and avoid the crash, but a similar loop in sof_ipc4_widget_setup_comp_dai() will miscalculate the ALH device count, causing broken audio.

The correct fix is to harden the matching logic by making sure that the

  1. widget is a DAI widget - so dai = w->private is valid
  2. the dai (and thus the copier) is ALH copier

Fixes: 0e357b5 ("ASoC: SOF: ipc4-topology: add SoundWire/ALH aggregation support")
Reported-by: Seppo Ingalsuo seppo.ingalsuo@linux.intel.com
Link: thesofproject/sof#9652

Other, non DAI copier widgets could have the same  stream name (sname) as
the ALH copier and in that case the copier->data is NULL, no alh_data is
attached, which could lead to NULL pointer dereference.
We could check for this NULL pointer in sof_ipc4_prepare_copier_module()
and avoid the crash, but a similar loop in sof_ipc4_widget_setup_comp_dai()
will miscalculate the ALH device count, causing broken audio.

The correct fix is to harden the matching logic by making sure that the
1. widget is a DAI widget - so dai = w->private is valid
2. the dai (and thus the copier) is ALH copier

Fixes: 0e357b5 ("ASoC: SOF: ipc4-topology: add SoundWire/ALH aggregation support")
Reported-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
Link: thesofproject/sof#9652
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
@bardliao
Copy link
Collaborator

That happens when the topology provides less ALH dais in the aggregation mode. For example, there are 2 amps in different links, but NUM_SDW_AMP_LINKS=1 in topology. Shouldn't we return error in this case?

@ujfalusi
Copy link
Collaborator Author

That happens when the topology provides less ALH dais in the aggregation mode. For example, there are 2 amps in different links, but NUM_SDW_AMP_LINKS=1 in topology. Shouldn't we return error in this case?

No, this happens when a module copier have the same stream name as the alh copier: thesofproject/sof#9652 (comment)

That is a NULL dereference, but we also miscount the number of alh things.

@bardliao
Copy link
Collaborator

@ujfalusi Can you fix the checkpatch warning?
WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
I.e. Change Link: https://github.com/thesofproject/sof/pull/9652 to Closes: https://github.com/thesofproject/sof/pull/9652

@ujfalusi
Copy link
Collaborator Author

@ujfalusi Can you fix the checkpatch warning? WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report I.e. Change Link: https://github.com/thesofproject/sof/pull/9652 to Closes: https://github.com/thesofproject/sof/pull/9652

I thought about that but it does not sound correct, the link is PR and not an issue and that PR is now updated to not set the stream name for the module.copier.
At the same time I want to give credit to Seppo for the find, I will leave it as Link:

@bardliao bardliao merged commit 5a72ff8 into thesofproject:topic/sof-dev Nov 21, 2024
8 of 9 checks passed
@ujfalusi ujfalusi deleted the peter/sof/pr/ipc4-tplg-alh-crash-01 branch December 13, 2024 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants