[PATCH net] bnxt_en: Fix NULL pointer crash in bnxt_ptp_enable during error cleanup

Breno Leitao posted 1 patch 1 month, 1 week ago
There is a newer version of this series
drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 7 +++++++
1 file changed, 7 insertions(+)
[PATCH net] bnxt_en: Fix NULL pointer crash in bnxt_ptp_enable during error cleanup
Posted by Breno Leitao 1 month, 1 week ago
When bnxt_init_one() fails during initialization (e.g.,
bnxt_init_int_mode returns -ENODEV), the error path calls
bnxt_free_hwrm_resources() which destroys the DMA pool and sets
bp->hwrm_dma_pool to NULL. Subsequently, bnxt_ptp_clear() is called,
which invokes ptp_clock_unregister().

Since commit a60fc3294a37 ("ptp: rework ptp_clock_unregister() to
disable events"), ptp_clock_unregister() now calls
ptp_disable_all_events(), which in turn invokes the driver's .enable()
callback (bnxt_ptp_enable()) to disable PTP events before completing the
unregistration.

bnxt_ptp_enable() attempts to send HWRM commands via bnxt_ptp_cfg_pin()
and bnxt_ptp_cfg_event(), both of which call hwrm_req_init(). This
function tries to allocate from bp->hwrm_dma_pool, causing a NULL
pointer dereference:

  bnxt_en 0000:01:00.0 (unnamed net_device) (uninitialized): bnxt_init_int_mode err: ffffffed
  KASAN: null-ptr-deref in range [0x0000000000000028-0x000000000000002f]
  Call Trace:
   __hwrm_req_init (drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c:72)
   bnxt_ptp_enable (drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:323 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:517)
   ptp_disable_all_events (drivers/ptp/ptp_chardev.c:66)
   ptp_clock_unregister (drivers/ptp/ptp_clock.c:518)
   bnxt_ptp_clear (drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:1134)
   bnxt_init_one (drivers/net/ethernet/broadcom/bnxt/bnxt.c:16889)

Lines are against commit f8f9c1f4d0c7 ("Linux 6.19-rc3")

Fix this by checking if bp->hwrm_dma_pool is NULL at the start of
bnxt_ptp_enable(). During error/cleanup paths when HWRM resources have
been freed, return success without attempting to send commands since the
hardware is being torn down anyway.

During normal operation, the DMA pool is always valid so PTP
functionality is unaffected.

Signed-off-by: Breno Leitao <leitao@debian.org>
Fixes: a60fc3294a37 ("ptp: rework ptp_clock_unregister() to disable events")
Cc: stable@vger.kernel.org
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
index a8a74f07bb54..a749bbfa398e 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
@@ -482,6 +482,13 @@ static int bnxt_ptp_enable(struct ptp_clock_info *ptp_info,
 	int pin_id;
 	int rc;
 
+	/* Return success if HWRM resources are not available.
+	 * This can happen during error/cleanup paths when DMA pool has been
+	 * freed.
+	 */
+	if (!bp->hwrm_dma_pool)
+		return 0;
+
 	switch (rq->type) {
 	case PTP_CLK_REQ_EXTTS:
 		/* Configure an External PPS IN */

---
base-commit: 80380f6ce46f38a0d1200caade2f03de4b6c1d27
change-id: 20251231-bnxt-c54d317d8bfe

Best regards,
--  
Breno Leitao <leitao@debian.org>
Re: [PATCH net] bnxt_en: Fix NULL pointer crash in bnxt_ptp_enable during error cleanup
Posted by Pavan Chebbi 1 month, 1 week ago
On Wed, Dec 31, 2025 at 6:35 PM Breno Leitao <leitao@debian.org> wrote:
>
> When bnxt_init_one() fails during initialization (e.g.,
> bnxt_init_int_mode returns -ENODEV), the error path calls
> bnxt_free_hwrm_resources() which destroys the DMA pool and sets
> bp->hwrm_dma_pool to NULL. Subsequently, bnxt_ptp_clear() is called,
> which invokes ptp_clock_unregister().
>
> Since commit a60fc3294a37 ("ptp: rework ptp_clock_unregister() to
> disable events"), ptp_clock_unregister() now calls
> ptp_disable_all_events(), which in turn invokes the driver's .enable()
> callback (bnxt_ptp_enable()) to disable PTP events before completing the
> unregistration.
>
> bnxt_ptp_enable() attempts to send HWRM commands via bnxt_ptp_cfg_pin()
> and bnxt_ptp_cfg_event(), both of which call hwrm_req_init(). This
> function tries to allocate from bp->hwrm_dma_pool, causing a NULL
> pointer dereference:
>
>   bnxt_en 0000:01:00.0 (unnamed net_device) (uninitialized): bnxt_init_int_mode err: ffffffed
>   KASAN: null-ptr-deref in range [0x0000000000000028-0x000000000000002f]
>   Call Trace:
>    __hwrm_req_init (drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c:72)
>    bnxt_ptp_enable (drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:323 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:517)
>    ptp_disable_all_events (drivers/ptp/ptp_chardev.c:66)
>    ptp_clock_unregister (drivers/ptp/ptp_clock.c:518)
>    bnxt_ptp_clear (drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:1134)
>    bnxt_init_one (drivers/net/ethernet/broadcom/bnxt/bnxt.c:16889)
>
> Lines are against commit f8f9c1f4d0c7 ("Linux 6.19-rc3")
>
> Fix this by checking if bp->hwrm_dma_pool is NULL at the start of
> bnxt_ptp_enable(). During error/cleanup paths when HWRM resources have
> been freed, return success without attempting to send commands since the
> hardware is being torn down anyway.
>
> During normal operation, the DMA pool is always valid so PTP
> functionality is unaffected.
>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> Fixes: a60fc3294a37 ("ptp: rework ptp_clock_unregister() to disable events")
> Cc: stable@vger.kernel.org
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> index a8a74f07bb54..a749bbfa398e 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> @@ -482,6 +482,13 @@ static int bnxt_ptp_enable(struct ptp_clock_info *ptp_info,
>         int pin_id;
>         int rc;
>
> +       /* Return success if HWRM resources are not available.
> +        * This can happen during error/cleanup paths when DMA pool has been
> +        * freed.
> +        */
> +       if (!bp->hwrm_dma_pool)

Thanks for the fix. While it's valid, just that to me, this check here
looks a bit odd.
Why not call bnxt_ptp_clear() before bnxt_free_hwrm_resources() in the
unwind path?

> +               return 0;
> +
>         switch (rq->type) {
>         case PTP_CLK_REQ_EXTTS:
>                 /* Configure an External PPS IN */
>
> ---
> base-commit: 80380f6ce46f38a0d1200caade2f03de4b6c1d27
> change-id: 20251231-bnxt-c54d317d8bfe
>
> Best regards,
> --
> Breno Leitao <leitao@debian.org>
>
Re: [PATCH net] bnxt_en: Fix NULL pointer crash in bnxt_ptp_enable during error cleanup
Posted by Breno Leitao 1 month ago
Hello Pavan,

On Wed, Dec 31, 2025 at 09:30:57PM +0530, Pavan Chebbi wrote:
> On Wed, Dec 31, 2025 at 6:35 PM Breno Leitao <leitao@debian.org> wrote:
> > Fix this by checking if bp->hwrm_dma_pool is NULL at the start of
> > bnxt_ptp_enable(). During error/cleanup paths when HWRM resources have
> > been freed, return success without attempting to send commands since the
> > hardware is being torn down anyway.
> >
> > During normal operation, the DMA pool is always valid so PTP
> > functionality is unaffected.
> >
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> > Fixes: a60fc3294a37 ("ptp: rework ptp_clock_unregister() to disable events")
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> > index a8a74f07bb54..a749bbfa398e 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> > @@ -482,6 +482,13 @@ static int bnxt_ptp_enable(struct ptp_clock_info *ptp_info,
> >         int pin_id;
> >         int rc;
> >
> > +       /* Return success if HWRM resources are not available.
> > +        * This can happen during error/cleanup paths when DMA pool has been
> > +        * freed.
> > +        */
> > +       if (!bp->hwrm_dma_pool)
> 
> Thanks for the fix. While it's valid, just that to me, this check here
> looks a bit odd.
> Why not call bnxt_ptp_clear() before bnxt_free_hwrm_resources() in the
> unwind path?

I thought about it, but, I didn't understand all the implication of
changing the unwind order. 

Anyway, I've have tested the current patch and it worked fine. Do you
think we should move kfree(bp->ptp_cfg) closer to bnxt_ptp_clear()?

Thanks for the review,
--breno


commit d07c08889f75966d6829b93304de5030cf4e66aa
Author: Breno Leitao <leitao@debian.org>
Date:   Wed Dec 31 04:00:57 2025 -0800

    bnxt_en: Fix NULL pointer crash in bnxt_ptp_enable during error cleanup
    
    When bnxt_init_one() fails during initialization (e.g.,
    bnxt_init_int_mode returns -ENODEV), the error path calls
    bnxt_free_hwrm_resources() which destroys the DMA pool and sets
    bp->hwrm_dma_pool to NULL. Subsequently, bnxt_ptp_clear() is called,
    which invokes ptp_clock_unregister().
    
    Since commit a60fc3294a37 ("ptp: rework ptp_clock_unregister() to
    disable events"), ptp_clock_unregister() now calls
    ptp_disable_all_events(), which in turn invokes the driver's .enable()
    callback (bnxt_ptp_enable()) to disable PTP events before completing the
    unregistration.
    
    bnxt_ptp_enable() attempts to send HWRM commands via bnxt_ptp_cfg_pin()
    and bnxt_ptp_cfg_event(), both of which call hwrm_req_init(). This
    function tries to allocate from bp->hwrm_dma_pool, causing a NULL
    pointer dereference:
    
      bnxt_en 0000:01:00.0 (unnamed net_device) (uninitialized): bnxt_init_int_mode err: ffffffed
      KASAN: null-ptr-deref in range [0x0000000000000028-0x000000000000002f]
      Call Trace:
       __hwrm_req_init (drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c:72)
       bnxt_ptp_enable (drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:323 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:517)
       ptp_disable_all_events (drivers/ptp/ptp_chardev.c:66)
       ptp_clock_unregister (drivers/ptp/ptp_clock.c:518)
       bnxt_ptp_clear (drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:1134)
       bnxt_init_one (drivers/net/ethernet/broadcom/bnxt/bnxt.c:16889)
    
    Lines are against commit f8f9c1f4d0c7 ("Linux 6.19-rc3")
    
    Fix this by clearing and unregistering ptp (bnxt_ptp_clear()) before
    freeing HWRM resources.
    
    Suggested-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
    Signed-off-by: Breno Leitao <leitao@debian.org>
    Fixes: a60fc3294a37 ("ptp: rework ptp_clock_unregister() to disable events")
    Cc: stable@vger.kernel.org

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index d17d0ea89c36..68fc9977b375 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -16882,10 +16882,10 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 init_err_pci_clean:
 	bnxt_hwrm_func_drv_unrgtr(bp);
+	bnxt_ptp_clear(bp);
 	bnxt_free_hwrm_resources(bp);
 	bnxt_hwmon_uninit(bp);
 	bnxt_ethtool_free(bp);
-	bnxt_ptp_clear(bp);
 	kfree(bp->ptp_cfg);
 	bp->ptp_cfg = NULL;
 	kfree(bp->fw_health);
Re: [PATCH net] bnxt_en: Fix NULL pointer crash in bnxt_ptp_enable during error cleanup
Posted by Pavan Chebbi 1 month ago
On Fri, Jan 2, 2026 at 9:13 PM Breno Leitao <leitao@debian.org> wrote:
>
> Hello Pavan,
>
> On Wed, Dec 31, 2025 at 09:30:57PM +0530, Pavan Chebbi wrote:
> > On Wed, Dec 31, 2025 at 6:35 PM Breno Leitao <leitao@debian.org> wrote:
> > > Fix this by checking if bp->hwrm_dma_pool is NULL at the start of
> > > bnxt_ptp_enable(). During error/cleanup paths when HWRM resources have
> > > been freed, return success without attempting to send commands since the
> > > hardware is being torn down anyway.
> > >
> > > During normal operation, the DMA pool is always valid so PTP
> > > functionality is unaffected.
> > >
> > > Signed-off-by: Breno Leitao <leitao@debian.org>
> > > Fixes: a60fc3294a37 ("ptp: rework ptp_clock_unregister() to disable events")
> > > Cc: stable@vger.kernel.org
> > > ---
> > >  drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> > > index a8a74f07bb54..a749bbfa398e 100644
> > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> > > @@ -482,6 +482,13 @@ static int bnxt_ptp_enable(struct ptp_clock_info *ptp_info,
> > >         int pin_id;
> > >         int rc;
> > >
> > > +       /* Return success if HWRM resources are not available.
> > > +        * This can happen during error/cleanup paths when DMA pool has been
> > > +        * freed.
> > > +        */
> > > +       if (!bp->hwrm_dma_pool)
> >
> > Thanks for the fix. While it's valid, just that to me, this check here
> > looks a bit odd.
> > Why not call bnxt_ptp_clear() before bnxt_free_hwrm_resources() in the
> > unwind path?
>
> I thought about it, but, I didn't understand all the implication of
> changing the unwind order.

It looks safe, it should just be fine.

>
> Anyway, I've have tested the current patch and it worked fine. Do you
> think we should move kfree(bp->ptp_cfg) closer to bnxt_ptp_clear()?

Yes, that looks like a right thing to do, along with bp->ptp_cfg = NULL.

>
> Thanks for the review,
> --breno
>
>
> commit d07c08889f75966d6829b93304de5030cf4e66aa
> Author: Breno Leitao <leitao@debian.org>
> Date:   Wed Dec 31 04:00:57 2025 -0800
>
>     bnxt_en: Fix NULL pointer crash in bnxt_ptp_enable during error cleanup
>
>     When bnxt_init_one() fails during initialization (e.g.,
>     bnxt_init_int_mode returns -ENODEV), the error path calls
>     bnxt_free_hwrm_resources() which destroys the DMA pool and sets
>     bp->hwrm_dma_pool to NULL. Subsequently, bnxt_ptp_clear() is called,
>     which invokes ptp_clock_unregister().
>
>     Since commit a60fc3294a37 ("ptp: rework ptp_clock_unregister() to
>     disable events"), ptp_clock_unregister() now calls
>     ptp_disable_all_events(), which in turn invokes the driver's .enable()
>     callback (bnxt_ptp_enable()) to disable PTP events before completing the
>     unregistration.
>
>     bnxt_ptp_enable() attempts to send HWRM commands via bnxt_ptp_cfg_pin()
>     and bnxt_ptp_cfg_event(), both of which call hwrm_req_init(). This
>     function tries to allocate from bp->hwrm_dma_pool, causing a NULL
>     pointer dereference:
>
>       bnxt_en 0000:01:00.0 (unnamed net_device) (uninitialized): bnxt_init_int_mode err: ffffffed
>       KASAN: null-ptr-deref in range [0x0000000000000028-0x000000000000002f]
>       Call Trace:
>        __hwrm_req_init (drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c:72)
>        bnxt_ptp_enable (drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:323 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:517)
>        ptp_disable_all_events (drivers/ptp/ptp_chardev.c:66)
>        ptp_clock_unregister (drivers/ptp/ptp_clock.c:518)
>        bnxt_ptp_clear (drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:1134)
>        bnxt_init_one (drivers/net/ethernet/broadcom/bnxt/bnxt.c:16889)
>
>     Lines are against commit f8f9c1f4d0c7 ("Linux 6.19-rc3")
>
>     Fix this by clearing and unregistering ptp (bnxt_ptp_clear()) before
>     freeing HWRM resources.
>
>     Suggested-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
>     Signed-off-by: Breno Leitao <leitao@debian.org>
>     Fixes: a60fc3294a37 ("ptp: rework ptp_clock_unregister() to disable events")
>     Cc: stable@vger.kernel.org
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index d17d0ea89c36..68fc9977b375 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -16882,10 +16882,10 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>
>  init_err_pci_clean:
>         bnxt_hwrm_func_drv_unrgtr(bp);
> +       bnxt_ptp_clear(bp);
>         bnxt_free_hwrm_resources(bp);
>         bnxt_hwmon_uninit(bp);
>         bnxt_ethtool_free(bp);
> -       bnxt_ptp_clear(bp);
>         kfree(bp->ptp_cfg);
>         bp->ptp_cfg = NULL;
>         kfree(bp->fw_health);