Re: [PATCH net-next v4 1/2] tls: annotate lockless access to sk->sk_err

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] tls: SG chain corruption via lockless sk_err consumption race
--

commit af572fb8fe2e730f2c3767f401e262816bb05b1d
Author: Quanye Yang <quanyeyang@proton.me>

tls: annotate lockless access to sk->sk_err

This commit annotates lockless reads of sk->sk_err using READ_ONCE()
across various TLS data paths to prevent compiler optimizations. It folds
multiple unannotated reads into single variables for consistency within
a function execution block.

Link: https://lore.kernel.org/netdev/3d9d442f-f168-43da-87b0-010ad5a78365@redhat.com/

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 312e51270f293..4221b2ad3c1d3 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
[ ... ]
> @@ -488,13 +489,15 @@ static void tls_encrypt_done(void *data, int err)
>  	sge->offset -= prot->prepend_size;
>  	sge->length += prot->prepend_size;
>  
> +	skerr = READ_ONCE(sk->sk_err);
> +
>  	/* Check if error is previously set on socket */
> -	if (err || sk->sk_err) {
> +	if (err || skerr) {

[Severity: High]
Can a concurrent lockless call to getsockopt(SO_ERROR) clear the error state
here?

If getsockopt() consumes and clears the socket error via xchg(), skerr
could read as 0. Does this allow the code to bypass the error check and
transmit data on a connection that has already been aborted?

[ ... ]
> @@ -701,13 +704,17 @@ static int tls_push_record(struct sock *sk, int flags,
>  static int bpf_exec_tx_verdict(struct sk_msg *msg, struct sock *sk,
>  			       u8 record_type, ssize_t *copied, int flags)
>  {
> +	int skerr;
>  	int err;
>  
>  	err = tls_push_record(sk, flags, record_type);
> -	if (err && err != -EINPROGRESS && sk->sk_err == EBADMSG) {
> -		*copied -= sk_msg_free(sk, msg);
> -		tls_free_open_rec(sk);
> -		err = -sk->sk_err;
> +	if (err && err != -EINPROGRESS) {
> +		skerr = READ_ONCE(sk->sk_err);
> +		if (skerr == EBADMSG) {

[Severity: High]
Is it safe to rely on reading the transient socket error to clean up a
fatally broken record?

When tls_push_record() prepares the scatterlist for encryption, it modifies
the list irreversibly (for example, by appending the content type via
sg_chain). If encryption fails synchronously, tls_err_abort() sets the
socket error to EBADMSG.

If a concurrent lockless call to getsockopt(SO_ERROR) consumes and clears
the error, the READ_ONCE() here might return 0 instead of EBADMSG.

Does this cause the cleanup to be bypassed, leaking the corrupted record in
the open record context? If so, on the next sendmsg() call from userspace,
reusing this corrupted record and appending new data might overwrite the
scatterlist chain link with a page pointer, potentially leading to memory
corruption or a kernel panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-tls-fix-sk-kcsan-err-v4-0-391d788327f3@proton.me?part=1