Re: [PATCH net v9 07/14] rxrpc: Fix update of call->tx_pending without holding lock

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 1 · High: 0 · Medium: 0 · Low: 0

- [Critical] rxrpc_send_data() (net/rxrpc/sendmsg.c) can store an
  *indeterminate* pointer into call->tx_pending.

--

rxrpc: Fix update of call->tx_pending without holding lock

This moves the release of call->user_mutex down into rxrpc_send_data() and
drops the _dropped_lock indicator parameter, so the callers no longer have
to work out whether the lock is still held.  The interrupted-reacquire path
in wait_for_space now returns directly instead of falling through the
common exit path, and the exit labels are renamed to out_txb/out_unlock.

Closes: https://sashiko.dev/#/patchset/20260702144919.172295-1-dhowells%40redhat.com

[ ... ]

[Severity: Critical]

Can out_txb publish an uninitialised txb into call->tx_pending?

txb is declared without an initialiser in rxrpc_send_data():

	struct rxrpc_txbuf *txb;

but the two flag tests at the top of the reload block run before txb is
ever assigned, and both jump out of the function:

reload:
	if (unlikely(test_bit(RXRPC_CALL_TX_NO_MORE, &call->flags))) {
		...
		ret = -EPROTO;
		goto maybe_error;
	}
	if (unlikely(test_bit(RXRPC_CALL_TX_ERROR, &call->flags))) {
		...
		ret = -EIO;
		goto maybe_error;
	}

	txb = call->tx_pending;
	call->tx_pending = NULL;

With copied == 0, which is the case on first entry, maybe_error falls
straight through to "goto out_txb", so "call->tx_pending = txb;" runs on a
never-assigned variable.

A two-sendmsg() sequence on one AF_RXRPC call looks like it reaches this:
the first sendmsg() sends the whole request without MSG_MORE, so
rxrpc_queue_packet() sets RXRPC_CALL_TX_NO_MORE for the last packet; a
second sendmsg() before the call completes passes the
rxrpc_call_is_complete() gate in rxrpc_do_sendmsg(), enters
rxrpc_send_data(), trips the TX_NO_MORE test and returns -EPROTO via
out_txb.

Does call->tx_pending then hold a stale stack value that rxrpc_destroy_call()
hands to rxrpc_put_txbuf()?

net/rxrpc/call_object.c:rxrpc_destroy_call() {
	...
	rxrpc_put_txbuf(call->tx_pending, rxrpc_txbuf_put_cleaned);
	...
}

net/rxrpc/txbuf.c:rxrpc_put_txbuf() {
	if (txb) {
		debug_id = txb->debug_id;
		call_debug_id = txb->call_debug_id;
		seq = txb->seq;
		dead = __refcount_dec_and_test(&txb->ref, &r);
		...
	}
}

That dereferences, decrements and possibly kfree()s whatever was stored.
CONFIG_INIT_STACK_ALL_ZERO would reduce this to a NULL store, but
CONFIG_INIT_STACK_NONE builds would not.

There also looks to be a txbuf reference dropped on the TX_ERROR path even
when the pointer happens to be NULL: the fatal secure_packet() error above
sets RXRPC_CALL_TX_ERROR and stores the affected txbuf into
call->tx_pending via out_txb, and the next sendmsg() overwrites that
pointer from the TX_ERROR branch before "txb = call->tx_pending;" runs, so
the txbuf and its data buffer are never freed.

The two flag tests were placed above the "txb = call->tx_pending;"
assignment by the earlier "rxrpc: Fix packet encryption error handling"
change in this series, and this patch's rename to out_txb keeps the same
path.  Would having those two paths use "goto out_unlock", or initialising
txb to NULL at declaration, be preferable?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907113743.1453210-1-dhowells%40redhat.com