Re: [PATCH net-next v13 07/15] quic: add connection id management

[Paolo Abeni <pabeni@redhat.com>]

From: AI Reviewer <ai@example.com>

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://netdev-ai.bots.linux.dev/sashiko/
---
Now I have enough information to produce the report.

commit e00266d0eea858a744fd827548eb8ebc6868f92c
Author: Xin Long <lucien.xin@gmail.com>

quic: add connection id management

This commit introduces struct quic_conn_id_set for managing QUIC
Connection IDs along with helpers to add, remove, and look up source
and destination CIDs, including a global hash table for source CID
lookup on the RX path.

> diff --git a/net/quic/connid.c b/net/quic/connid.c
> new file mode 100644
> index 000000000000..b1a39feb827e
> --- /dev/null
> +++ b/net/quic/connid.c
> @@ -0,0 +1,256 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* QUIC kernel implementation
> + * (C) Copyright Red Hat Corp. 2023
> + *
> + * This file is part of the QUIC kernel implementation
> + *
> + * Initialization/cleanup for QUIC protocol support.
> + *
> + * Written or modified by:
> + *    Xin Long <lucien.xin@gmail.com>
> + */
> +
> +#include <linux/quic.h>
> +#include <net/sock.h>
> +
> +#include "common.h"
> +#include "connid.h"
> +
> +/* Lookup a source connection ID (scid) in the global source connection ID hash
> + * table.
> + */
> +struct quic_conn_id *quic_conn_id_lookup(struct net *net, u8 *scid, u32 len)
> +{
> +	struct quic_shash_head *head = quic_source_conn_id_head(net, scid, len);
> +	struct quic_source_conn_id *s_conn_id;
> +	struct quic_conn_id *conn_id = NULL;
> +	struct hlist_nulls_node *node;
> +
> +	hlist_nulls_for_each_entry_rcu(s_conn_id, node, &head->head, node) {
> +		if (net != sock_net(s_conn_id->sk))
> +			continue;
> +		if (s_conn_id->common.id.len != len ||
> +		    memcmp(scid, &s_conn_id->common.id.data, len))
> +			continue;
> +		if (likely(refcount_inc_not_zero(&s_conn_id->sk->sk_refcnt)))
> +			conn_id = &s_conn_id->common.id;
> +		break;
> +	}

[Critical]
Can s_conn_id->sk be freed while this RCU walk is running?  The
s_conn_id itself is freed via call_rcu() in quic_source_conn_id_free(),
but the struct sock it points to does not appear to be RCU-protected.

A grep across this series for SOCK_RCU_FREE and SLAB_TYPESAFE_BY_RCU
under net/quic returns no matches, and quic_init_sock() does not call
sock_set_flag(sk, SOCK_RCU_FREE), so sk_destruct() will free the sock
synchronously rather than after a grace period.

Consider this sequence:

  CPU0 (RX path, rcu_read_lock held)        CPU1 (close path)
  hlist_nulls_for_each_entry_rcu(..)
    /* sees s_conn_id still linked */
                                            sk_destruct()
                                              quic_destroy_sock()
                                                quic_conn_id_set_free()
                                                  /* unhash + call_rcu
                                                     defers s_conn_id
                                                     free, but not sk */
                                              sk_prot_free()  /* sk gone */
    sock_net(s_conn_id->sk)         /* UAF read of freed sock */
    refcount_inc_not_zero(
        &s_conn_id->sk->sk_refcnt)  /* UAF refcount op */

Once quic_packet_get_sock() (in the later 'quic: add packet parser
base' patch) wires this lookup into every short- and long-header
packet, an incoming packet racing with close() on the matching socket
would dereference freed sock memory.

Would setting SOCK_RCU_FREE on the QUIC sockets, switching the proto
slabs to SLAB_TYPESAFE_BY_RCU, taking sock_hold() when the sk pointer
is stored in s_conn_id, or otherwise gating the sk free behind an RCU
grace period address this?

> +
> +/* Check if a given stateless reset token exists in any connection ID in the
> + * connection ID set.
> + */
> +bool quic_conn_id_token_exists(struct quic_conn_id_set *id_set, u8 *token)
> +{
> +	struct quic_common_conn_id *common;
> +	struct quic_dest_conn_id *dcid;
> +
> +	dcid = (struct quic_dest_conn_id *)id_set->active;
> +	if (memchr_inv(dcid->token, 0, QUIC_CONN_ID_TOKEN_LEN) &&
> +	    !memcmp(dcid->token, token, QUIC_CONN_ID_TOKEN_LEN))
> +		return true; /* Fast path. */
> +
> +	list_for_each_entry(common, &id_set->head, list) {
> +		dcid = (struct quic_dest_conn_id *)common;
> +		if (common == id_set->active)
> +			continue;
> +		if (memchr_inv(dcid->token, 0, QUIC_CONN_ID_TOKEN_LEN) &&
> +		    !memcmp(dcid->token, token, QUIC_CONN_ID_TOKEN_LEN))
> +			return true;
> +	}
> +	return false;
> +}

[Medium]
Is memcmp() the right comparison primitive for the stateless reset
token here?  RFC 9000 section 10.3 treats the token as a secret that
authorises tearing down a connection, and the commit message states
this helper is used during stateless reset processing on incoming
packets.

lib/string.c's memcmp() returns at the first differing byte (and
performs unaligned word-sized loads on architectures that allow it),
so the time taken to return false leaks how many leading bytes of the
supplied token match a stored token.  Would crypto_memneq() from
<crypto/algapi.h>, which is constant-time, be more appropriate for
comparing the token bytes?

> +
[ ... ]
> +/* Add a connection ID with sequence number and associated private data to the
> + * connection ID set.
> + */
> +int quic_conn_id_add(struct quic_conn_id_set *id_set,
> +		     struct quic_conn_id *conn_id, u32 number, void *data)
> +{
[ ... ]
> +	/* Locate insertion point to keep list ordered by number. */
> +	list = &id_set->head;
> +	list_for_each_entry(common, list, list) {
> +		if (number == common->number)
> +			return 0; /* Ignore if it already exists on the list. */
> +		if (number < common->number) {
> +			list = &common->list;
> +			break;
> +		}
> +	}
[ ... ]
> +	list_add_tail(&common->list, list);
> +
> +	if (number == quic_conn_id_last_number(id_set) + 1) {
> +		if (!id_set->active)
> +			id_set->active = common;
> +		id_set->count++;
> +
> +		/* Increment count for consecutive following IDs. */
> +		list_for_each_entry_continue(common, &id_set->head, list) {
> +			if (common->number != ++number)
> +				break;
> +			id_set->count++;
> +		}
> +	}
> +	return 0;
> +}

[Medium]
Does this function correctly maintain id_set->count when a CID is
inserted before all existing entries?  The loop above explicitly
handles the "number < common->number" case, suggesting out-of-order
insertion is intended to be supported.

Consider starting from list=[5], count=1 (so first=5, last=5) and
calling quic_conn_id_add(..., number=3, ...):

  - The location loop sets list = &common(5)->list and breaks.
  - list_add_tail() inserts common(3) before common(5),
    yielding list=[3, 5].
  - The post-insert check evaluates:
        quic_conn_id_last_number(id_set)
            = quic_conn_id_first_number(id_set) + count - 1
            = 3 + 1 - 1
            = 3
    so the test "number == last + 1" becomes "3 == 4" and the count
    update branch is skipped.

The end state is list=[3, 5] with count still 1, so entry 5 is
silently uncounted.  A subsequent quic_conn_id_remove(id_set, 3)
then trips the WARN_ON_ONCE(number >= last_number) because
last_number reads as 3.

Should the count adjustment also handle the case where the inserted
CID lowers first_number?

> +
> +/* Remove consecutive connection IDs from the set with sequence numbers less
> + * than or equal to a number.
> + */
> +void quic_conn_id_remove(struct quic_conn_id_set *id_set, u32 number)
[ ... ]
-- 
This is an AI-generated review.