diff mbox series

Revert "PCI: j721e: Drop redundant struct device *"

Message ID 20220124122132.435743-1-christian.gmeiner@gmail.com
State New
Headers show
Series Revert "PCI: j721e: Drop redundant struct device *" | expand

Commit Message

Christian Gmeiner Jan. 24, 2022, 12:21 p.m. UTC
This reverts commit 19e863828acf6d8ac8475ba1fd93c0fe17fdc4ef.

Fixes the following oops:
 Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
 Internal error: Oops: 96000004 [#1] PREEMPT SMP
 Modules linked in:
 CPU: 1 PID: 7 Comm: kworker/u4:0 Not tainted 5.17.0-rc1-00086-ge38b27816fea-dirty #71
 Hardware name: CPE0108 (DT)
 Workqueue: events_unbound deferred_probe_work_func
 pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
 pc : j721e_pcie_probe+0x184/0x600
 lr : j721e_pcie_probe+0x170/0x600
 sp : ffff80000957bae0
 x29: ffff80000957bae0 x28: ffff800009357000 x27: ffff00000000c078
 x26: ffff00003fe047a8 x25: 0000000000000000 x24: ffff0000000f5280
 x23: ffff800008c98f78 x22: ffff800008f90ff0 x21: ffff000000231410
 x20: ffff000002ef2780 x19: 0000000000000021 x18: 0000000000000001
 x17: 0000000000000000 x16: 0000000000058c00 x15: ffffffffffffffff
 x14: ffffffffffffffff x13: 0000000000000010 x12: 0101010101010101
 x11: 0000000000000040 x10: ffff8000093e06c8 x9 : ffff8000093e06c0
 x8 : ffff000000400270 x7 : 0000000000000000 x6 : ffff000000231590
 x5 : ffff80000957b9e0 x4 : 0000000000000000 x3 : ffff0000002314f4
 x2 : 0000000000000000 x1 : ffff0000000f5280 x0 : 0000000000000000
 Call trace:
  j721e_pcie_probe+0x184/0x600
  platform_probe+0x68/0xe0
  really_probe+0x144/0x320
  __driver_probe_device+0xc4/0xe0
  driver_probe_device+0x7c/0x110
  __device_attach_driver+0x90/0xe0
  bus_for_each_drv+0x78/0xd0
  __device_attach+0xf0/0x150
  device_initial_probe+0x14/0x20
  bus_probe_device+0x9c/0xb0
  deferred_probe_work_func+0x88/0xc0
  process_one_work+0x1bc/0x340
  worker_thread+0x1f8/0x420
  kthread+0x110/0x120
  ret_from_fork+0x10/0x20
 Code: f9400280 a90573fb d0005396 913fc2d6 (f9400800)

Fixes: 19e863828acf ("PCI: j721e: Drop redundant struct device *")
Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
---
 drivers/pci/controller/cadence/pci-j721e.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Comments

Bjorn Helgaas Jan. 26, 2022, 8:51 p.m. UTC | #1
On Mon, Jan 24, 2022 at 01:21:22PM +0100, Christian Gmeiner wrote:
> This reverts commit 19e863828acf6d8ac8475ba1fd93c0fe17fdc4ef.
> 
> Fixes the following oops:
>  Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010

My fault, sorry for breaking this.  Thanks a lot for the report!

19e863828acf ("PCI: j721e: Drop redundant struct device *") failed to
consider the uses of pcie->cdns_pcie->dev before pcie->cdns_pcie is
initialized.  I'll figure out what to do about it.

>  Internal error: Oops: 96000004 [#1] PREEMPT SMP
>  Modules linked in:
>  CPU: 1 PID: 7 Comm: kworker/u4:0 Not tainted 5.17.0-rc1-00086-ge38b27816fea-dirty #71
>  Hardware name: CPE0108 (DT)
>  Workqueue: events_unbound deferred_probe_work_func
>  pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>  pc : j721e_pcie_probe+0x184/0x600
>  lr : j721e_pcie_probe+0x170/0x600
>  sp : ffff80000957bae0
>  x29: ffff80000957bae0 x28: ffff800009357000 x27: ffff00000000c078
>  x26: ffff00003fe047a8 x25: 0000000000000000 x24: ffff0000000f5280
>  x23: ffff800008c98f78 x22: ffff800008f90ff0 x21: ffff000000231410
>  x20: ffff000002ef2780 x19: 0000000000000021 x18: 0000000000000001
>  x17: 0000000000000000 x16: 0000000000058c00 x15: ffffffffffffffff
>  x14: ffffffffffffffff x13: 0000000000000010 x12: 0101010101010101
>  x11: 0000000000000040 x10: ffff8000093e06c8 x9 : ffff8000093e06c0
>  x8 : ffff000000400270 x7 : 0000000000000000 x6 : ffff000000231590
>  x5 : ffff80000957b9e0 x4 : 0000000000000000 x3 : ffff0000002314f4
>  x2 : 0000000000000000 x1 : ffff0000000f5280 x0 : 0000000000000000
>  Call trace:
>   j721e_pcie_probe+0x184/0x600
>   platform_probe+0x68/0xe0
>   really_probe+0x144/0x320
>   __driver_probe_device+0xc4/0xe0
>   driver_probe_device+0x7c/0x110
>   __device_attach_driver+0x90/0xe0
>   bus_for_each_drv+0x78/0xd0
>   __device_attach+0xf0/0x150
>   device_initial_probe+0x14/0x20
>   bus_probe_device+0x9c/0xb0
>   deferred_probe_work_func+0x88/0xc0
>   process_one_work+0x1bc/0x340
>   worker_thread+0x1f8/0x420
>   kthread+0x110/0x120
>   ret_from_fork+0x10/0x20
>  Code: f9400280 a90573fb d0005396 913fc2d6 (f9400800)
> 
> Fixes: 19e863828acf ("PCI: j721e: Drop redundant struct device *")
> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
> ---
>  drivers/pci/controller/cadence/pci-j721e.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> index 489586a4cdc7..cd43d1898482 100644
> --- a/drivers/pci/controller/cadence/pci-j721e.c
> +++ b/drivers/pci/controller/cadence/pci-j721e.c
> @@ -51,10 +51,11 @@ enum link_status {
>  #define MAX_LANES			2
>  
>  struct j721e_pcie {
> -	struct cdns_pcie	*cdns_pcie;
> +	struct device		*dev;
>  	struct clk		*refclk;
>  	u32			mode;
>  	u32			num_lanes;
> +	struct cdns_pcie	*cdns_pcie;
>  	void __iomem		*user_cfg_base;
>  	void __iomem		*intd_cfg_base;
>  	u32			linkdown_irq_regfield;
> @@ -98,7 +99,7 @@ static inline void j721e_pcie_intd_writel(struct j721e_pcie *pcie, u32 offset,
>  static irqreturn_t j721e_pcie_link_irq_handler(int irq, void *priv)
>  {
>  	struct j721e_pcie *pcie = priv;
> -	struct device *dev = pcie->cdns_pcie->dev;
> +	struct device *dev = pcie->dev;
>  	u32 reg;
>  
>  	reg = j721e_pcie_intd_readl(pcie, STATUS_REG_SYS_2);
> @@ -164,7 +165,7 @@ static const struct cdns_pcie_ops j721e_pcie_ops = {
>  static int j721e_pcie_set_mode(struct j721e_pcie *pcie, struct regmap *syscon,
>  			       unsigned int offset)
>  {
> -	struct device *dev = pcie->cdns_pcie->dev;
> +	struct device *dev = pcie->dev;
>  	u32 mask = J721E_MODE_RC;
>  	u32 mode = pcie->mode;
>  	u32 val = 0;
> @@ -183,7 +184,7 @@ static int j721e_pcie_set_mode(struct j721e_pcie *pcie, struct regmap *syscon,
>  static int j721e_pcie_set_link_speed(struct j721e_pcie *pcie,
>  				     struct regmap *syscon, unsigned int offset)
>  {
> -	struct device *dev = pcie->cdns_pcie->dev;
> +	struct device *dev = pcie->dev;
>  	struct device_node *np = dev->of_node;
>  	int link_speed;
>  	u32 val = 0;
> @@ -204,7 +205,7 @@ static int j721e_pcie_set_link_speed(struct j721e_pcie *pcie,
>  static int j721e_pcie_set_lane_count(struct j721e_pcie *pcie,
>  				     struct regmap *syscon, unsigned int offset)
>  {
> -	struct device *dev = pcie->cdns_pcie->dev;
> +	struct device *dev = pcie->dev;
>  	u32 lanes = pcie->num_lanes;
>  	u32 val = 0;
>  	int ret;
> @@ -219,7 +220,7 @@ static int j721e_pcie_set_lane_count(struct j721e_pcie *pcie,
>  
>  static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie)
>  {
> -	struct device *dev = pcie->cdns_pcie->dev;
> +	struct device *dev = pcie->dev;
>  	struct device_node *node = dev->of_node;
>  	struct of_phandle_args args;
>  	unsigned int offset = 0;
> @@ -376,6 +377,7 @@ static int j721e_pcie_probe(struct platform_device *pdev)
>  	if (!pcie)
>  		return -ENOMEM;
>  
> +	pcie->dev = dev;
>  	pcie->mode = mode;
>  	pcie->linkdown_irq_regfield = data->linkdown_irq_regfield;
>  
> -- 
> 2.34.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Bjorn Helgaas Jan. 27, 2022, 10:29 p.m. UTC | #2
On Mon, Jan 24, 2022 at 01:21:22PM +0100, Christian Gmeiner wrote:
> This reverts commit 19e863828acf6d8ac8475ba1fd93c0fe17fdc4ef.
> 
> Fixes the following oops:
>  Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010

Hi Christian,

Would you mind trying this patch?

commit 9d36a93af8fe ("PCI: j721e: Initialize pcie->cdns_pcie before using it")
Author: Bjorn Helgaas <bhelgaas@google.com>
Date:   Thu Jan 27 15:49:49 2022 -0600

    PCI: j721e: Initialize pcie->cdns_pcie before using it
    
    Christian reported a NULL pointer dereference in j721e_pcie_probe() caused
    by 19e863828acf ("PCI: j721e: Drop redundant struct device *"), which
    removed struct j721e_pcie.dev since there's another copy in struct
    cdns_pcie.dev reachable via j721e_pcie->cdns_pcie->dev.
    
    The problem is that j721e_pcie->cdns_pcie was dereferenced before being
    initialized:
    
      j721e_pcie_probe
        pcie = devm_kzalloc()             # struct j721e_pcie
        j721e_pcie_ctrl_init(pcie)
          dev = pcie->cdns_pcie->dev      <-- dereference cdns_pcie
        switch (mode) {
        case PCI_MODE_RC:
          cdns_pcie = ...                 # alloc as part of pci_host_bridge
          pcie->cdns_pcie = cdns_pcie     <-- initialize pcie->cdns_pcie
    
    Initialize pcie->cdns_pcie before it is used.
    
    Fixes: 19e863828acf ("PCI: j721e: Drop redundant struct device *")
    Reported-by: Christian Gmeiner <christian.gmeiner@gmail.com>
    Link: https://lore.kernel.org/r/20220124122132.435743-1-christian.gmeiner@gmail.com
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index 489586a4cdc7..5d950c1d9fd0 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -372,10 +372,48 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 
 	mode = (u32)data->mode;
 
+	switch (mode) {
+	case PCI_MODE_RC:
+		if (!IS_ENABLED(CONFIG_PCIE_CADENCE_HOST))
+			return -ENODEV;
+
+		bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc));
+		if (!bridge)
+			return -ENOMEM;
+
+		if (!data->byte_access_allowed)
+			bridge->ops = &cdns_ti_pcie_host_ops;
+		rc = pci_host_bridge_priv(bridge);
+		rc->quirk_retrain_flag = data->quirk_retrain_flag;
+		rc->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag;
+
+		cdns_pcie = &rc->pcie;
+		break;
+	case PCI_MODE_EP:
+		if (!IS_ENABLED(CONFIG_PCIE_CADENCE_EP))
+			return -ENODEV;
+
+		ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
+		if (!ep)
+			return -ENOMEM;
+
+		ep->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag;
+
+		cdns_pcie = &ep->pcie;
+		break;
+	default:
+		dev_err(dev, "INVALID device type %d\n", mode);
+		return 0;
+	}
+
+	cdns_pcie->dev = dev;
+	cdns_pcie->ops = &j721e_pcie_ops;
+
 	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
 	if (!pcie)
 		return -ENOMEM;
 
+	pcie->cdns_pcie = cdns_pcie;
 	pcie->mode = mode;
 	pcie->linkdown_irq_regfield = data->linkdown_irq_regfield;
 
@@ -426,28 +464,6 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 
 	switch (mode) {
 	case PCI_MODE_RC:
-		if (!IS_ENABLED(CONFIG_PCIE_CADENCE_HOST)) {
-			ret = -ENODEV;
-			goto err_get_sync;
-		}
-
-		bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc));
-		if (!bridge) {
-			ret = -ENOMEM;
-			goto err_get_sync;
-		}
-
-		if (!data->byte_access_allowed)
-			bridge->ops = &cdns_ti_pcie_host_ops;
-		rc = pci_host_bridge_priv(bridge);
-		rc->quirk_retrain_flag = data->quirk_retrain_flag;
-		rc->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag;
-
-		cdns_pcie = &rc->pcie;
-		cdns_pcie->dev = dev;
-		cdns_pcie->ops = &j721e_pcie_ops;
-		pcie->cdns_pcie = cdns_pcie;
-
 		gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
 		if (IS_ERR(gpiod)) {
 			ret = PTR_ERR(gpiod);
@@ -497,23 +513,6 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 
 		break;
 	case PCI_MODE_EP:
-		if (!IS_ENABLED(CONFIG_PCIE_CADENCE_EP)) {
-			ret = -ENODEV;
-			goto err_get_sync;
-		}
-
-		ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
-		if (!ep) {
-			ret = -ENOMEM;
-			goto err_get_sync;
-		}
-		ep->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag;
-
-		cdns_pcie = &ep->pcie;
-		cdns_pcie->dev = dev;
-		cdns_pcie->ops = &j721e_pcie_ops;
-		pcie->cdns_pcie = cdns_pcie;
-
 		ret = cdns_pcie_init_phy(dev, cdns_pcie);
 		if (ret) {
 			dev_err(dev, "Failed to init phy\n");
@@ -525,8 +524,6 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 			goto err_pcie_setup;
 
 		break;
-	default:
-		dev_err(dev, "INVALID device type %d\n", mode);
 	}
 
 	return 0;
Christian Gmeiner Jan. 31, 2022, 11:06 a.m. UTC | #3
Am Do., 27. Jan. 2022 um 23:29 Uhr schrieb Bjorn Helgaas <helgaas@kernel.org>:
>
> On Mon, Jan 24, 2022 at 01:21:22PM +0100, Christian Gmeiner wrote:
> > This reverts commit 19e863828acf6d8ac8475ba1fd93c0fe17fdc4ef.
> >
> > Fixes the following oops:
> >  Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
>
> Hi Christian,
>
> Would you mind trying this patch?
>

Works - thanks. You can add my Tested-by.

> commit 9d36a93af8fe ("PCI: j721e: Initialize pcie->cdns_pcie before using it")
> Author: Bjorn Helgaas <bhelgaas@google.com>
> Date:   Thu Jan 27 15:49:49 2022 -0600
>
>     PCI: j721e: Initialize pcie->cdns_pcie before using it
>
>     Christian reported a NULL pointer dereference in j721e_pcie_probe() caused
>     by 19e863828acf ("PCI: j721e: Drop redundant struct device *"), which
>     removed struct j721e_pcie.dev since there's another copy in struct
>     cdns_pcie.dev reachable via j721e_pcie->cdns_pcie->dev.
>
>     The problem is that j721e_pcie->cdns_pcie was dereferenced before being
>     initialized:
>
>       j721e_pcie_probe
>         pcie = devm_kzalloc()             # struct j721e_pcie
>         j721e_pcie_ctrl_init(pcie)
>           dev = pcie->cdns_pcie->dev      <-- dereference cdns_pcie
>         switch (mode) {
>         case PCI_MODE_RC:
>           cdns_pcie = ...                 # alloc as part of pci_host_bridge
>           pcie->cdns_pcie = cdns_pcie     <-- initialize pcie->cdns_pcie
>
>     Initialize pcie->cdns_pcie before it is used.
>
>     Fixes: 19e863828acf ("PCI: j721e: Drop redundant struct device *")
>     Reported-by: Christian Gmeiner <christian.gmeiner@gmail.com>
>     Link: https://lore.kernel.org/r/20220124122132.435743-1-christian.gmeiner@gmail.com
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> index 489586a4cdc7..5d950c1d9fd0 100644
> --- a/drivers/pci/controller/cadence/pci-j721e.c
> +++ b/drivers/pci/controller/cadence/pci-j721e.c
> @@ -372,10 +372,48 @@ static int j721e_pcie_probe(struct platform_device *pdev)
>
>         mode = (u32)data->mode;
>
> +       switch (mode) {
> +       case PCI_MODE_RC:
> +               if (!IS_ENABLED(CONFIG_PCIE_CADENCE_HOST))
> +                       return -ENODEV;
> +
> +               bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc));
> +               if (!bridge)
> +                       return -ENOMEM;
> +
> +               if (!data->byte_access_allowed)
> +                       bridge->ops = &cdns_ti_pcie_host_ops;
> +               rc = pci_host_bridge_priv(bridge);
> +               rc->quirk_retrain_flag = data->quirk_retrain_flag;
> +               rc->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag;
> +
> +               cdns_pcie = &rc->pcie;
> +               break;
> +       case PCI_MODE_EP:
> +               if (!IS_ENABLED(CONFIG_PCIE_CADENCE_EP))
> +                       return -ENODEV;
> +
> +               ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
> +               if (!ep)
> +                       return -ENOMEM;
> +
> +               ep->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag;
> +
> +               cdns_pcie = &ep->pcie;
> +               break;
> +       default:
> +               dev_err(dev, "INVALID device type %d\n", mode);
> +               return 0;
> +       }
> +
> +       cdns_pcie->dev = dev;
> +       cdns_pcie->ops = &j721e_pcie_ops;
> +
>         pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
>         if (!pcie)
>                 return -ENOMEM;
>
> +       pcie->cdns_pcie = cdns_pcie;
>         pcie->mode = mode;
>         pcie->linkdown_irq_regfield = data->linkdown_irq_regfield;
>
> @@ -426,28 +464,6 @@ static int j721e_pcie_probe(struct platform_device *pdev)
>
>         switch (mode) {
>         case PCI_MODE_RC:
> -               if (!IS_ENABLED(CONFIG_PCIE_CADENCE_HOST)) {
> -                       ret = -ENODEV;
> -                       goto err_get_sync;
> -               }
> -
> -               bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc));
> -               if (!bridge) {
> -                       ret = -ENOMEM;
> -                       goto err_get_sync;
> -               }
> -
> -               if (!data->byte_access_allowed)
> -                       bridge->ops = &cdns_ti_pcie_host_ops;
> -               rc = pci_host_bridge_priv(bridge);
> -               rc->quirk_retrain_flag = data->quirk_retrain_flag;
> -               rc->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag;
> -
> -               cdns_pcie = &rc->pcie;
> -               cdns_pcie->dev = dev;
> -               cdns_pcie->ops = &j721e_pcie_ops;
> -               pcie->cdns_pcie = cdns_pcie;
> -
>                 gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
>                 if (IS_ERR(gpiod)) {
>                         ret = PTR_ERR(gpiod);
> @@ -497,23 +513,6 @@ static int j721e_pcie_probe(struct platform_device *pdev)
>
>                 break;
>         case PCI_MODE_EP:
> -               if (!IS_ENABLED(CONFIG_PCIE_CADENCE_EP)) {
> -                       ret = -ENODEV;
> -                       goto err_get_sync;
> -               }
> -
> -               ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
> -               if (!ep) {
> -                       ret = -ENOMEM;
> -                       goto err_get_sync;
> -               }
> -               ep->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag;
> -
> -               cdns_pcie = &ep->pcie;
> -               cdns_pcie->dev = dev;
> -               cdns_pcie->ops = &j721e_pcie_ops;
> -               pcie->cdns_pcie = cdns_pcie;
> -
>                 ret = cdns_pcie_init_phy(dev, cdns_pcie);
>                 if (ret) {
>                         dev_err(dev, "Failed to init phy\n");
> @@ -525,8 +524,6 @@ static int j721e_pcie_probe(struct platform_device *pdev)
>                         goto err_pcie_setup;
>
>                 break;
> -       default:
> -               dev_err(dev, "INVALID device type %d\n", mode);
>         }
>
>         return 0;
Bjorn Helgaas Jan. 31, 2022, 11:10 p.m. UTC | #4
On Mon, Jan 31, 2022 at 12:06:21PM +0100, Christian Gmeiner wrote:
> Am Do., 27. Jan. 2022 um 23:29 Uhr schrieb Bjorn Helgaas <helgaas@kernel.org>:
> >
> > On Mon, Jan 24, 2022 at 01:21:22PM +0100, Christian Gmeiner wrote:
> > > This reverts commit 19e863828acf6d8ac8475ba1fd93c0fe17fdc4ef.
> > >
> > > Fixes the following oops:
> > >  Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
> >
> > Hi Christian,
> >
> > Would you mind trying this patch?
> >
> 
> Works - thanks. You can add my Tested-by.

Thanks, applied to for-linus for v5.17.

> 
> > commit 9d36a93af8fe ("PCI: j721e: Initialize pcie->cdns_pcie before using it")
> > Author: Bjorn Helgaas <bhelgaas@google.com>
> > Date:   Thu Jan 27 15:49:49 2022 -0600
> >
> >     PCI: j721e: Initialize pcie->cdns_pcie before using it
> >
> >     Christian reported a NULL pointer dereference in j721e_pcie_probe() caused
> >     by 19e863828acf ("PCI: j721e: Drop redundant struct device *"), which
> >     removed struct j721e_pcie.dev since there's another copy in struct
> >     cdns_pcie.dev reachable via j721e_pcie->cdns_pcie->dev.
> >
> >     The problem is that j721e_pcie->cdns_pcie was dereferenced before being
> >     initialized:
> >
> >       j721e_pcie_probe
> >         pcie = devm_kzalloc()             # struct j721e_pcie
> >         j721e_pcie_ctrl_init(pcie)
> >           dev = pcie->cdns_pcie->dev      <-- dereference cdns_pcie
> >         switch (mode) {
> >         case PCI_MODE_RC:
> >           cdns_pcie = ...                 # alloc as part of pci_host_bridge
> >           pcie->cdns_pcie = cdns_pcie     <-- initialize pcie->cdns_pcie
> >
> >     Initialize pcie->cdns_pcie before it is used.
> >
> >     Fixes: 19e863828acf ("PCI: j721e: Drop redundant struct device *")
> >     Reported-by: Christian Gmeiner <christian.gmeiner@gmail.com>
> >     Link: https://lore.kernel.org/r/20220124122132.435743-1-christian.gmeiner@gmail.com
> >     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >
> > diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> > index 489586a4cdc7..5d950c1d9fd0 100644
> > --- a/drivers/pci/controller/cadence/pci-j721e.c
> > +++ b/drivers/pci/controller/cadence/pci-j721e.c
> > @@ -372,10 +372,48 @@ static int j721e_pcie_probe(struct platform_device *pdev)
> >
> >         mode = (u32)data->mode;
> >
> > +       switch (mode) {
> > +       case PCI_MODE_RC:
> > +               if (!IS_ENABLED(CONFIG_PCIE_CADENCE_HOST))
> > +                       return -ENODEV;
> > +
> > +               bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc));
> > +               if (!bridge)
> > +                       return -ENOMEM;
> > +
> > +               if (!data->byte_access_allowed)
> > +                       bridge->ops = &cdns_ti_pcie_host_ops;
> > +               rc = pci_host_bridge_priv(bridge);
> > +               rc->quirk_retrain_flag = data->quirk_retrain_flag;
> > +               rc->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag;
> > +
> > +               cdns_pcie = &rc->pcie;
> > +               break;
> > +       case PCI_MODE_EP:
> > +               if (!IS_ENABLED(CONFIG_PCIE_CADENCE_EP))
> > +                       return -ENODEV;
> > +
> > +               ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
> > +               if (!ep)
> > +                       return -ENOMEM;
> > +
> > +               ep->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag;
> > +
> > +               cdns_pcie = &ep->pcie;
> > +               break;
> > +       default:
> > +               dev_err(dev, "INVALID device type %d\n", mode);
> > +               return 0;
> > +       }
> > +
> > +       cdns_pcie->dev = dev;
> > +       cdns_pcie->ops = &j721e_pcie_ops;
> > +
> >         pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> >         if (!pcie)
> >                 return -ENOMEM;
> >
> > +       pcie->cdns_pcie = cdns_pcie;
> >         pcie->mode = mode;
> >         pcie->linkdown_irq_regfield = data->linkdown_irq_regfield;
> >
> > @@ -426,28 +464,6 @@ static int j721e_pcie_probe(struct platform_device *pdev)
> >
> >         switch (mode) {
> >         case PCI_MODE_RC:
> > -               if (!IS_ENABLED(CONFIG_PCIE_CADENCE_HOST)) {
> > -                       ret = -ENODEV;
> > -                       goto err_get_sync;
> > -               }
> > -
> > -               bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc));
> > -               if (!bridge) {
> > -                       ret = -ENOMEM;
> > -                       goto err_get_sync;
> > -               }
> > -
> > -               if (!data->byte_access_allowed)
> > -                       bridge->ops = &cdns_ti_pcie_host_ops;
> > -               rc = pci_host_bridge_priv(bridge);
> > -               rc->quirk_retrain_flag = data->quirk_retrain_flag;
> > -               rc->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag;
> > -
> > -               cdns_pcie = &rc->pcie;
> > -               cdns_pcie->dev = dev;
> > -               cdns_pcie->ops = &j721e_pcie_ops;
> > -               pcie->cdns_pcie = cdns_pcie;
> > -
> >                 gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> >                 if (IS_ERR(gpiod)) {
> >                         ret = PTR_ERR(gpiod);
> > @@ -497,23 +513,6 @@ static int j721e_pcie_probe(struct platform_device *pdev)
> >
> >                 break;
> >         case PCI_MODE_EP:
> > -               if (!IS_ENABLED(CONFIG_PCIE_CADENCE_EP)) {
> > -                       ret = -ENODEV;
> > -                       goto err_get_sync;
> > -               }
> > -
> > -               ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
> > -               if (!ep) {
> > -                       ret = -ENOMEM;
> > -                       goto err_get_sync;
> > -               }
> > -               ep->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag;
> > -
> > -               cdns_pcie = &ep->pcie;
> > -               cdns_pcie->dev = dev;
> > -               cdns_pcie->ops = &j721e_pcie_ops;
> > -               pcie->cdns_pcie = cdns_pcie;
> > -
> >                 ret = cdns_pcie_init_phy(dev, cdns_pcie);
> >                 if (ret) {
> >                         dev_err(dev, "Failed to init phy\n");
> > @@ -525,8 +524,6 @@ static int j721e_pcie_probe(struct platform_device *pdev)
> >                         goto err_pcie_setup;
> >
> >                 break;
> > -       default:
> > -               dev_err(dev, "INVALID device type %d\n", mode);
> >         }
> >
> >         return 0;
> 
> 
> 
> -- 
> greets
> --
> Christian Gmeiner, MSc
> 
> https://christian-gmeiner.info/privacypolicy
diff mbox series

Patch

diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index 489586a4cdc7..cd43d1898482 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -51,10 +51,11 @@  enum link_status {
 #define MAX_LANES			2
 
 struct j721e_pcie {
-	struct cdns_pcie	*cdns_pcie;
+	struct device		*dev;
 	struct clk		*refclk;
 	u32			mode;
 	u32			num_lanes;
+	struct cdns_pcie	*cdns_pcie;
 	void __iomem		*user_cfg_base;
 	void __iomem		*intd_cfg_base;
 	u32			linkdown_irq_regfield;
@@ -98,7 +99,7 @@  static inline void j721e_pcie_intd_writel(struct j721e_pcie *pcie, u32 offset,
 static irqreturn_t j721e_pcie_link_irq_handler(int irq, void *priv)
 {
 	struct j721e_pcie *pcie = priv;
-	struct device *dev = pcie->cdns_pcie->dev;
+	struct device *dev = pcie->dev;
 	u32 reg;
 
 	reg = j721e_pcie_intd_readl(pcie, STATUS_REG_SYS_2);
@@ -164,7 +165,7 @@  static const struct cdns_pcie_ops j721e_pcie_ops = {
 static int j721e_pcie_set_mode(struct j721e_pcie *pcie, struct regmap *syscon,
 			       unsigned int offset)
 {
-	struct device *dev = pcie->cdns_pcie->dev;
+	struct device *dev = pcie->dev;
 	u32 mask = J721E_MODE_RC;
 	u32 mode = pcie->mode;
 	u32 val = 0;
@@ -183,7 +184,7 @@  static int j721e_pcie_set_mode(struct j721e_pcie *pcie, struct regmap *syscon,
 static int j721e_pcie_set_link_speed(struct j721e_pcie *pcie,
 				     struct regmap *syscon, unsigned int offset)
 {
-	struct device *dev = pcie->cdns_pcie->dev;
+	struct device *dev = pcie->dev;
 	struct device_node *np = dev->of_node;
 	int link_speed;
 	u32 val = 0;
@@ -204,7 +205,7 @@  static int j721e_pcie_set_link_speed(struct j721e_pcie *pcie,
 static int j721e_pcie_set_lane_count(struct j721e_pcie *pcie,
 				     struct regmap *syscon, unsigned int offset)
 {
-	struct device *dev = pcie->cdns_pcie->dev;
+	struct device *dev = pcie->dev;
 	u32 lanes = pcie->num_lanes;
 	u32 val = 0;
 	int ret;
@@ -219,7 +220,7 @@  static int j721e_pcie_set_lane_count(struct j721e_pcie *pcie,
 
 static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie)
 {
-	struct device *dev = pcie->cdns_pcie->dev;
+	struct device *dev = pcie->dev;
 	struct device_node *node = dev->of_node;
 	struct of_phandle_args args;
 	unsigned int offset = 0;
@@ -376,6 +377,7 @@  static int j721e_pcie_probe(struct platform_device *pdev)
 	if (!pcie)
 		return -ENOMEM;
 
+	pcie->dev = dev;
 	pcie->mode = mode;
 	pcie->linkdown_irq_regfield = data->linkdown_irq_regfield;