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 <[email protected]> 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/[email protected]/ > 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/[email protected]?part=1
