netdev - Re: [Intel-wired-lan] [PATCH iwl-net 1/2] ice: fix NULL access of tx->in_use in ice_ptp_ts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9e72404e-ab66-43dc-8065-1c7008178db6@molgen.mpg.de>
Date: Thu, 7 Aug 2025 23:29:06 +0200
From: Paul Menzel <pmenzel@...gen.mpg.de>
To: Jacob Keller <jacob.e.keller@...el.com>
Cc: Przemek Kitszel <przemyslaw.kitszel@...el.com>,
 intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org
Subject: Re: [Intel-wired-lan] [PATCH iwl-net 1/2] ice: fix NULL access of
 tx->in_use in ice_ptp_ts_irq

Dear Jacob,


Thank you for the patch.

Am 07.08.25 um 19:35 schrieb Jacob Keller:
> The E810 device has support for a "low latency" firmware interface to
> access and read the Tx timestamps. This interface does not use the standard
> Tx timestamp logic, due to the latency overhead of proxying sideband
> command requests over the firmware AdminQ.
> 
> The logic still makes use of the Tx timestamp tracking structure,
> ice_ptp_tx, as it uses the same "ready" bitmap to track which Tx
> timestamps.
> 
> Unfortunately, the ice_ptp_ts_irq() function does not check if the tracker
> is initialized before its first access. This results in NULL dereference or
> use-after-free bugs similar to the following:
> 
> [245977.278756] BUG: kernel NULL pointer dereference, address: 0000000000000000
> [245977.278774] RIP: 0010:_find_first_bit+0x19/0x40
> [245977.278796] Call Trace:
> [245977.278809]  ? ice_misc_intr+0x364/0x380 [ice]
> 
> This can occur if a Tx timestamp interrupt races with the driver reset
> logic.

Do you have a reproducer?

> Fix this by only checking the in_use bitmap (and other fields) if the
> tracker is marked as initialized. The reset flow will clear the init field
> under lock before it tears the tracker down, thus preventing any
> use-after-free or NULL access.

Great commit message. Thank you for taking the time to write this down.

> Fixes: f9472aaabd1f ("ice: Process TSYN IRQ in a separate function")
> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_ptp.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index e358eb1d719f..fb0f6365a6d6 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -2701,16 +2701,19 @@ irqreturn_t ice_ptp_ts_irq(struct ice_pf *pf)
>   		 */
>   		if (hw->dev_caps.ts_dev_info.ts_ll_int_read) {
>   			struct ice_ptp_tx *tx = &pf->ptp.port.tx;
> -			u8 idx;
> +			u8 idx, last;
>   
>   			if (!ice_pf_state_is_nominal(pf))
>   				return IRQ_HANDLED;
>   
>   			spin_lock(&tx->lock);
> -			idx = find_next_bit_wrap(tx->in_use, tx->len,
> -						 tx->last_ll_ts_idx_read + 1);
> -			if (idx != tx->len)
> -				ice_ptp_req_tx_single_tstamp(tx, idx);
> +			if (tx->init) {
> +				last = tx->last_ll_ts_idx_read + 1;
> +				idx = find_next_bit_wrap(tx->in_use, tx->len,
> +							 last);
> +				if (idx != tx->len)
> +					ice_ptp_req_tx_single_tstamp(tx, idx);
> +			}
>   			spin_unlock(&tx->lock);
>   
>   			return IRQ_HANDLED;
> 

Reviewed-by: Paul Menzel <pmenzel@...gen.mpg.de>


Kind regards,

Paul