Re: [PATCH v4 7/8] soc: qcom: pmic-glink-altmode: pass down HPD_IRQ events

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing `cancel_work_sync()` upon device unbind leads to a use-after-free vulnerability for the `altmode` and `alt_port` structures.
- [High] Missing `reinit_completion()` can cause subsequent firmware requests to prematurely succeed if a previous request timed out.
--

commit 94236240b5d1e90ad191dae53982a24598cd3d4c
Author: Dmitry Baryshkov <[email protected]>

soc: qcom: pmic-glink-altmode: pass down HPD_IRQ events

This commit passes IRQ_HPD events from the firmware to the DisplayPort
driver through the HPD bridge. This enables the DP source to receive
notifications about events on the sink side.

> diff --git a/drivers/soc/qcom/pmic_glink_altmode.c b/drivers/soc/qcom/pmic_glink_altmode.c
> index 619bad2c27eeb..946eb20b8f836 100644
> --- a/drivers/soc/qcom/pmic_glink_altmode.c
> +++ b/drivers/soc/qcom/pmic_glink_altmode.c
> @@ -373,7 +373,11 @@ static void pmic_glink_altmode_worker(struct work_struct *work)
>  		else
>  			conn_status = connector_status_disconnected;
>  
> -		drm_aux_hpd_bridge_notify(&alt_port->bridge->dev, conn_status);
> +		drm_aux_hpd_bridge_notify_extra(&alt_port->bridge->dev,
> +						conn_status,
> +						alt_port->hpd_irq ?
> +						DRM_CONNECTOR_DP_IRQ_HPD :
> +						DRM_CONNECTOR_NO_EXTRA_STATUS);
>  	} else if (alt_port->mux_ctrl == MUX_CTRL_STATE_TUNNELING) {
>  		if (alt_port->svid == USB_TYPEC_TBT_SID)
>  			pmic_glink_altmode_enable_tbt(altmode, alt_port);

[Severity: High]
This isn't a bug introduced by this patch, but since we are modifying the
workqueue handler, is there a potential use-after-free due to missing work
cancellation on teardown?

In pmic_glink_altmode_probe(), the driver allocates the altmode structure via
devm_kzalloc() and initializes altmode->enable_work and alt_port->work, but
it does not register a .remove callback or a devres action to synchronously
cancel these work items.

When the auxiliary device is unbound, the devres framework will automatically
free the altmode structure and release resources.

If pmic_glink_altmode_worker() is currently pending or executing, could it
dereference the freed altmode and alt_port pointers?

[Severity: High]
This is a pre-existing issue, but I also noticed a potential state machine
issue regarding how requests are sent to the firmware.

In pmic_glink_altmode_request(), the driver sends a request and waits for an
acknowledgment:

	ret = pmic_glink_send(altmode->client, &req, sizeof(req));
	...
	wait_for_completion_timeout(&altmode->pan_ack, ...);

If the wait times out, the function returns -ETIMEDOUT. If the delayed
acknowledgment eventually arrives, pmic_glink_altmode_callback() will call
complete(), incrementing the counter.

Since the driver never calls reinit_completion() before sending a new request,
will the next call to pmic_glink_altmode_request() instantly succeed by
consuming the stale completion counter instead of waiting for the actual
hardware acknowledgment?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic