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

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

quoted

-----Original Message-----
From: Kohei Enju <redacted>
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 [off-list ref]; Andrew Lunn
[off-list ref]; David S. Miller [off-list ref]; Eric
Dumazet [off-list ref]; Jakub Kicinski [off-list ref]; Paolo
Abeni [off-list ref]; Loktionov, Aleksandr
[off-list ref]; Alice Michael
[off-list ref]; Greenwalt, Paul [off-list ref];
Fijalkowski, Maciej [off-list ref];
kohei.enju@gmail.com; Kohei Enju [off-list ref]
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/ (local)

quoted

Signed-off-by: Kohei Enju <redacted>
---
 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

quoted

 		if (err) {
--
2.51.0