Re: [PATCH v3 2/2] pinctrl: qcom: Add MSM8952 tlmm pinctrl driver

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The `_groups` arrays for `qdss_tracedata_b` and `atest_combodac_to_gpio_native` list several GPIO pins that do not actually include these functions in their corresponding `PINGROUP` macro definitions.

Pre-existing issues:
- [High] Use-After-Free on pin function memory during driver unbind due to devres LIFO ordering.
--

commit e99bbcf6f4490c02c597eff9b2155619cf4f3564
Author: Muzaffer Kadir <muzafferkadir@mainlining.org>

pinctrl: qcom: Add MSM8952 tlmm pinctrl driver

This commit adds the pinctrl, pinmux, pinconf, and gpiolib driver for the
Qualcomm TLMM block found on the MSM8952 platform. It defines the available
pins, their functions, and the pingroups for the SoC.

> diff --git a/drivers/pinctrl/qcom/pinctrl-msm8952.c b/drivers/pinctrl/qcom/pinctrl-msm8952.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/pinctrl/qcom/pinctrl-msm8952.c
[ ... ]
> +static const char *const qdss_tracedata_b_groups[] = {
> +	"gpio8",  "gpio9",  "gpio12", "gpio13", "gpio23", "gpio42",
> +	"gpio43", "gpio44", "gpio45", "gpio46", "gpio47", "gpio66",
> +	"gpio86", "gpio87", "gpio88", "gpio92",
> +};
[ ... ]
> +static const char *const atest_combodac_to_gpio_native_groups[] = {
> +	"gpio4",  "gpio12", "gpio13", "gpio20", "gpio21", "gpio28",  "gpio29",
> +	"gpio30", "gpio39", "gpio40", "gpio41", "gpio42", "gpio43",  "gpio44",
> +	"gpio45", "gpio46", "gpio47", "gpio48", "gpio67", "gpio115",
> +};
[ ... ]
> +static const struct msm_pingroup msm8952_groups[] = {
[ ... ]
> +	PINGROUP(8, blsp_spi3, blsp_uart3, sec_mi2s, pbs0, NA, NA, NA, NA, NA,
> +		 NA, NA, NA),

[Severity: Medium]
Will attempting to mux qdss_tracedata_b or atest_combodac_to_gpio_native on
some of these advertised pins fail at runtime?

The qdss_tracedata_b_groups array claims that qdss_tracedata_b can be
muxed on gpio8, gpio9, gpio12, and gpio13. Similarly,
atest_combodac_to_gpio_native_groups claims gpio12, gpio13, etc., support
it. However, looking at the hardware group definitions in msm8952_groups
(such as for gpio8), these functions appear to be missing.

If a device tree configuration requests these mux mappings, will it cause
msm_pinmux_set_mux to return -EINVAL because the requested function isn't
actually in the pin's function list?

[ ... ]
> +static int msm8952_pinctrl_probe(struct platform_device *pdev)
> +{
> +	return msm_pinctrl_probe(pdev, &msm8952_pinctrl);
> +}

[Severity: High]
This is a pre-existing issue, but does calling msm_pinctrl_probe expose a
use-after-free regression during driver unbind?

Looking at msm_pinctrl_probe in drivers/pinctrl/qcom/pinctrl-msm.c:

msm_pinctrl_probe() {
    ...
    ret = devm_pinctrl_register_and_init(&pdev->dev, &pctrl->desc,
                                         pctrl, &pctrl->pctrl);
    ...
    for (i = 0; i < soc_data->nfunctions; i++) {
        func = &soc_data->functions[i];
        ret = pinmux_generic_add_pinfunction(pctrl->pctrl, func, NULL);
    ...
}

Because devm_kzalloc is used inside pinmux_generic_add_pinfunction, and
it is called after the pinctrl device is registered with devm, the devres LIFO
ordering means this memory will be freed before the pinctrl unregister action
runs.

If the device is unbound, and a user concurrently reads the debugfs
pinmux-functions file, could pinmux_generic_get_function_name dereference
the freed memory while the radix tree still holds pointers to it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-msm8952-pinctrl-v3-0-110515fff513@mainlining.org?part=2