Re: [PATCH 1/3] ASoC: qcom: q6apm: Wait for SPF to be ready before registering

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ASoC: qcom: q6apm: unsynchronized state polling in
  q6apm_wait_spf_ready()
- [Low] ASoC: qcom: q6apm: missing Fixes trailer for apm_probe()
  readiness fix
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] ASoC: qdsp6: use-after-free in apm_probe() [1]
--

commit 57c0d12f224a6a066f01b2ff52ff14b9359892f8
Author: Shawn Guo <shengchao.guo@oss.qualcomm.com>

> ASoC: qcom: q6apm: Wait for SPF to be ready before registering
>
> apm_probe() sends APM_CMD_GET_SPF_STATE once and ignores the result.
> At cold boot this is harmless: module loading and PDR lookup delay the
> probe by tens of milliseconds after the DSP is up, so SPF is listening
> by the time the command is sent.
>
> After an ADSP restart, everything is already loaded and PDR reports the
> audio PD up at once. The GPR service is added about 0.5 ms after the
> remoteproc reports the DSP up, and SPF drops a command sent that early
> without replying:
>
>   remoteproc remoteproc0: remote processor adsp is now up
>   qcom,apr ...: Adding APR/GPR dev: gprsvc:service:2:1
>   qcom-apm gprsvc:service:2:1: CMD timeout for [1001021] opcode
>
> Once the q6apm component is registered, a sound card whose other
> components are still bound can bind right away, inside apm_probe(), and
> issue APM_CMD_SHARED_MEM_MAP_REGIONS before SPF can handle it. The map
> times out and the card fails to come up:
>
>   qcom-apm gprsvc:service:2:1: CMD timeout for [100100c] opcode
>
> Poll GET_SPF_STATE before registering the component. Simply retrying
> q6apm_get_apm_state() is not enough: it goes through
> audioreach_send_cmd_sync(), whose 5 second timeout makes every dropped
> command stall the probe, and the card with it, for 5 seconds. Instead,
> send the command directly and wait only 100 ms for the reply, for up to
> 5 seconds in total, which is the worst case of the single command sent
> before. A timed out try is retried at once, while a "not ready" reply
> or a send failure backs off for 100 ms first. On Nord, SPF answers
> within 100-200 ms of the restart. If it still does not answer, warn and
> carry on as before rather than failing the probe.
>
> Signed-off-by: Shawn Guo <shengchao.guo@oss.qualcomm.com>

[Severity: Low]
Should this bug fix include a Fixes: tag for the commit that added the
unchecked q6apm_get_apm_state() call in apm_probe()?

  Fixes: 5477518b8a0e ("ASoC: qdsp6: audioreach: add q6apm support")

> diff --git a/sound/soc/qcom/qdsp6/q6apm.c b/sound/soc/qcom/qdsp6/q6apm.c
> index 4cb597d737b16..e7ba6bc931421 100644
> --- a/sound/soc/qcom/qdsp6/q6apm.c
> +++ b/sound/soc/qcom/qdsp6/q6apm.c

[ ... ]

> @@ -154,6 +156,47 @@ static int q6apm_get_apm_state(struct q6apm *apm)
>  	return apm->state;
>  }
>  
> +/*
> + * Right after the DSP boots, the SPF drops commands without replying. Poll
> + * GET_SPF_STATE with a short timeout per try instead of the 5 s one of
> + * q6apm_send_cmd_sync(), so a dropped command does not stall the probe.
> + */
> +static bool q6apm_wait_spf_ready(struct q6apm *apm)
> +{
> +	unsigned long timeout = jiffies + msecs_to_jiffies(Q6APM_SPF_READY_MS);
> +	gpr_device_t *gdev = apm->gdev;
> +
> +	do {
> +		struct gpr_pkt *pkt __free(kfree) =
> +			audioreach_alloc_apm_cmd_pkt(0, APM_CMD_GET_SPF_STATE, 0);
> +		long rc;
> +
> +		if (IS_ERR(pkt))
> +			return false;
> +
> +		pkt->hdr.dest_domain = audioreach_gpr_dest_domain(gdev);
> +
> +		mutex_lock(&apm->lock);
> +		apm->result.opcode = 0;
> +		apm->state = 0;
> +		rc = gpr_send_pkt(gdev, pkt);
> +		if (rc >= 0)
> +			rc = wait_event_timeout(apm->wait,
> +					apm->result.opcode == APM_CMD_RSP_GET_SPF_STATE,
> +					msecs_to_jiffies(Q6APM_SPF_POLL_MS));
> +		mutex_unlock(&apm->lock);
> +
> +		if (apm->state)
> +			return true;

[Severity: Medium]
Can q6apm_wait_spf_ready() race with apm_callback() on apm->result.opcode
and apm->state?

sound/soc/qcom/qdsp6/q6apm.c:apm_callback() {
    ...
	case APM_CMD_RSP_GET_SPF_STATE:
		apm->result.opcode = hdr->opcode;
		apm->result.status = 0;
		/* First word of result it state */
		apm->state = result->opcode;
		wake_up(&apm->wait);
		break;
    ...
}

CPU0                              CPU1 (apr_rxwq)
q6apm_wait_spf_ready()
  wait_event_timeout() -> 0
  /* next iteration */
  mutex_lock(&apm->lock)
  apm->result.opcode = 0
                                  apm_callback()
                                    apm->result.opcode =
                                      APM_CMD_RSP_GET_SPF_STATE
                                    apm->state = 1
  apm->state = 0  <-- clobbers state
  gpr_send_pkt()
  wait_event_timeout()
    /* opcode already set */
  if (apm->state) /* 0 */

When a poll attempt times out just as a delayed reply arrives, the next
iteration can overwrite apm->state = 1 with 0 while leaving
apm->result.opcode set, or return true on the stale reply while the newly
sent APM_CMD_GET_SPF_STATE is still in flight to overwrite apm->result
during a later command.

Also, apm_callback() writes apm->result.opcode before apm->state without
a lock or barrier, so wait_event_timeout() can observe the opcode update
before apm->state is visible. Because apm_probe() sets g_apm = apm before
calling q6apm_wait_spf_ready(), resetting apm->state = 0 and reading it
outside apm->lock also races with concurrent q6apm_is_adsp_ready() calls.

[ ... ]

> @@ -906,7 +949,12 @@ static int apm_probe(gpr_device_t *gdev)
>  	dev_set_drvdata(dev, apm);
>  	g_apm = apm;
>  
> -	q6apm_get_apm_state(apm);
> +	/*
> +	 * Once the component is registered a card can bind and send commands
> +	 * at once, so wait for the SPF first.
> +	 */
> +	if (!q6apm_wait_spf_ready(apm))
> +		dev_warn(dev, "SPF not ready\n");
>  
>  	ret = snd_soc_register_component(dev, &q6apm_audio_component, NULL, 0);
>  	if (ret < 0) {

[Severity: Critical]
This problem wasn't introduced by this patch, but can a failure in
snd_soc_register_component() or of_platform_populate() leave g_apm
pointing to freed memory [1]?

apm is allocated with devm_kzalloc_obj(), and g_apm = apm is set before
snd_soc_register_component() and of_platform_populate(). If either call
fails, devres frees apm while g_apm remains non-NULL, causing a
use-after-free when a later caller such as q6apm_is_adsp_ready()
dereferences g_apm.

[1] https://sashiko.dev/#/bug/linux-8f66e185-2dfb-43c1-bfaa-a2be8b96868d

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115032027.414953-1-shengchao.guo@oss.qualcomm.com?part=1