Re: [PATCH 1/2] PCI: dwc: rcar-gen4: Replace pe_rst GPIO PERST with GPIO reset aux driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] PCI: dwc: rcar-gen4: broken error handling in rcar_gen4_pcie_host_perst_get()
- [High] PCI: dwc: rcar-gen4: rcar->perst UAF across suspend/resume in host_deinit()
- [Medium] PCI: dwc: Kconfig: PCIE_RCAR_GEN4_HOST selects RESET_GPIO without dependencies
--

commit 5d439d30a4d77a2c89c803394ba82b6874d3a0c5
Author: Marek Vasut <marek.vasut+renesas@mailbox.org>

PCI: dwc: rcar-gen4: Replace pe_rst GPIO PERST with GPIO reset aux driver

This commit removes the pe_rst GPIO descriptor from the DesignWare PCIe core
and switches the R-Car Gen4 PCIe host driver to acquire PERST# as a reset
control via the reset-gpio auxiliary driver or from a root port subnode in
rcar_gen4_pcie_host_init().

> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index 0a0d56fcf0ac9..d65cf1e9d842b 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -350,6 +350,7 @@ config PCIE_RCAR_GEN4_HOST
>  	depends on PCI_MSI
>  	select PCIE_DW_HOST
>  	select PCIE_RCAR_GEN4
> +	select RESET_GPIO

[Severity: Medium]
In drivers/reset/Kconfig, RESET_GPIO is guarded by if RESET_CONTROLLER and
has depends on GPIOLIB:

drivers/reset/Kconfig:
if RESET_CONTROLLER
...
config RESET_GPIO
	tristate "GPIO reset controller"
	depends on GPIOLIB
	select AUXILIARY_BUS

Since PCIE_RCAR_GEN4_HOST only depends on ARM64 && (ARCH_RENESAS ||
COMPILE_TEST) and PCI_MSI, can selecting RESET_GPIO here trigger Kconfig
unmet direct dependency warnings and build failures when RESET_CONTROLLER=n
or GPIOLIB=n (for example with COMPILE_TEST=y)?

> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index fceecbd16355c..d3bf01485e11d 100644
> --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
[ ... ]
> @@ -594,19 +583,84 @@ static int rcar_gen4_pcie_enable_device(struct pci_host_bridge *bridge,
>  	return 0;
>  }
>  
> +static int rcar_gen4_pcie_host_perst_get(struct dw_pcie *dw)
> +{
> +	struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
> +	struct device_node *root_port;
> +	struct device *dev = dw->dev;
> +	struct reset_control *perst;
> +	int index;
[ ... ]
> +	index = reset_control_get_count(dev);
> +	if (index < 0)
> +		return index;
> +
> +	/* Attempt to request GPIO PERST# in PCIe controller node. */
> +	perst = of_reset_control_get_exclusive_by_index(dev->of_node, index);
> +	if (!IS_ERR(perst))
> +		goto exit;
> +
> +	/* No GPIO reset, test for root port subnode. */
> +	root_port = of_get_next_available_child(dev->of_node, NULL);
> +	if (!root_port)
> +		return dev_err_probe(dev, -EINVAL, "Failed to get PERST# in root port\n");
> +
> +	/* Attempt to request reset controller PERST# in root port subnode. */
> +	perst = of_reset_control_get_optional_exclusive(root_port, "perst");
> +	of_node_put(root_port);
> +	if (IS_ERR(perst))
> +		return dev_err_probe(dev, PTR_ERR(perst), "Failed to get PERST#\n");

[Severity: High]
What happens in rcar_gen4_pcie_host_perst_get() when reset-gpios is present
in dev->of_node, but of_reset_control_get_exclusive_by_index() returns
ERR_PTR(-EPROBE_DEFER) because the GPIO provider or the reset-gpio auxiliary
driver has not bound yet?

In drivers/reset/core.c:__fwnode_reset_control_get(), if reset-gpios is
present in the node, __reset_add_reset_gpio_device() or __reset_find_rcdev()
returns -EPROBE_DEFER until the GPIO device and the reset.gpio auxiliary
device are ready.

Because rcar_gen4_pcie_host_perst_get() does not check whether PTR_ERR(perst)
is something other than -ENOENT before falling through to the root_port
lookup:

1. On R-Car V4H boards (such as r8a779g0.dtsi with White Hawk or Sparrow
Hawk), the controller node has a pci@0,0 child node without a "perst" reset
property while reset-gpios is in the controller node. Falling through on
-EPROBE_DEFER causes of_reset_control_get_optional_exclusive(root_port,
"perst") to return NULL, so rcar->perst is set to NULL and probe succeeds
without acquiring or deasserting the GPIO PERST#.

2. On R-Car S4 and V4M boards (such as r8a779f0.dtsi Spider and r8a779h0.dtsi
Gray Hawk), there is no root port child node, so falling through on
-EPROBE_DEFER hits if (!root_port) and fails probe permanently with -EINVAL
instead of deferring.

3. Since reset-gpios is optional according to the binding and commit
description, if a controller node has neither reset-gpios nor a root port
child node, of_reset_control_get_exclusive_by_index() returns ERR_PTR(-ENOENT)
and if (!root_port) now fails probe with -EINVAL instead of treating PERST# as
optional.

Should this check for PTR_ERR(perst) != -ENOENT after
of_reset_control_get_exclusive_by_index(), and allow a missing root_port when
PTR_ERR(perst) == -ENOENT?

[ ... ]
> @@ -616,6 +670,10 @@ static int rcar_gen4_pcie_host_init(struct dw_pcie_rp *pp)
>  	struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
>  	int ret;
>  
> +	ret = rcar_gen4_pcie_host_perst_get(dw);
> +	if (ret)
> +		return ret;
> +
>  	if (pp->bridge)
>  		pp->bridge->enable_device = rcar_gen4_pcie_enable_device;
>  
> @@ -623,7 +681,7 @@ static int rcar_gen4_pcie_host_init(struct dw_pcie_rp *pp)
>  
>  	ret = rcar->drvdata->init(rcar);
>  	if (ret)
> -		return ret;
> +		goto err_init;
>  
>  	/*
>  	 * According to the section 3.5.7.2 "RC Mode" in DWC PCIe Dual Mode
> @@ -646,6 +704,8 @@ static int rcar_gen4_pcie_host_init(struct dw_pcie_rp *pp)
>  
>  err:
>  	rcar->drvdata->deinit(rcar);
> +err_init:
> +	reset_control_put(rcar->perst);
>  	return ret;
>  }
>  
> @@ -656,6 +716,7 @@ static void rcar_gen4_pcie_host_deinit(struct dw_pcie_rp *pp)
>  
>  	rcar_gen4_pcie_host_perst_assert(pp, true);
>  	rcar->drvdata->deinit(rcar);
> +	reset_control_put(rcar->perst);
>  }

[Severity: High]
Can moving rcar_gen4_pcie_host_perst_get() and reset_control_put(rcar->perst)
into the dw_pcie_host_ops .init and .deinit callbacks cause a use-after-free
and double-free of rcar->perst across system suspend/resume followed by
driver removal?

In drivers/pci/controller/dwc/pcie-designware-host.c, the .deinit and .init
callbacks are also invoked during noirq suspend and resume:

dw_pcie_suspend_noirq()
  pci->pp.ops->deinit(&pci->pp)
    rcar_gen4_pcie_host_deinit()
      reset_control_put(rcar->perst);

dw_pcie_resume_noirq()
  pci->pp.ops->init(&pci->pp)
    rcar_gen4_pcie_host_init()
  ...
err_deinit:
  pci->pp.ops->deinit(&pci->pp)
    rcar_gen4_pcie_host_deinit()
      reset_control_put(rcar->perst);

When dw_pcie_suspend_noirq() runs, rcar_gen4_pcie_host_deinit() frees
rcar->perst via reset_control_put() while leaving the dangling pointer in
rcar->perst.

If the device is later unbound while suspended, or if dw_pcie_resume_noirq()
fails (either inside rcar_gen4_pcie_host_init() or at err_deinit: when link
training times out in dw_pcie_wait_for_link()), a subsequent
rcar_gen4_pcie_remove() -> dw_pcie_host_deinit() ->
rcar_gen4_pcie_host_deinit() will call reset_control_assert(rcar->perst) and
reset_control_put(rcar->perst) on the already-freed rcar->perst pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028172133.20903-1-marek.vasut+renesas@mailbox.org?part=1