Re: [PATCH] RDMA/irdma: fix double free of close_entry in TIME_WAIT teardown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] RDMA/irdma: refcount underflow and UAF in irdma_cm_timer_tick()
--

commit 3abe5e40541e0bcf80a0a51f21ea66e768ed3bae
Author: Wentao Liang <vulab@iscas.ac.cn>

RDMA/irdma: fix double free of close_entry in TIME_WAIT teardown

This commit reorders freeing close_entry and clearing cm_node->close_entry
before calling irdma_rem_ref_cm_node() in irdma_handle_close_entry() to avoid
a double free if dropping the cm_node reference triggers
irdma_destroy_connection().

> diff --git a/drivers/infiniband/hw/irdma/cm.c b/drivers/infiniband/hw/irdma/cm.c
> index 91c0e72982836..d8b8939d6abdb 100644
> --- a/drivers/infiniband/hw/irdma/cm.c
> +++ b/drivers/infiniband/hw/irdma/cm.c
> @@ -1243,13 +1243,15 @@ static void irdma_handle_close_entry(struct irdma_cm_node *cm_node,
>  		} else {
>  			spin_unlock_irqrestore(&iwqp->lock, flags);
>  		}
> -	} else if (rem_node) {
> -		/* TIME_WAIT state */
> -		irdma_rem_ref_cm_node(cm_node);
>  	}
>  
>  	kfree(close_entry);
>  	cm_node->close_entry = NULL;
> +
> +	if (!iwqp && rem_node) {
> +		/* TIME_WAIT state */
> +		irdma_rem_ref_cm_node(cm_node);
> +	}
>  }

[Severity: Critical]
If irdma_rem_ref_cm_node(cm_node) here drops the last reference on cm_node,
does the caller irdma_cm_timer_tick() suffer a use-after-free on cm_node and
underflow cm_node->refcnt?

irdma_handle_close_entry() has only two callers: irdma_destroy_connection()
passes rem_node = 0, and irdma_cm_timer_tick() passes rem_node = 1.

Before iterating timer_list, irdma_cm_timer_tick() calls
irdma_timer_list_prep(), which takes a dedicated reference on each queued
cm_node for timer_list:

drivers/infiniband/hw/irdma/cm.c:irdma_timer_list_prep() {
	...
		if ((cm_node->close_entry || cm_node->send_entry) &&
		    refcount_inc_not_zero(&cm_node->refcnt))
			list_add(&cm_node->timer_entry, timer_list);
	...
}

In normal non-racing TIME_WAIT expiration, cm_node->refcnt is at least 2 when
entering irdma_handle_close_entry(cm_node, 1) (one for the connection in
IRDMA_CM_STATE_TIME_WAIT and one for timer_list), so the put in
irdma_handle_close_entry() drops refcnt from 2 to 1 and is not the final put.

The put in irdma_handle_close_entry(cm_node, 1) only becomes the final put
(reducing refcnt from 1 to 0 and calling irdma_destroy_connection()) when a
concurrent FIN or RST packet in irdma_handle_fin_pkt() or
irdma_handle_rst_pkt() processes IRDMA_CM_STATE_TIME_WAIT and drops the
TIME_WAIT reference without clearing cm_node->close_entry while cm_node is on
timer_list (or when irdma_handle_close_entry() drops the TIME_WAIT reference
without changing cm_node->state out of IRDMA_CM_STATE_TIME_WAIT before a
concurrent FIN or RST arrives):

drivers/infiniband/hw/irdma/cm.c:irdma_handle_fin_pkt() {
	...
	case IRDMA_CM_STATE_TIME_WAIT:
		cm_node->tcp_cntxt.rcv_nxt++;
		irdma_cleanup_retrans_entry(cm_node);
		cm_node->state = IRDMA_CM_STATE_CLOSED;
		irdma_rem_ref_cm_node(cm_node);
		break;
	...
}

drivers/infiniband/hw/irdma/cm.c:irdma_handle_rst_pkt() {
	...
	case IRDMA_CM_STATE_TIME_WAIT:
		cm_node->state = IRDMA_CM_STATE_CLOSED;
		irdma_rem_ref_cm_node(cm_node);
		break;
	...
}

When that happens, irdma_rem_ref_cm_node(cm_node) inside
irdma_handle_close_entry(cm_node, 1) consumes the caller's timer_list
reference, runs irdma_destroy_connection(cm_node), and schedules
kfree_rcu(cm_node, rcu_head). It then returns to irdma_cm_timer_tick(), which
is outside RCU protection and continues using cm_node before dropping the
timer_list reference at done:

drivers/infiniband/hw/irdma/cm.c:irdma_cm_timer_tick() {
	...
			} else {
				irdma_handle_close_entry(cm_node, 1);
			}
		}

		spin_lock_irqsave(&cm_node->retrans_list_lock, flags);

		send_entry = cm_node->send_entry;
		if (!send_entry)
			goto done;
	...
done:
		spin_unlock_irqrestore(&cm_node->retrans_list_lock, flags);
		irdma_rem_ref_cm_node(cm_node);
	}
}

Even with close_entry freed and cleared beforehand, wouldn't
irdma_cm_timer_tick() still access a destroyed cm_node and call
irdma_rem_ref_cm_node(cm_node) again when cm_node->refcnt is already 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104025347.1527584-1-vulab@iscas.ac.cn?part=1