Re: [PATCH net-next v17 11/15] quic: add crypto key derivation and installation

[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] quic_init_sock() (net/quic/socket.c:58) sets sk->sk_destruct =
  quic_sock_destruct.

--

quic: add crypto key derivation and installation

This patch adds quic_crypto, which derives and installs QUIC packet
protection keys for the Initial, Handshake, 0-RTT and 1-RTT levels. It
handles key updates and stateless reset token generation. It also adds a
quic_sock_destruct() callback that frees the per-level crypto transforms
when the last socket reference goes away.

> diff --git a/net/quic/socket.c b/net/quic/socket.c
> index 12f0817c8b55f..9b1106a9f5e73 100644
> --- a/net/quic/socket.c
> +++ b/net/quic/socket.c
> @@ -40,10 +40,22 @@ static void quic_write_space(struct sock *sk)
>  	rcu_read_unlock();
>  }
>  
> +static void quic_sock_destruct(struct sock *sk)
> +{
> +	u8 i;
> +
> +	/* Deferred crypto free for async encryption/decryption. */
> +	for (i = 0; i < QUIC_CRYPTO_MAX; i++)
> +		quic_crypto_free(quic_crypto(sk, i));

[Severity: High]
Is it safe to free the crypto transforms from sk_destruct, given the
context it can run in?

sk_destruct runs in whatever context drops the last socket reference.
Later in this series, several of those contexts are softirq:

  quic_packet_rcv() (UDP encap receive) ends with sock_put(sk)
  quic_timer_timeout() ends with sock_put(sk)
  quic_packet_rcv_err() drops its reference with sock_put()
  quic_packet_xmit() takes a reference per TX skb through
    skb_set_owner_sk_safe(); sock_efree() releases it on TX completion,
    usually in NAPI

If the user has already closed the socket, any of these can end up as:

  sock_put()
    __sk_destruct()
      quic_sock_destruct()
        quic_crypto_free()
          crypto_free_aead()
            crypto_destroy_tfm()
              alg->cra_exit()

The AEADs come from crypto_alloc_aead(cipher->aead, 0, 0), so an async
hardware implementation can be selected. hisi_sec2, for example, registers
both "gcm(aes)" and "ccm(aes)". Its sec_aead_exit() calls
dma_free_coherent(). Behind an IOMMU that leads to __iommu_dma_free() and
then vunmap(), which begins with:

mm/vmalloc.c:vunmap() {
	...
	BUG_ON(in_interrupt());
	...
}

Can this hit that BUG when a QUIC socket is destroyed from softirq?
Listen sockets already hold AEAD transforms in this series, via
quic_packet_get_alpn()->quic_crypto_initial_keys_install().

macsec hit the same crash and fixed it in commit 6624bba469a3 ("macsec:
use rcu_work to defer RX SA crypto cleanup out of softirq").

The note about in-flight async requests holding a socket reference covers
the lifetime of the transforms. It does not cover the context in which
the free runs.

Would it work to defer the tfm teardown to process context, for example
with a work item or rcu_work queued from quic_sock_destruct()?

> +
> +	quic_sk_destruct(sk);
> +}
> +
>  static int quic_init_sock(struct sock *sk)
>  {
>  	u8 i;
>  
> +	sk->sk_destruct = quic_sock_destruct;
>  	sk->sk_write_space = quic_write_space;
>  	sock_set_flag(sk, SOCK_USE_WRITE_QUEUE);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1792164438.git.lucien.xin%40gmail.com