Re: [PATCH net v2 2/4] net: tls: prevent chain-after-chain in plain text SG

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The patch fails to handle empty TLS records (size == 0, start == end) when a BPF program pops all data, leading to a chain-after-chain kernel panic or premature scatterlist termination.
--

commit a19dc6e44cc96cee4ac7cb297275155ee108e228
Author: Jakub Kicinski <kuba@kernel.org>

net: tls: prevent chain-after-chain in plain text SG

This commit adjusts the scatterlist chaining logic in tls_push_record() to
prevent an illegal chain-after-chain configuration when end = 0.

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 2608b0c01849f..3bfdaf5e64f50 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -789,21 +789,33 @@ static int tls_push_record(struct sock *sk, int flags,
>  	i = msg_pl->sg.end;
>  	sk_msg_iter_var_prev(i);

Does this code handle the case where a BPF program pops all data and leaves
an empty record where msg_pl->sg.size == 0 and start == end?

[ ... ]

> +	if (i < msg_pl->sg.start)
> +		sg_chain(msg_pl->sg.data, ARRAY_SIZE(msg_pl->sg.data),
> +			 msg_pl->sg.data);

If start == end == 0, i wraps to NR_MSG_FRAG_IDS - 1. The condition
i < msg_pl->sg.start evaluates to false, skipping the wrap link.

>  	rec->content_type = record_type;
>  	if (prot->version == TLS_1_3_VERSION) {
>  		/* Add content type to end of message.  No padding added */
>  		sg_set_buf(&rec->sg_content_type, &rec->content_type, 1);
>  		sg_mark_end(&rec->sg_content_type);
> -		sg_chain(msg_pl->sg.data, msg_pl->sg.end + 1,
> -			 &rec->sg_content_type);
> +		sg_chain(msg_pl->sg.data, i + 2, &rec->sg_content_type);

In the start == end == 0 scenario, the content type link is placed at
data[NR_MSG_FRAG_IDS].

If start == end > 0 (for example, start = 5 and end = 5), i becomes 4.
The condition i < msg_pl->sg.start is true, so the wrap link is created.
Here, msg_pl->sg.data is chained at i + 2 (which is 6, meaning data[5])
to rec->sg_content_type.

>  	} else {
>  		sg_mark_end(sk_msg_elem(msg_pl, i));
>  	}
>  
> -	if (msg_pl->sg.end < msg_pl->sg.start)
> -		sg_chain(msg_pl->sg.data, ARRAY_SIZE(msg_pl->sg.data),
> -			 msg_pl->sg.data);
> -
>  	i = msg_pl->sg.start;
>  	sg_chain(rec->sg_aead_in, 2, &msg_pl->sg.data[i]);

When start == end == 0, sg_aead_in chains to data[0]. Since the list starting
at data[0] never links to data[NR_MSG_FRAG_IDS], the scatterlist iterator
might hit a premature SG_END at data[0] instead of finding the content type
link. Can this cause the crypto API to fail because the expected cryptlen
is not satisfied?

When start == end > 0 (e.g., 5), sg_aead_in is chained to msg_pl->sg.data[5].
Since data[5] is already a chain link to rec->sg_content_type, doesn't this
result in an illegal chain-after-chain configuration that the scatterlist API
cannot resolve?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511174920.433155-1-kuba@kernel.org?part=2