Re: [PATCH net] ice: fix Tx hang dump using stale tx_buf[0].skb

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] net/ice: TOCTOU race in ice_tx_hang_reporter_dump() causing SKB UAF
- [High] ice: out-of-bounds access in ice_tx_hang_reporter_dump() during ring resize
- [High] devlink: NULL pointer deref in ice_tx_hang_reporter_dump on teardown
--

commit 9b2375ee849517a745f8e8f71365535e40788bc1
Author: Vincent Choiniere <choiniere.vincent@hydroquebec.com>

ice: fix Tx hang dump using stale tx_buf[0].skb

This commit updates the ice devlink health reporter to read the SKB from the
Tx buffer at the next_to_clean index instead of slot 0. It also adds a type
check to ensure the buffer contains an SKB before dumping it.

Link: https://lore.kernel.org/all/20260327072332.130320-1-aleksandr.loktionov@intel.com/

> diff --git a/drivers/net/ethernet/intel/ice/devlink/health.c b/drivers/net/ethernet/intel/ice/devlink/health.c
> index 8e9a8a8178d48..059859c36dfb2 100644
> --- a/drivers/net/ethernet/intel/ice/devlink/health.c
> +++ b/drivers/net/ethernet/intel/ice/devlink/health.c
> @@ -390,12 +390,15 @@ static int ice_tx_hang_reporter_dump(struct devlink_health_reporter *reporter,
>  				     struct netlink_ext_ack *extack)
>  {
>  	struct ice_tx_hang_event *event = priv_ctx;
> -	struct sk_buff *skb;
> +	struct sk_buff *skb = NULL;
> +	struct ice_tx_buf *tx_buf;
>  
>  	if (!event)
>  		return 0;
>  
> -	skb = event->tx_ring->tx_buf->skb;
> +	tx_buf = &event->tx_ring->tx_buf[event->next_to_clean];

[Severity: High]
Can event->tx_ring->tx_buf be NULL here?

If the interface is concurrently brought down, ice_free_tx_ring() frees the
tx_buf array and sets tx_ring->tx_buf to NULL.

Computing the address with the index offset and then evaluating tx_buf->type
below would trigger a page fault.

[Severity: High]
Could an out-of-bounds access occur if the ring is concurrently resized?

If a user runs ethtool -G to shrink the ring:

CPU1 (ethtool)
ice_set_ringparam()
    ice_free_tx_ring(vsi->tx_rings[i]);
    *vsi->tx_rings[i] = tx_rings[i];

CPU2 (Workqueue)
ice_tx_hang_reporter_dump()
    tx_buf = &event->tx_ring->tx_buf[event->next_to_clean];

Since event->next_to_clean is cached from the timeout event, couldn't it
exceed the bounds of the newly allocated, smaller tx_buf array?

> +	if (tx_buf->type == ICE_TX_BUF_SKB)
> +		skb = tx_buf->skb;

[Severity: High]
Could this TOCTOU race happen with NAPI?

If this dump runs in a workqueue without disabling NAPI or taking a Tx queue
lock:

CPU1 (NAPI)
ice_clean_tx_irq()
    napi_consume_skb(tx_buf->skb, napi_budget);
    ...
    tx_buf->type = ICE_TX_BUF_EMPTY;

CPU2 (Workqueue)
ice_tx_hang_reporter_dump()
    if (tx_buf->type == ICE_TX_BUF_SKB)
        skb = tx_buf->skb;
    ...
    if (skb)
        devlink_fmsg_dump_skb(fmsg, skb);

If CPU1 frees the SKB after CPU2 performs the type check, doesn't
devlink_fmsg_dump_skb() end up dereferencing a freed pointer?

>  	devlink_fmsg_obj_nest_start(fmsg);
>  	ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, event, head);
>  	ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, event, intr);

[ ... ]

> @@ -409,7 +412,8 @@ static int ice_tx_hang_reporter_dump(struct devlink_health_reporter *reporter,
>  	ice_fmsg_put_ptr(fmsg, "skb-ptr", skb);
>  	devlink_fmsg_binary_pair_put(fmsg, "desc", event->tx_ring->desc,
>  				     event->tx_ring->count * sizeof(struct ice_tx_desc));

[Severity: High]
Could event->tx_ring->desc also be NULL here if the ring was concurrently
freed?

Since ice_free_tx_ring() doesn't clear tx_ring->count, wouldn't
devlink_fmsg_binary_pair_put() attempt to copy from a NULL desc pointer?

> -	devlink_fmsg_dump_skb(fmsg, skb);
> +	if (skb)
> +		devlink_fmsg_dump_skb(fmsg, skb);
>  	devlink_fmsg_obj_nest_end(fmsg);
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016-ice-tx-hang-dump-v1-1-3df0f5979a51@hydroquebec.com?part=1