Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ASoC: SOF: ipc4-topology: Harden loops for looking up ALH copiers #5247

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 [email protected]
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 <[email protected]>
Link: thesofproject/sof#9652
Signed-off-by: Peter Ujfalusi <[email protected]>
@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
11 of 14 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