Message-ID: <CALs4sv1wProin+_J-k3h0Vr_BXcTdrOp1ZezhBiWYh1twFNd9Q@mail.gmail.com>
Date: Sun, 4 Jan 2026 11:50:41 +0530
From: Pavan Chebbi <pavan.chebbi@...adcom.com>
To: Breno Leitao <leitao@...ian.org>
Cc: Michael Chan <michael.chan@...adcom.com>, Andrew Lunn <andrew+netdev@...n.ch>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Richard Cochran <richardcochran@...il.com>, 
	"Russell King (Oracle)" <rmk+kernel@...linux.org.uk>, Vadim Fedorenko <vadim.fedorenko@...ux.dev>, 
	Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, kernel-team@...a.com, stable@...r.kernel.org
Subject: Re: [PATCH net] bnxt_en: Fix NULL pointer crash in bnxt_ptp_enable
 during error cleanup

On Fri, Jan 2, 2026 at 9:13 PM Breno Leitao <leitao@...ian.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@...ian.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@...ian.org>
> > > Fixes: a60fc3294a37 ("ptp: rework ptp_clock_unregister() to disable events")
> > > Cc: stable@...r.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@...ian.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@...adcom.com>
>     Signed-off-by: Breno Leitao <leitao@...ian.org>
>     Fixes: a60fc3294a37 ("ptp: rework ptp_clock_unregister() to disable events")
>     Cc: stable@...r.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);

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5469 bytes)

Powered by blists - more mailing lists