Re: [PATCH 1/8] usb: typec: tcpm: allocate pd_list entries non device managed

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: typec: tcpm: leak of pd_list entries on
  tcpm_fw_get_caps() error paths
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] usb: typec: tcpm: fwnode reference leak in tcpm_fw_get_caps()
  [1]
--

commit b86bde7535f2bafb293f7e234db00124b8e22755
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

usb: typec: tcpm: allocate pd_list entries non device managed

This commit replaces devm_kzalloc_obj() and devm_kfree() with unmanaged
kzalloc_obj() and kfree() for port->pd_list[i] allocations.

> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index bb90211660cae..07d2a03d2b8d7 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c

[ ... ]

> @@ -8391,7 +8391,7 @@ static int tcpm_fw_get_caps(struct tcpm_port *port, struct fwnode_handle *fwnode
>  	}
>  
>  	for (i = 0; i < port->pd_count; i++) {
> -		port->pd_list[i] = devm_kzalloc_obj(port->dev, struct pd_data);
> +		port->pd_list[i] = kzalloc_obj(*port->pd_list[i]);
>  		if (!port->pd_list[i]) {
>  			ret = -ENOMEM;
>  			goto put_capabilities;

[Severity: Medium]
Does switching port->pd_list[i] to unmanaged kzalloc_obj() leak those
entries when tcpm_fw_get_caps() or tcpm_register_port() fails?

In tcpm_fw_get_caps(), neither put_caps nor put_capabilities frees already
allocated port->pd_list[0..i] entries if a later loop iteration or PDO
validation fails.  Even when tcpm_fw_get_caps() succeeds,
tcpm_port_unregister_pd() (where kfree(port->pd_list[i]) runs) is only
reached via out_unregister_pd at the end of tcpm_register_port(), so earlier
failures such as -EPROBE_DEFER from usb_role_switch_get() jump to
out_destroy_wq or out_role_sw_put and leak port->pd_list[i]:

drivers/usb/typec/tcpm/tcpm.c:tcpm_register_port() {
    ...
	if (IS_ERR(port->role_sw)) {
		err = PTR_ERR(port->role_sw);
		goto out_destroy_wq;
	}

	err = devm_tcpm_psy_register(port);
	if (err)
		goto out_role_sw_put;
    ...
out_unregister_pd:
	tcpm_port_unregister_pd(port);
out_role_sw_put:
	usb_role_switch_put(port->role_sw);
out_destroy_wq:
	tcpm_debugfs_exit(port);
	kthread_destroy_worker(port->wq);
	return ERR_PTR(err);
}

[Severity: Medium]
This problem wasn't introduced by this patch, but jumping to
put_capabilities when kzalloc_obj() fails on iteration i > 0 leaks the caps
fwnode reference [1] acquired by fwnode_get_next_child_node() in the
previous iteration, because it bypasses put_caps where fwnode_handle_put(caps)
is called.

[1] https://sashiko.dev/#/bug/linux-d106a4e9-4871-4f7c-8575-50cb448be123

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-fusb302-probe-cleanup-v1-0-e5c65a8c58c8@collabora.com?part=1