Re: [PATCH 2/2] soc: qcom: apr: Publish GPR callback before client probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] soc: qcom: apr: race with gpr_do_rx_callback() on apr_device_probe() failure
--

commit b8085aea7f4dfac0f85898613864891fa165e4b0
Author: Mohit Sharma <mohit.sharma@oss.qualcomm.com>

soc: qcom: apr: Publish GPR callback before client probe

This commit installs the service callback in apr_device_probe() before calling
the client driver's probe function when the packet router type is GPR, so that
responses to commands sent during probe can be dispatched. If the client probe
fails, it clears the callback before returning the error.

> diff --git a/drivers/soc/qcom/apr.c b/drivers/soc/qcom/apr.c
> index ea7f83916d8d8..829e5c8fdf984 100644
> --- a/drivers/soc/qcom/apr.c
> +++ b/drivers/soc/qcom/apr.c
> @@ -365,13 +365,24 @@ static int apr_device_probe(struct device *dev)
>  {
>  	struct apr_device *adev = to_apr_device(dev);
>  	struct apr_driver *adrv = to_apr_driver(dev->driver);
> +	struct packet_router *apr = dev_get_drvdata(dev->parent);
>  	int ret;
>  
> +	if (apr->type == PR_TYPE_GPR)
> +		adev->svc.callback = adrv->gpr_callback;
> +
>  	ret = adrv->probe(adev);
> -	if (!ret)
> +	if (ret) {
> +		if (apr->type == PR_TYPE_GPR)
> +			adev->svc.callback = NULL;
> +
> +		return ret;
> +	}

[Severity: High]
Can clearing adev->svc.callback on probe failure without synchronizing with
apr->rxwq race with an in-flight gpr_do_rx_callback() execution and cause a
use-after-free on devm-allocated driver data?

In drivers/soc/qcom/apr.c, apr_add_device() inserts &adev->svc into
apr->svcs_idr before calling device_register(&adev->dev). Once
apr_device_probe() sets adev->svc.callback = adrv->gpr_callback, incoming
packets for that port on apr->rxwq can invoke svc->callback via
gpr_do_rx_callback():

drivers/soc/qcom/apr.c:gpr_do_rx_callback() {
	spin_lock_irqsave(&gpr->svcs_lock, flags);
	svc = idr_find(&gpr->svcs_idr, hdr->dest_port);
	spin_unlock_irqrestore(&gpr->svcs_lock, flags);

	if (!svc) {
		dev_err(gpr->dev, "GPR: Port(%x) is not registered\n",
			hdr->dest_port);
		return -EINVAL;
	}

	if (svc->callback)
		svc->callback(&resp, svc->priv, 0);
}

During adrv->probe(adev), client drivers such as apm_probe() in
sound/soc/qcom/qdsp6/q6apm.c and prm_probe() in sound/soc/qcom/qdsp6/q6prm.c
allocate their private state with devm_kzalloc(), publish it via
dev_set_drvdata(), and can then fail or defer probe after sending commands or
while unsolicited packets arrive:

sound/soc/qcom/qdsp6/q6apm.c:apm_probe() {
	...
	dev_set_drvdata(dev, apm);
	g_apm = apm;

	q6apm_get_apm_state(apm);

	ret = snd_soc_register_component(dev, &q6apm_audio_component, NULL, 0);
	if (ret < 0) {
		dev_err(dev, "failed to register q6apm: %d\n", ret);
		return ret;
	}

	ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
	if (ret)
		snd_soc_unregister_component(dev);

	return ret;
}

sound/soc/qcom/qdsp6/q6prm.c:prm_probe() {
	...
	dev_set_drvdata(dev, cc);

	if (!q6apm_is_adsp_ready())
		return -EPROBE_DEFER;

	return devm_of_platform_populate(dev);
}

When adrv->probe(adev) returns an error, apr_device_probe() clears
adev->svc.callback = NULL with a plain store and returns immediately to
really_probe() -> device_unbind_cleanup(), which frees devres allocations
before clearing drvdata:

drivers/base/dd.c:device_unbind_cleanup() {
	devres_release_all(dev);
	arch_teardown_dma_ops(dev);
	kfree(dev->dma_range_map);
	dev->dma_range_map = NULL;
	device_set_driver(dev, NULL);
	dev_set_drvdata(dev, NULL);
}

If gpr_do_rx_callback() on apr->rxwq has already passed the svc->callback
check, could apm_callback() or prm_callback() concurrently dereference and
write to the freed struct q6apm or struct q6prm obtained from
dev_get_drvdata(&gdev->dev) (or hit a NULL function pointer dereference if
svc->callback is reloaded after the unlocked check in gpr_do_rx_callback())?

Should adev->svc.callback be updated with READ_ONCE()/WRITE_ONCE() (or under
lock) and apr->rxwq be flushed after clearing adev->svc.callback on the probe
error path before returning?

> +
> +	if (apr->type != PR_TYPE_GPR)
>  		adev->svc.callback = adrv->gpr_callback;
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static void apr_device_remove(struct device *dev)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022144130.2443667-2-mohit.sharma@oss.qualcomm.com?part=2