Re: [Intel-wired-lan] [PATCH v1 iwl-net] ice: fix potential NULL pointer deref in error path of ice_set_ringparam()

[Kohei Enju <kohei@enjuk.jp>]

On Fri, 20 Feb 2026 20:04:52 +0000, "Loktionov, Aleksandr" wrote:

> > -----Original Message-----
> > From: Kohei Enju <kohei@enjuk.jp>
> > Sent: Friday, February 20, 2026 7:40 PM
> > To: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org
> > Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
> > Przemyslaw <przemyslaw.kitszel@intel.com>; Andrew Lunn
> > <andrew+netdev@lunn.ch>; David S. Miller <davem@davemloft.net>; Eric
> > Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> > Abeni <pabeni@redhat.com>; Loktionov, Aleksandr
> > <aleksandr.loktionov@intel.com>; Alice Michael
> > <alice.michael@intel.com>; Greenwalt, Paul <paul.greenwalt@intel.com>;
> > Fijalkowski, Maciej <maciej.fijalkowski@intel.com>;
> > kohei.enju@gmail.com; Kohei Enju <kohei@enjuk.jp>
> > Subject: [PATCH v1 iwl-net] ice: fix potential NULL pointer deref in
> > error path of ice_set_ringparam()
> >=20
> > ice_set_ringparam nullifies tstamp_ring of temporary tx_rings, without
> > clearing ICE_TX_RING_FLAGS_TXTIME bit.
> > When ICE_TX_RING_FLAGS_TXTIME is set and the subsequent
> > ice_setup_tx_ring() call fails, a NULL pointer dereference could
> > happen in the unwinding sequence:
> >=20
> > ice_clean_tx_ring()
> > -> ice_is_txtime_cfg() =3D=3D true (ICE_TX_RING_FLAGS_TXTIME is set)
> > -> ice_free_tx_tstamp_ring()
> >   -> ice_free_tstamp_ring()
> >     -> tstamp_ring->desc (NULL deref)
> >=20
> > Clear ICE_TX_RING_FLAGS_TXTIME bit to avoid the potential issue.
> >=20
> > Note that this potential issue is found by manual code review.
> > Compile test only since unfortunately I don't have E830 devices.
> >=20
> > Fixes: ccde82e90946 ("ice: add E830 Earliest TxTime First Offload
> > support")
> If it's a fix, shouldn't it go to net?

This fix relies on a commit 8a4e78094945 ("ice: fix race condition in TX
timestamp ring cleanup"), which changed type of flags from u8 to
BITMAP, as you advised in [1].
Since the commit only exists in Tony's tree, I chose iwl-net, not net.

[1] https://lore.kernel.org/intel-wired-lan/IA3PR11MB8986EB459D2FD1697644CF98E598A@IA3PR11MB8986.namprd11.prod.outlook.com/

> 
> > Signed-off-by: Kohei Enju <kohei@enjuk.jp>
> > ---
> >  drivers/net/ethernet/intel/ice/ice_ethtool.c | 1 +
> >  1 file changed, 1 insertion(+)
> >=20
> > diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> > b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> > index 7f769a90dde1..5ed86648d0d6 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> > @@ -3290,6 +3290,7 @@ ice_set_ringparam(struct net_device *netdev,
> > struct ethtool_ringparam *ring,
> >  		tx_rings[i].desc =3D NULL;
> >  		tx_rings[i].tx_buf =3D NULL;
> >  		tx_rings[i].tstamp_ring =3D NULL;
> > +		clear_bit(ICE_TX_RING_FLAGS_TXTIME, tx_rings[i].flags);
> >  		tx_rings[i].tx_tstamps =3D &pf->ptp.port.tx;
> >  		err =3D ice_setup_tx_ring(&tx_rings[i]);
> If ice_setup_tx_ring() internally reads ICE_TX_RING_FLAGS_TXTIME to decide =
> whether to allocate the tstamp_ring, then clearing the bit first means ice_=
> setup_tx_ring() skips TxTime setup even on success - leaving TxTime silentl=
> y broken after ice_set_ringparam() completes normally. The crash is fixed o=
> n the error path, but I'm afraid a functional regression is introduced on t=
> he success path.
> Please correct me if I'm wrong.

I believe, in the successful path, tx_tstamps and the flag
ICE_TX_RING_FLAGS_TXTIME are restored in ice_up().

ice_up()
 ice_vsi_cfg_lan()
  ice_vsi_cfg_lan_txqs()
   ice_vsi_cfg_txqs()
    ice_vsi_cfg_txq()
     ice_is_txtime_ena() ( == test_bit(ring->q_index,  pf->txtime_txqs))
     ice_alloc_setup_tstamp_ring()
      ice_alloc_tstamp_ring()
       - allocate tx_tstamps
       - set_bit(ICE_TX_RING_FLAGS_TXTIME, tx_ring->flags);

Enablement of TxTime feature is managed by pf->txtime_txqs, and
ring->flags represents only a 'result' of the enablement.

Therefore I think it's not a problem, but please let me know if my
understanding is wrong.
Thank you for reviewing, Alex!

> 
> Thank you
> Alex
> 
> >  		if (err) {
> > --
> > 2.51.0