Re: [PATCH net] tls: fix use-after-free in tls_sw_sendmsg() cork error path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Rolling back a split record causes iterator corruption and integer underflow.

Pre-existing issues:
- [High] Incomplete Use-After-Free fix leaves dangling `msg_en` pointer in error paths.
- [High] Silent data loss when `-ENOMEM` is returned and memory wait fails.
- [High] Memory leak of remainder record when asynchronous encryption returns `-EINPROGRESS`.
--

commit 660863238de21d269b42799c777dacfb8d63bed1
Author: Sechang Lim <rhkrqnwk98@gmail.com>

tls: fix use-after-free in tls_sw_sendmsg() cork error path

This commit attempts to fix a use-after-free in tls_sw_sendmsg_locked()
by re-fetching the msg_pl pointer from ctx->open_rec after
bpf_exec_tx_verdict() returns -ENOSPC and the record is split.

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
[ ... ]
> @@ -1161,10 +1161,11 @@ static int tls_sw_sendmsg_locked(struct sock *sk, struct msghdr *msg,
>  			if (ret) {
>  				if (ret == -EINPROGRESS)
>  					num_async++;
>  				else if (ret == -ENOMEM)
>  					goto wait_for_memory;
>  				else if (ctx->open_rec && ret == -ENOSPC) {
> +					msg_pl = &ctx->open_rec->msg_plaintext;
>  					if (msg_pl->cork_bytes) {
>  						ret = 0;
>  						goto send_end;
>  					}
>  					goto rollback_iter;
>  				} else if (ret != -EAGAIN)
>  					goto send_end;
>  			}
[ ... ]
> rollback_iter:
>  			copied -= try_to_copy;
>  			sk_msg_sg_copy_clear(msg_pl, first);
>  			iov_iter_revert(&msg->msg_iter,
>  					msg_pl->sg.size - orig_size);

[Severity: High]
When jumping to rollback_iter after the record is successfully split,
does msg_pl->sg.size - orig_size underflow?

Because msg_pl was updated to point to the new smaller remainder record,
msg_pl->sg.size is now smaller than the pre-split orig_size. This
subtraction results in a large positive value being passed to
iov_iter_revert(), corrupting the iterator state.

Additionally, sk_msg_sg_copy_clear() is called with the pre-split first
index, which is invalid for the new msg_pl scatterlist array. Does this
corrupt the remainder record?

[ ... ]
> fallback_to_reg_send:
>  			sk_msg_trim(sk, msg_pl, orig_size);
>  		}
>
>  		required_size = msg_pl->sg.size + try_to_copy;
>
>  		ret = tls_clone_plaintext_msg(sk, required_size);
>  		if (ret) {
>  			if (ret != -ENOSPC)
>  				goto send_end;
>
>  			/* Adjust try_to_copy according to the amount that was
>  			 * actually allocated. The difference is due
>  			 * to max sg elements limit
>  			 */
>  			try_to_copy -= required_size - msg_pl->sg.size;
>  			full_record = true;
>  			sk_msg_trim(sk, msg_en,
>  				    msg_pl->sg.size + prot->overhead_size);

[Severity: High]
This is a pre-existing issue, but does this leave the msg_en pointer
dangling?

The patch fixes the use-after-free for msg_pl, but msg_en is not
re-fetched. If the original record was queued and potentially freed by
tls_tx_records(), dereferencing msg_en here in the fallback path
results in a use-after-free identical to the one fixed for msg_pl.

[ ... ]
> wait_for_memory:
>  		ret = sk_stream_wait_memory(sk, &timeo);
>  		if (ret) {
> trim_sgl:
>  			if (ctx->open_rec)
>  				tls_trim_both_msgs(sk, orig_size);
>  			goto send_end;
>  		}

[Severity: High]
This is a pre-existing issue, but does this cause silent data loss if
sk_stream_wait_memory() fails?

In the zero-copy path, the copied variable is incremented by
try_to_copy before bpf_exec_tx_verdict() is called. If
bpf_exec_tx_verdict() returns -ENOMEM, we jump to wait_for_memory.

If the wait is interrupted (for example, by a signal), it jumps to
trim_sgl and drops the appended bytes via tls_trim_both_msgs(). However,
copied is never decremented to reflect this drop, leading userspace to
believe bytes were buffered or sent when they were actually discarded.

>  		if (ctx->open_rec && msg_en->sg.size < required_size)
>  			goto alloc_encrypted;

[Severity: High]
This is a pre-existing issue, but can this also trigger a use-after-free
on msg_en?

If bpf_exec_tx_verdict() returns -ENOMEM after the record was split and
queued, the code jumps directly to wait_for_memory without updating
msg_en. When sk_stream_wait_memory() resumes, msg_en->sg.size is
dereferenced, but it may point to the freed original record.


While reviewing this area, I also noticed a pre-existing issue in
tls_push_record() in the same file:

[Severity: High]
This is a pre-existing issue, but does this leak the tmp remainder
record when asynchronous encryption returns -EINPROGRESS?

tls_push_record() allocates a tmp remainder record when splitting:

net/tls/tls_sw.c:tls_push_record() {
    ...
    if (split) {
        tmp = tls_split_open_record(sk, rec, &orig_end, &msg_pl, &msg_en,
                                    &copied, &zc_frame, &bpf_strp);
    ...
    rc = tls_do_encryption(sk, tls_ctx, ctx, req, req_size, copied,
                           record_type);
    if (rc < 0) {
        if (rc != -EINPROGRESS) {
            tls_err_abort(sk, -EBADMSG);
            if (split) {
                ...
                tls_merge_open_record(sk, rec, tmp, orig_end);
            }
        }
        ctx->async_capable = 1;
        return rc;
    }
}

If tls_do_encryption() returns -EINPROGRESS, the function returns
immediately. The newly allocated tmp record is never assigned to
ctx->open_rec, never merged back, and never queued. Are the tmp record
and its allocations permanently leaked here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605214802.1647008-1-rhkrqnwk98@gmail.com?part=1