Re: [PATCH net v3 04/14] rxrpc: Fix packet encryption error handling

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
https://sashiko.dev/#/patchset/20260713081022.2186481-8-dhowells@redhat.com?part=4
---
--- Patch [4]: [PATCH net v3 04/14] rxrpc: Fix packet encryption error handling ---
commit 01fc73762a5a2162d360f4b9e10f752440ebbd67
Author: David Howells <dhowells@redhat.com>

rxrpc: Fix packet encryption error handling

This commit modifies the error handling in rxrpc_send_data() when
secure_packet() fails. It treats -ENOMEM as a retryable error and marks
other errors as terminal by setting the RXRPC_CALL_TX_ERROR flag to abort
the call.

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

> diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
> --- a/net/rxrpc/sendmsg.c
> +++ b/net/rxrpc/sendmsg.c
[ ... ]
> @@ -440,12 +446,21 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  		/* add the packet to the send queue if it's now full */
>  		if (!txb->space ||
>  		    (msg_data_left(msg) == 0 && !more)) {
> -			if (msg_data_left(msg) == 0 && !more)
> -				txb->flags |= RXRPC_LAST_PACKET;
> -
> +			/* Do any required crypto.  If this fails, it could
> +			 * have corrupted the txbuf content with a partial
> +			 * encrypt.  Assume that ENOMEM is retryable, but
> +			 * everything else is terminal.
> +			 */
>  			ret = call->security->secure_packet(call, txb);
> -			if (ret < 0)
> +			if (ret < 0) {
> +				if (ret == -ENOMEM)
> +					goto maybe_error;

[Severity: High]
If secure_packet() fails with -ENOMEM, does this jump to maybe_error bypass
rxrpc_queue_packet() but still return a successful write length?

Looking at rxrpc_send_data(), the user data has already been copied into txb
and 'copied' has been incremented. The maybe_error path handles this:

maybe_error:
	if (copied) {
		...
		goto success;
	}

Because copied > 0, it jumps to success and returns the number of bytes
copied, including the data in the unqueued txb.

Could this mislead userspace into thinking the data was sent when it was
actually dropped, leaving the unencrypted packet permanently stuck in
call->tx_pending and leading to an application hang?

> +				set_bit(RXRPC_CALL_TX_ERROR, &call->flags);
>  				goto out;
> +			}
> +
> +			if (msg_data_left(msg) == 0 && !more)
> +				txb->flags |= RXRPC_LAST_PACKET;
>  			rxrpc_queue_packet(rx, call, txb, notify_end_tx);
>  			txb = NULL;
>  		}