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

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

[Kohei Enju]

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:

ice_clean_tx_ring()
-> ice_is_txtime_cfg() == true (ICE_TX_RING_FLAGS_TXTIME is set)
-> ice_free_tx_tstamp_ring()
  -> ice_free_tstamp_ring()
    -> tstamp_ring->desc (NULL deref)

Clear ICE_TX_RING_FLAGS_TXTIME bit to avoid the potential issue.

Note that this potential issue is found by manual code review.
Compile test only since unfortunately I don't have E830 devices.

Fixes: ccde82e90946 ("ice: add E830 Earliest TxTime First Offload support")
Signed-off-by: Kohei Enju <kohei@enjuk.jp>
---
 drivers/net/ethernet/intel/ice/ice_ethtool.c | 1 +
 1 file changed, 1 insertion(+)

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 = NULL;
 		tx_rings[i].tx_buf = NULL;
 		tx_rings[i].tstamp_ring = NULL;
+		clear_bit(ICE_TX_RING_FLAGS_TXTIME, tx_rings[i].flags);
 		tx_rings[i].tx_tstamps = &pf->ptp.port.tx;
 		err = ice_setup_tx_ring(&tx_rings[i]);
 		if (err) {
-- 
2.51.0

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

[Loktionov, Aleksandr]

> -----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()
> 
> 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:
> 
> ice_clean_tx_ring()
> -> ice_is_txtime_cfg() == true (ICE_TX_RING_FLAGS_TXTIME is set)
> -> ice_free_tx_tstamp_ring()
>   -> ice_free_tstamp_ring()
>     -> tstamp_ring->desc (NULL deref)
> 
> Clear ICE_TX_RING_FLAGS_TXTIME bit to avoid the potential issue.
> 
> Note that this potential issue is found by manual code review.
> Compile test only since unfortunately I don't have E830 devices.
> 
> Fixes: ccde82e90946 ("ice: add E830 Earliest TxTime First Offload
> support")
If it's a fix, shouldn't it go to net?

> Signed-off-by: Kohei Enju <kohei@enjuk.jp>
> ---
>  drivers/net/ethernet/intel/ice/ice_ethtool.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> 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 = NULL;
>  		tx_rings[i].tx_buf = NULL;
>  		tx_rings[i].tstamp_ring = NULL;
> +		clear_bit(ICE_TX_RING_FLAGS_TXTIME, tx_rings[i].flags);
>  		tx_rings[i].tx_tstamps = &pf->ptp.port.tx;
>  		err = 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 silently broken after ice_set_ringparam() completes normally. The crash is fixed on the error path, but I'm afraid a functional regression is introduced on the success path.
Please correct me if I'm wrong.

Thank you
Alex

>  		if (err) {
> --
> 2.51.0

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

[Kohei Enju]

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

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

[Greenwalt, Paul]

On 2/20/2026 12:04 PM, 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()
>>
>> 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:
>>
>> ice_clean_tx_ring()
>> -> ice_is_txtime_cfg() == true (ICE_TX_RING_FLAGS_TXTIME is set)
>> -> ice_free_tx_tstamp_ring()
>>   -> ice_free_tstamp_ring()
>>     -> tstamp_ring->desc (NULL deref)
>>
>> Clear ICE_TX_RING_FLAGS_TXTIME bit to avoid the potential issue.
>>
>> Note that this potential issue is found by manual code review.
>> Compile test only since unfortunately I don't have E830 devices.
>>
>> Fixes: ccde82e90946 ("ice: add E830 Earliest TxTime First Offload
>> support")
> If it's a fix, shouldn't it go to net?
> 

Thank you for identifying this bug. However, clearing
ICE_TX_RING_FLAGS_TXTIME before ice_setup_tx_ring() will break TxTime
offload on the success path.

ice_setup_tx_ring() doesn't read this flag, but ice_up() (called later)
needs it set to reallocate the tstamp ring for the new ring size. Your
change would silently disable TxTime after any successful ring parameter
change.

The fix should clear the flag only in the error path:

if (err) {
	while (i--) {
		clear_bit(ICE_TX_RING_FLAGS_TXTIME, tx_rings[i].flags);
		ice_clean_tx_ring(&tx_rings[i]);
	}
	vfree(tx_rings);
	goto done;
}

Also, per Alex's comment, please target 'net'.

Thanks,
Paul

>> Signed-off-by: Kohei Enju <kohei@enjuk.jp>
>> ---
>>  drivers/net/ethernet/intel/ice/ice_ethtool.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> 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 = NULL;
>>  		tx_rings[i].tx_buf = NULL;
>>  		tx_rings[i].tstamp_ring = NULL;
>> +		clear_bit(ICE_TX_RING_FLAGS_TXTIME, tx_rings[i].flags);
>>  		tx_rings[i].tx_tstamps = &pf->ptp.port.tx;
>>  		err = 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 silently broken after ice_set_ringparam() completes normally. The crash is fixed on the error path, but I'm afraid a functional regression is introduced on the success path.
> Please correct me if I'm wrong.
> 
> Thank you
> Alex
> 
>>  		if (err) {
>> --
>> 2.51.0
> 

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

[Tony Nguyen]

On 2/20/2026 4:35 PM, Greenwalt, Paul wrote:
> 
> 
> On 2/20/2026 12:04 PM, Loktionov, Aleksandr wrote:

...

> Also, per Alex's comment, please target 'net'.

Since this is for an Intel driver, iwl-net is the correct target.

Also, as was responded, this works off changes that are only on 
net-queue at the moment.

https://lore.kernel.org/intel-wired-lan/20260220204526.7514-1-kohei@enjuk.jp/

Thanks,
Tony

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

[Greenwalt, Paul]

On 2/20/2026 5:50 PM, Tony Nguyen wrote:
> 
> 
> On 2/20/2026 4:35 PM, Greenwalt, Paul wrote:
>>
>>
>> On 2/20/2026 12:04 PM, Loktionov, Aleksandr wrote:
> 
> ...
> 
>> Also, per Alex's comment, please target 'net'.
> 
> Since this is for an Intel driver, iwl-net is the correct target.
> 
> Also, as was responded, this works off changes that are only on net-
> queue at the moment.
> 
> https://lore.kernel.org/intel-wired-lan/20260220204526.7514-1-
> kohei@enjuk.jp/
> 

Thanks for the clarification Kohei and Tony.

Reviewed-by: Paul Greenwalt <paul.greenwalt@intel.com>

> Thanks,
> Tony