Re: [net-next,v9,03/15] quic: provide common utilities and data structures

[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.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
quic: provide common utilities and data structures

This patch provides foundational data structures and utilities for
the QUIC stack, including hash tables for socket lookup, connection
ID management, and helpers for encoding/decoding transport parameters.

> diff --git a/net/quic/common.c b/net/quic/common.c
> new file mode 100644
> index 000000000000..94321f3f213d
> --- /dev/null
> +++ b/net/quic/common.c

[ ... ]

> +/* Reads a QUIC transport parameter value. */
> +u8 quic_get_param(u64 *pdest, u8 **pp, u32 *plen)
> +{
> +	u64 valuelen;
> +
> +	if (!quic_get_var(pp, plen, &valuelen))
> +		return 0;
> +
> +	if (*plen < valuelen)
> +		return 0;
> +
> +	if (!quic_get_var(pp, plen, pdest))
> +		return 0;
> +
> +	return (u8)valuelen;
> +}

Does quic_get_param() need to verify that quic_get_var() consumed exactly
valuelen bytes?

The function reads valuelen at the first quic_get_var() call, then decodes
the value with the second quic_get_var(). However, quic_get_var() determines
how many bytes to consume based on the varint prefix in the data, not from
valuelen.

If a malformed packet has valuelen=2 but the varint encoding requires 4 bytes,
quic_get_var() will consume 4 bytes while the function returns valuelen=2.
This could desynchronize parsing of subsequent parameters since the caller
expects the buffer to have advanced by valuelen bytes based on the return
value.

> diff --git a/net/quic/common.h b/net/quic/common.h
> new file mode 100644
> index 000000000000..bfec0aaf2907
> --- /dev/null
> +++ b/net/quic/common.h

[ ... ]

> +#define QUIC_CONN_ID_MAX_LEN	20

[ ... ]

> +struct quic_conn_id {
> +	u8 data[QUIC_CONN_ID_MAX_LEN];
> +	u8 len;
> +};
> +
> +static inline void quic_conn_id_update(struct quic_conn_id *conn_id, u8 *data, u32 len)
> +{
> +	memcpy(conn_id->data, data, len);
> +	conn_id->len = (u8)len;
> +}

Should quic_conn_id_update() validate that len does not exceed
QUIC_CONN_ID_MAX_LEN before the memcpy?

The destination buffer conn_id->data is a 20-byte array, but the len
parameter is u32 with no bounds check. If a caller passes len > 20, the
memcpy would overflow the buffer.