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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In net/quic/common.c, the per-token length check in
  quic_data_from_string() does not do what the v16 changelog says it…

--

quic: provide common utilities and data structures

This patch adds the base data structures and helpers for the QUIC stack:
packet header types, connection ID and address handling, and hash tables
for socket and connection ID lookup. It also adds a binary data type with
helpers for parsing, matching and memory management, plus encoders and
decoders for transport parameters and frames.

> diff --git a/net/quic/common.c b/net/quic/common.c
> new file mode 100644
> index 0000000000000..8114be4fbb1e6
> --- /dev/null
> +++ b/net/quic/common.c

[ ... ]

> +int quic_data_from_string(struct quic_data *to, u8 *from, u32 len)
> +{
> +	u32 remlen = to->len;
> +	struct quic_data d;
> +	u8 *p = to->data;
> +
> +	to->len = 0;
> +	while (len) {
> +		while (len && *from == ' ') {
> +			from++;
> +			len--;
> +		}
> +		if (!len)
> +			break;
> +		if (!remlen)
> +			return -EOVERFLOW;
> +		d.data = p++;
> +		d.len  = 0;
> +		remlen--;
> +		while (len) {
> +			if (*from == ',') {
> +				from++;
> +				len--;
> +				break;
> +			}
> +			if (!remlen)
> +				return -EOVERFLOW;
> +			if (d.len == U8_MAX)
> +				return -EINVAL;
> +			*p++ = *from++;

[Severity: Low]
Does this match what the v16 changelog says? The note reads:

  - Check each data length before copying to avoid partially writing
    oversized tokens before returning -EINVAL in quic_data_from_string().

The U8_MAX check sits inside the per-byte copy loop, and nothing checks
the token's length before copying starts. With a token longer than 255
bytes, *p++ = *from++ copies its first 255 bytes into to->data before
-EINVAL is returned. Every earlier token has already been written in
full by then.

Also, because to->len is set to 0 on entry, the caller's original length
is lost on every error return, both -EINVAL and -EOVERFLOW.

This doesn't look like a memory safety problem. remlen bounds every
write, and on error to->data[0..to->len) is still a valid truncated
length-prefixed list. Nothing in this series calls
quic_data_from_string() yet.

A later caller, such as an ALPN setsockopt, might expect the destination
to stay unchanged on error. Would it make sense to scan each token's
length before copying it, or else to reword the changelog to match what
the check does?

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