Re: [PATCH net v3] tipc: fix slab-use-after-free Read in tipc_aead_decrypt_done

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
tipc: fix slab-use-after-free Read in tipc_aead_decrypt_done

This commit attempts to fix a slab-use-after-free read in
tipc_aead_decrypt_done by taking a reference on the network namespace
before starting asynchronous decryption. It mirrors the encrypt-side fix
by keeping the reference across the async return and dropping it when done.

> diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
> index 6d3b6b89b1d17..84a6489da0360 100644
> --- a/net/tipc/crypto.c
> +++ b/net/tipc/crypto.c
> @@ -941,12 +941,20 @@ static int tipc_aead_decrypt(struct net *net, struct tipc_aead *aead,
>  		goto exit;
>  	}
>  
> +	/* Get net to avoid freed tipc_crypto when delete namespace */
> +	if (!maybe_get_net(aead->crypto->net)) {

[Severity: High]
Could accessing aead->crypto->net read freed memory if the namespace
teardown races with packet decryption?

If aead is a TX cluster key, its crypto pointer points to the per-netns
crypto_tx structure. During a concurrent namespace teardown, tipc_exit_net()
synchronously frees crypto_tx without waiting for an RCU grace period.

Since packet reception (tipc_rcv) executes in softirq under rcu_read_lock()
but without holding a namespace reference, dereferencing aead->crypto to get
the net pointer could result in a use-after-free.

Because tipc_aead_decrypt() already receives the struct net *net directly as
its first parameter, would it be safer to use maybe_get_net(net) instead to
avoid accessing the potentially freed aead->crypto structure entirely?

> +		tipc_bearer_put(b);
> +		rc = -ENODEV;
> +		goto exit;
> +	}
> +
>  	/* Now, do decrypt */
>  	rc = crypto_aead_decrypt(req);
>  	if (rc == -EINPROGRESS || rc == -EBUSY)
>  		return rc;
>  
>  	tipc_bearer_put(b);
> +	put_net(aead->crypto->net);

[Severity: High]
Does this put_net() call have the same use-after-free risk? Could this also
use the net parameter directly instead of dereferencing aead->crypto->net?

[ ... ]