Re: [PATCH net v12 12/15] rxrpc: Fix packet encryption error handling

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] rxrpc_send_data() (net/rxrpc/sendmsg.c:460-468) treats every
  -ENOMEM from ->secure_packet() as "encryption hasn't happened yet".

--

rxrpc: Fix packet encryption error handling

This changes rxrpc_send_data() so that -ENOMEM from ->secure_packet() is
treated as retryable. When the failing packet was the final one, the last
append is rewound. Any other error sets a new RXRPC_CALL_TX_ERROR flag, which
makes later sendmsg() calls on the call fail with -EIO.

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

> diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
> index dff166ff78eb1..072237f5e17a0 100644
> --- a/net/rxrpc/sendmsg.c
> +++ b/net/rxrpc/sendmsg.c

[ ... ]

> @@ -443,14 +452,29 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  		/* add the packet to the send queue if it's now full */
>  		if (!txb->space ||
>  		    (len == 0 && !more)) {
> -			if (len == 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) {
> +				/* Assume that ENOMEM here means that the
> +				 * encryption hasn't happened yet.  The data is
> +				 * aligned to avoid the need for slow buffering
> +				 * in the crypto walk.
> +				 */
> +				if (ret == -ENOMEM)
> +					goto maybe_error_rewind;
> +				set_bit(RXRPC_CALL_TX_ERROR, &call->flags);

[Severity: High]
Is it safe to assume that -ENOMEM from ->secure_packet() means none of the
txbuf has been encrypted yet?

For rxgk at RXRPC_SECURITY_ENCRYPT, rxgk_secure_packet_encrypted()
encrypts txb->data in place. It returns any -ENOMEM from the crypto layer
unchanged:

rxgk_secure_packet_encrypted()
  crypto_krb5_encrypt()
    krb5_aead_encrypt()
      crypto_aead_encrypt()    /* src == dst, request flags 0 */
        ... cts(cbc(aes))

The AES-NI and ARM32 CE drivers have their own cts_cbc_encrypt(). It
encrypts the CBC prefix in place first, then runs a second walk over the
CTS tail:

arch/x86/crypto/aesni-intel_glue.c:cts_cbc_encrypt() {
    ...
	err = cbc_encrypt(&subreq);
    ...
	err = skcipher_walk_virt(&walk, &subreq, false);
	if (err)
		return err;
    ...
}

With CONFIG_HIGHMEM, scatterwalk_clamp() limits each walk segment to the
current 4K page. Txbufs come from __page_frag_alloc_align() inside a
higher-order page, so the CTS tail can straddle a 4K boundary. When it does,
the walk falls back to skcipher_next_slow():

crypto/skcipher.c:skcipher_next_slow() {
    ...
	buffer = kzalloc(n, skcipher_walk_gfp(walk));
	if (!buffer)
		return skcipher_walk_done(walk, -ENOMEM);
    ...
}

krb5_aead_encrypt() passes request flags of 0, so this kzalloc() uses
GFP_ATOMIC. It can fail after the prefix has already been turned into
ciphertext. In that setup, the comment about the data being aligned to avoid
slow buffering doesn't seem to hold.

The rewind in maybe_error_rewind doesn't seem to help in this case:

	if (rewind_by && !more && !len) {

Suppose the txbuf filled up (txb->space == 0) with len > 0 left, or
MSG_MORE is set. Then nothing is rewound and a positive short count is
returned. The next sendmsg() copies nothing into the full tx_pending and
calls ->secure_packet() again on the partly encrypted buffer.

Now suppose the txbuf already held data from an earlier MSG_MORE sendmsg(),
as with AFS sending a header and then a body. Only the latest append is
rewound, so the earlier bytes in the encrypted prefix are left as
ciphertext.

On retry, that prefix gets encrypted a second time. krb5enc computes its
checksum over this mixed plaintext. authenc computes its MAC over the
resulting ciphertext. Either way, wouldn't the peer verify the packet and
accept the corrupted payload?

The commit message says the problem is that a retry "might succeed with a
corrupt buffer". Should -ENOMEM from the encryption step also set
RXRPC_CALL_TX_ERROR? Alternatively, could retryability depend on whether
the in-place transform has started?

[ ... ]

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