Re: [PATCH for-linus] PCI/bwctrl: Fix NULL pointer deref on unbind and bind

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Jan 02, 2025 at 10:20:35PM +0100, Lukas Wunner wrote:
> The interrupt handler for bandwidth notifications, pcie_bwnotif_irq(),
> dereferences a "data" pointer.
> 
> On unbind, that pointer is set to NULL by pcie_bwnotif_remove().  However
> the interrupt handler may still be invoked afterwards and will dereference
> that NULL pointer.
> 
> That's because the interrupt is requested using a devm_*() helper and the
> driver core releases devm_*() resources *after* calling ->remove().
> 
> pcie_bwnotif_remove() does clear the Link Bandwidth Management Interrupt
> Enable and Link Autonomous Bandwidth Interrupt Enable bits in the Link
> Control Register, but that won't prevent execution of pcie_bwnotif_irq():
> The interrupt for bandwidth notifications may be shared with AER, DPC,
> PME, and hotplug.  So pcie_bwnotif_irq() may be executed as long as the
> interrupt is requested.
> 
> There's a similar race on bind:  pcie_bwnotif_probe() requests the
> interrupt when the "data" pointer still points to NULL.  A NULL pointer
> deref may thus likewise occur if AER, DPC, PME or hotplug raise an
> interrupt in-between the bandwidth controller's call to devm_request_irq()
> and assignment of the "data" pointer.
> 
> Drop the devm_*() usage and reorder requesting of the interrupt to fix the
> issue.
> 
> While at it, drop a stray but harmless no_free_ptr() invocation when
> assigning the "data" pointer in pcie_bwnotif_probe().
> 
> Evert reports a hang on shutdown of an ASUS ROG Strix SCAR 17 G733PYV.
> The issue is no longer reproducible with the present commit.
> 
> Evert found that attaching a USB-C monitor prevented the hang.  The
> machine contains an ASMedia USB 3.2 controller below a hotplug-capable
> Root Port.  So one possible explanation is that the controller gets
> hot-removed on shutdown unless something is connected.  And the ensuing
> hotplug interrupt occurs exactly when the bandwidth controller is
> unregistering.  The precise cause could not be determined because the
> screen had already turned black when the hang occurred.
> 
> Fixes: 665745f27487 ("PCI/bwctrl: Re-add BW notification portdrv as PCIe BW controller")
> Reported-by: Evert Vorster <evorster@gmail.com>
> Tested-by: Evert Vorster <evorster@gmail.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219629
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Applied to pci/for-linus for v6.13, thanks, Evert and Lukas for
working to hard to get this resolved!

> ---
>  drivers/pci/pcie/bwctrl.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/pcie/bwctrl.c b/drivers/pci/pcie/bwctrl.c
> index b59cacc..7cd8211 100644
> --- a/drivers/pci/pcie/bwctrl.c
> +++ b/drivers/pci/pcie/bwctrl.c
> @@ -303,17 +303,18 @@ static int pcie_bwnotif_probe(struct pcie_device *srv)
>  	if (ret)
>  		return ret;
>  
> -	ret = devm_request_irq(&srv->device, srv->irq, pcie_bwnotif_irq,
> -			       IRQF_SHARED, "PCIe bwctrl", srv);
> +	scoped_guard(rwsem_write, &pcie_bwctrl_setspeed_rwsem)
> +		scoped_guard(rwsem_write, &pcie_bwctrl_lbms_rwsem)
> +			port->link_bwctrl = data;
> +
> +	ret = request_irq(srv->irq, pcie_bwnotif_irq, IRQF_SHARED,
> +			  "PCIe bwctrl", srv);
>  	if (ret)
> -		return ret;
> +		goto err_reset_data;
>  
> -	scoped_guard(rwsem_write, &pcie_bwctrl_setspeed_rwsem) {
> -		scoped_guard(rwsem_write, &pcie_bwctrl_lbms_rwsem) {
> -			port->link_bwctrl = no_free_ptr(data);
> +	scoped_guard(rwsem_write, &pcie_bwctrl_setspeed_rwsem)
> +		scoped_guard(rwsem_write, &pcie_bwctrl_lbms_rwsem)
>  			pcie_bwnotif_enable(srv);
> -		}
> -	}
>  
>  	pci_dbg(port, "enabled with IRQ %d\n", srv->irq);
>  
> @@ -323,6 +324,12 @@ static int pcie_bwnotif_probe(struct pcie_device *srv)
>  		port->link_bwctrl->cdev = NULL;
>  
>  	return 0;
> +
> +err_reset_data:
> +	scoped_guard(rwsem_write, &pcie_bwctrl_setspeed_rwsem)
> +		scoped_guard(rwsem_write, &pcie_bwctrl_lbms_rwsem)
> +			port->link_bwctrl = NULL;
> +	return ret;
>  }
>  
>  static void pcie_bwnotif_remove(struct pcie_device *srv)
> @@ -333,6 +340,8 @@ static void pcie_bwnotif_remove(struct pcie_device *srv)
>  
>  	pcie_bwnotif_disable(srv->port);
>  
> +	free_irq(srv->irq, srv);
> +
>  	scoped_guard(rwsem_write, &pcie_bwctrl_setspeed_rwsem)
>  		scoped_guard(rwsem_write, &pcie_bwctrl_lbms_rwsem)
>  			srv->port->link_bwctrl = NULL;
> -- 
> 2.43.0
>