[PATCH net] net/tls: preserve sk_msg sg.copy when splitting records

[PATCH net] net/tls: preserve sk_msg sg.copy when splitting records

[Yiming Qian]

tls_split_open_record() copies scatterlist entries from the current
plaintext sk_msg into a newly allocated plaintext sk_msg when an open
record is split.

The scatterlist entry and the corresponding msg->sg.copy bit are one
ownership record. Splice-backed entries are created by sk_msg_page_add()
with the copy bit set so sk_msg_compute_data_pointers() does not expose
them as writable BPF msg->data.

The split path used memcpy() to copy both partial and whole tail entries
but left the new sk_msg copy bitmap clear. A subsequent SK_MSG verdict on
the split tail could therefore receive a writable data pointer to a page
that was only supposed to be copied, allowing BPF to overwrite externally
owned page cache.

Add a helper for copying one sg.copy bit and use it for the partial tmp
entry and for each copied tail entry.

Fixes: d3b18ad31f93 ("tls: add bpf support to sk_msg handling")
Reported-by: Yiming Qian <yimingqian591@gmail.com>
Reported-by: Keenan Dong <keenanat2000@gmail.com>
Signed-off-by: Yiming Qian <yimingqian591@gmail.com>
Signed-off-by: Keenan Dong <keenanat2000@gmail.com>
---
 include/linux/skmsg.h | 9 +++++++++
 net/tls/tls_sw.c      | 7 +++++++
 2 files changed, 16 insertions(+)

diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
index 19f4f253b4f90..f3988ce2219db 100644
--- a/include/linux/skmsg.h
+++ b/include/linux/skmsg.h
@@ -283,6 +283,15 @@ static inline void sk_msg_sg_copy(struct sk_msg *msg, u32 i, bool copy_state)
 	} while (1);
 }
 
+static inline void sk_msg_sg_copy_one(struct sk_msg *dst, u32 dst_i,
+				      const struct sk_msg *src, u32 src_i)
+{
+	if (test_bit(src_i, src->sg.copy))
+		__set_bit(dst_i, dst->sg.copy);
+	else
+		__clear_bit(dst_i, dst->sg.copy);
+}
+
 static inline void sk_msg_sg_copy_set(struct sk_msg *msg, u32 start)
 {
 	sk_msg_sg_copy(msg, start, true);
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 964ebc268ee46..434753de8aadd 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -623,6 +623,7 @@ static int tls_split_open_record(struct sock *sk, struct tls_rec *from,
 	struct scatterlist *sge, *osge, *nsge;
 	u32 orig_size = msg_opl->sg.size;
 	struct scatterlist tmp = { };
+	u32 tmp_i = NR_MSG_FRAG_IDS;
 	struct sk_msg *msg_npl;
 	struct tls_rec *new;
 	int ret;
@@ -644,6 +645,7 @@ static int tls_split_open_record(struct sock *sk, struct tls_rec *from,
 		if (sge->length > apply) {
 			u32 len = sge->length - apply;
 
+			tmp_i = i;
 			get_page(sg_page(sge));
 			sg_set_page(&tmp, sg_page(sge), len,
 				    sge->offset + apply);
@@ -675,6 +677,10 @@ static int tls_split_open_record(struct sock *sk, struct tls_rec *from,
 	nsge = sk_msg_elem(msg_npl, j);
 	if (tmp.length) {
 		memcpy(nsge, &tmp, sizeof(*nsge));
+		if (WARN_ON_ONCE(tmp_i == NR_MSG_FRAG_IDS))
+			__clear_bit(j, msg_npl->sg.copy);
+		else
+			sk_msg_sg_copy_one(msg_npl, j, msg_opl, tmp_i);
 		sk_msg_iter_var_next(j);
 		nsge = sk_msg_elem(msg_npl, j);
 	}
@@ -682,6 +688,7 @@ static int tls_split_open_record(struct sock *sk, struct tls_rec *from,
 	osge = sk_msg_elem(msg_opl, i);
 	while (osge->length) {
 		memcpy(nsge, osge, sizeof(*nsge));
+		sk_msg_sg_copy_one(msg_npl, j, msg_opl, i);
 		sg_unmark_end(nsge);
 		sk_msg_iter_var_next(i);
 		sk_msg_iter_var_next(j);

Re: [PATCH net] net/tls: preserve sk_msg sg.copy when splitting records

[sashiko-bot]

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

Pre-existing issues:
- [Critical] tls_merge_open_record incorrectly merges naturally contiguous scatterlist elements, leading to a page double-free.
- [Critical] tls_split_open_record() ignores the split_point argument, leading to an out-of-bounds write in the crypto layer.
- [High] sk_msg element shifting functions fail to shift the msg->sg.copy bitmap, allowing BPF to overwrite externally owned page cache memory.
- [High] sk_msg_alloc reuses freed scatterlist indices without clearing the msg->sg.copy bit, causing valid memory to be treated as read-only.
--

commit d2043cc374b476d5c2b7d9a3d811593ca2bebf5c
Author: Yiming Qian <yimingqian591@gmail.com>

net/tls: preserve sk_msg sg.copy when splitting records

This patch modifies tls_split_open_record to correctly copy the sg.copy
bit state from the original sk_msg into the new sk_msg when an open record
is split. It introduces a helper sk_msg_sg_copy_one to facilitate this bit
copying for both partial and whole tail entries.

> diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
> index 19f4f253b4f90..f3988ce2219db 100644
> --- a/include/linux/skmsg.h
> +++ b/include/linux/skmsg.h
[ ... ]
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 964ebc268ee46..434753de8aadd 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -623,6 +623,7 @@ static int tls_split_open_record(struct sock *sk, struct tls_rec *from,
>  	struct scatterlist *sge, *osge, *nsge;
>  	u32 orig_size = msg_opl->sg.size;
>  	struct scatterlist tmp = { };

[Severity: Critical]
This is a pre-existing issue, but does tls_split_open_record ignore the
split_point argument?

When a BPF program uses bpf_msg_push_data to expand a payload beyond the
pre-allocated encrypted buffer capacity, tls_push_record calculates a
necessary split_point and calls tls_split_open_record.

However, tls_split_open_record ignores the split_point argument entirely
and unconditionally sets apply = msg_opl->apply_bytes.

If apply_bytes is 0, the function effectively splits at 0 bytes, creating
an empty msg_opl. The fallback logic in tls_push_record then merges them
back together.

Subsequently, sk_msg_trim silently fails because the size is larger than
the target, and tls_do_encryption uses the undersized buffer to encrypt
the oversized msg_pl. Can this cause an out-of-bounds write in the
scatterlist array when constructing the crypto request?

> +	u32 tmp_i = NR_MSG_FRAG_IDS;
>  	struct sk_msg *msg_npl;
[ ... ]

net/tls/tls_sw.c:tls_merge_open_record() {
    ...
    if (sg_page(osge) == sg_page(nsge) &&
        osge->offset + osge->length == nsge->offset) {
        osge->length += nsge->length;
        put_page(sg_page(nsge));
    }
    ...
}

[Severity: Critical]
This isn't a bug introduced by this patch, but does tls_merge_open_record
incorrectly merge naturally contiguous scatterlist elements?

When tls_split_open_record splits an open record exactly on an element
boundary, no extra page reference is acquired.

If encryption subsequently fails, tls_merge_open_record attempts to merge
the elements back. To detect a split element, it merely checks if the
pages are the same and the offsets are contiguous.

If the user originally provided two elements that were naturally contiguous
in memory, this condition falsely evaluates to true. tls_merge_open_record
then adds their lengths and calls put_page on sg_page(nsge), improperly
releasing a reference that was never taken.

The element remains in the scatterlist, resulting in another put_page
during destruction. Does this directly cause a double-free on the page?

net/core/filter.c:sk_msg_shift_left() {
    ...
    do {
        prev = i;
        sk_msg_iter_var_next(i);
        msg->sg.data[prev] = msg->sg.data[i];
    } while (i != msg->sg.end);
    ...
}

net/core/filter.c:sk_msg_shift_right() {
    ...
    while (i != msg->sg.end) {
        msg->sg.data[i] = sge;
        sk_msg_iter_var_next(i);
        sge = tmp;
        tmp = sk_msg_elem_cpy(msg, i);
    }
    ...
}

[Severity: High]
This is a pre-existing issue, but do the sk_msg element shifting functions
fail to shift the msg->sg.copy bitmap?

BPF helpers manipulate sk_msg scatterlist arrays using sk_msg_shift_left
and sk_msg_shift_right. These functions physically shift the msg->sg.data
entries but fail to concurrently shift the corresponding msg->sg.copy
bitmap.

Consequently, shifted scatterlist elements incorrectly inherit the
read-only states of their new indices.

If a read-only element is shifted into an index where the copy bit is 0,
sk_msg_compute_data_pointers will erroneously provide a writable data
pointer to the BPF program. Could this allow an unprivileged BPF program
to write to externally owned page cache pages?

net/core/skmsg.c:sk_msg_alloc() {
    ...
        sge = &msg->sg.data[msg->sg.end];
        sg_unmark_end(sge);
        sg_set_page(sge, pfrag->page, use, orig_offset);
        get_page(pfrag->page);
        sk_msg_iter_next(msg, end);
    ...
}

net/core/skmsg.c:sk_msg_free_elem() {
    ...
    memset(sge, 0, sizeof(*sge));
    return len;
}

[Severity: High]
This isn't a bug introduced by this patch, but does sk_msg_alloc reuse
freed scatterlist indices without clearing the msg->sg.copy bit?

When sk_msg_free_elem or sk_msg_trim frees a scatterlist element, it zeroes
the sg.data entry but does not clear the corresponding bit in the
msg->sg.copy bitmap.

If sk_msg_alloc later allocates new memory and reuses that same index in
the ring buffer, the stale copy bit remains set.

Because of this, sk_msg_compute_data_pointers incorrectly assumes the newly
allocated, writable memory is read-only and sets msg->data to NULL. Does
this abruptly break BPF programs that rely on writing to pushed data?

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

Re: [PATCH net] net/tls: preserve sk_msg sg.copy when splitting records

[钱一铭]

Thanks for the review.

  The sk_msg_shift_left/right issue is the related pre-existing sk_msg
  sg.copy bug that I explicitly separated in the report. This patch does
  not attempt to fix that helper/shift transform site.

  This patch is limited to a different transform site in kTLS:
  tls_split_open_record() copies SG entries into a new sk_msg but failed to
  copy the corresponding sg.copy bitmap state. The fix preserves that
  metadata only for the entries copied by tls_split_open_record().

  The other findings appear pre-existing and independent. They should not
  block this focused fix, and can be investigated or fixed separately if
  needed.

<sashiko-bot@kernel.org> 于2026年6月5日周五 21:40写道：
>
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] tls_merge_open_record incorrectly merges naturally contiguous scatterlist elements, leading to a page double-free.
> - [Critical] tls_split_open_record() ignores the split_point argument, leading to an out-of-bounds write in the crypto layer.
> - [High] sk_msg element shifting functions fail to shift the msg->sg.copy bitmap, allowing BPF to overwrite externally owned page cache memory.
> - [High] sk_msg_alloc reuses freed scatterlist indices without clearing the msg->sg.copy bit, causing valid memory to be treated as read-only.
> --
>
> commit d2043cc374b476d5c2b7d9a3d811593ca2bebf5c
> Author: Yiming Qian <yimingqian591@gmail.com>
>
> net/tls: preserve sk_msg sg.copy when splitting records
>
> This patch modifies tls_split_open_record to correctly copy the sg.copy
> bit state from the original sk_msg into the new sk_msg when an open record
> is split. It introduces a helper sk_msg_sg_copy_one to facilitate this bit
> copying for both partial and whole tail entries.
>
> > diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
> > index 19f4f253b4f90..f3988ce2219db 100644
> > --- a/include/linux/skmsg.h
> > +++ b/include/linux/skmsg.h
> [ ... ]
> > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> > index 964ebc268ee46..434753de8aadd 100644
> > --- a/net/tls/tls_sw.c
> > +++ b/net/tls/tls_sw.c
> > @@ -623,6 +623,7 @@ static int tls_split_open_record(struct sock *sk, struct tls_rec *from,
> >       struct scatterlist *sge, *osge, *nsge;
> >       u32 orig_size = msg_opl->sg.size;
> >       struct scatterlist tmp = { };
>
> [Severity: Critical]
> This is a pre-existing issue, but does tls_split_open_record ignore the
> split_point argument?
>
> When a BPF program uses bpf_msg_push_data to expand a payload beyond the
> pre-allocated encrypted buffer capacity, tls_push_record calculates a
> necessary split_point and calls tls_split_open_record.
>
> However, tls_split_open_record ignores the split_point argument entirely
> and unconditionally sets apply = msg_opl->apply_bytes.
>
> If apply_bytes is 0, the function effectively splits at 0 bytes, creating
> an empty msg_opl. The fallback logic in tls_push_record then merges them
> back together.
>
> Subsequently, sk_msg_trim silently fails because the size is larger than
> the target, and tls_do_encryption uses the undersized buffer to encrypt
> the oversized msg_pl. Can this cause an out-of-bounds write in the
> scatterlist array when constructing the crypto request?
>
> > +     u32 tmp_i = NR_MSG_FRAG_IDS;
> >       struct sk_msg *msg_npl;
> [ ... ]
>
> net/tls/tls_sw.c:tls_merge_open_record() {
>     ...
>     if (sg_page(osge) == sg_page(nsge) &&
>         osge->offset + osge->length == nsge->offset) {
>         osge->length += nsge->length;
>         put_page(sg_page(nsge));
>     }
>     ...
> }
>
> [Severity: Critical]
> This isn't a bug introduced by this patch, but does tls_merge_open_record
> incorrectly merge naturally contiguous scatterlist elements?
>
> When tls_split_open_record splits an open record exactly on an element
> boundary, no extra page reference is acquired.
>
> If encryption subsequently fails, tls_merge_open_record attempts to merge
> the elements back. To detect a split element, it merely checks if the
> pages are the same and the offsets are contiguous.
>
> If the user originally provided two elements that were naturally contiguous
> in memory, this condition falsely evaluates to true. tls_merge_open_record
> then adds their lengths and calls put_page on sg_page(nsge), improperly
> releasing a reference that was never taken.
>
> The element remains in the scatterlist, resulting in another put_page
> during destruction. Does this directly cause a double-free on the page?
>
> net/core/filter.c:sk_msg_shift_left() {
>     ...
>     do {
>         prev = i;
>         sk_msg_iter_var_next(i);
>         msg->sg.data[prev] = msg->sg.data[i];
>     } while (i != msg->sg.end);
>     ...
> }
>
> net/core/filter.c:sk_msg_shift_right() {
>     ...
>     while (i != msg->sg.end) {
>         msg->sg.data[i] = sge;
>         sk_msg_iter_var_next(i);
>         sge = tmp;
>         tmp = sk_msg_elem_cpy(msg, i);
>     }
>     ...
> }
>
> [Severity: High]
> This is a pre-existing issue, but do the sk_msg element shifting functions
> fail to shift the msg->sg.copy bitmap?
>
> BPF helpers manipulate sk_msg scatterlist arrays using sk_msg_shift_left
> and sk_msg_shift_right. These functions physically shift the msg->sg.data
> entries but fail to concurrently shift the corresponding msg->sg.copy
> bitmap.
>
> Consequently, shifted scatterlist elements incorrectly inherit the
> read-only states of their new indices.
>
> If a read-only element is shifted into an index where the copy bit is 0,
> sk_msg_compute_data_pointers will erroneously provide a writable data
> pointer to the BPF program. Could this allow an unprivileged BPF program
> to write to externally owned page cache pages?
>
> net/core/skmsg.c:sk_msg_alloc() {
>     ...
>         sge = &msg->sg.data[msg->sg.end];
>         sg_unmark_end(sge);
>         sg_set_page(sge, pfrag->page, use, orig_offset);
>         get_page(pfrag->page);
>         sk_msg_iter_next(msg, end);
>     ...
> }
>
> net/core/skmsg.c:sk_msg_free_elem() {
>     ...
>     memset(sge, 0, sizeof(*sge));
>     return len;
> }
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does sk_msg_alloc reuse
> freed scatterlist indices without clearing the msg->sg.copy bit?
>
> When sk_msg_free_elem or sk_msg_trim frees a scatterlist element, it zeroes
> the sg.data entry but does not clear the corresponding bit in the
> msg->sg.copy bitmap.
>
> If sk_msg_alloc later allocates new memory and reuses that same index in
> the ring buffer, the stale copy bit remains set.
>
> Because of this, sk_msg_compute_data_pointers incorrectly assumes the newly
> allocated, writable memory is read-only and sets msg->data to NULL. Does
> this abruptly break BPF programs that rely on writing to pushed data?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260604134019.39161-1-yimingqian591@gmail.com?part=1

Re: [PATCH net] net/tls: preserve sk_msg sg.copy when splitting records

[Jakub Kicinski]

On Thu,  4 Jun 2026 13:40:11 +0000 Yiming Qian wrote:
> tls_split_open_record() copies scatterlist entries from the current
> plaintext sk_msg into a newly allocated plaintext sk_msg when an open
> record is split.
> 
> The scatterlist entry and the corresponding msg->sg.copy bit are one
> ownership record. Splice-backed entries are created by sk_msg_page_add()
> with the copy bit set so sk_msg_compute_data_pointers() does not expose
> them as writable BPF msg->data.
> 
> The split path used memcpy() to copy both partial and whole tail entries
> but left the new sk_msg copy bitmap clear. A subsequent SK_MSG verdict on
> the split tail could therefore receive a writable data pointer to a page
> that was only supposed to be copied, allowing BPF to overwrite externally
> owned page cache.
> 
> Add a helper for copying one sg.copy bit and use it for the partial tmp
> entry and for each copied tail entry.
> 
> Fixes: d3b18ad31f93 ("tls: add bpf support to sk_msg handling")
> Reported-by: Yiming Qian <yimingqian591@gmail.com>
> Reported-by: Keenan Dong <keenanat2000@gmail.com>
> Signed-off-by: Yiming Qian <yimingqian591@gmail.com>
> Signed-off-by: Keenan Dong <keenanat2000@gmail.com>
> ---
>  include/linux/skmsg.h | 9 +++++++++
>  net/tls/tls_sw.c      | 7 +++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
> index 19f4f253b4f90..f3988ce2219db 100644
> --- a/include/linux/skmsg.h
> +++ b/include/linux/skmsg.h
> @@ -283,6 +283,15 @@ static inline void sk_msg_sg_copy(struct sk_msg *msg, u32 i, bool copy_state)
>  	} while (1);
>  }
>  
> +static inline void sk_msg_sg_copy_one(struct sk_msg *dst, u32 dst_i,
> +				      const struct sk_msg *src, u32 src_i)
> +{
> +	if (test_bit(src_i, src->sg.copy))
> +		__set_bit(dst_i, dst->sg.copy);
> +	else
> +		__clear_bit(dst_i, dst->sg.copy);

__assign_bit()?

Also _assign() may be a better suffix for the helper than _one 

> +}
> +
>  static inline void sk_msg_sg_copy_set(struct sk_msg *msg, u32 start)
>  {
>  	sk_msg_sg_copy(msg, start, true);
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 964ebc268ee46..434753de8aadd 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -623,6 +623,7 @@ static int tls_split_open_record(struct sock *sk, struct tls_rec *from,
>  	struct scatterlist *sge, *osge, *nsge;
>  	u32 orig_size = msg_opl->sg.size;
>  	struct scatterlist tmp = { };
> +	u32 tmp_i = NR_MSG_FRAG_IDS;
>  	struct sk_msg *msg_npl;
>  	struct tls_rec *new;
>  	int ret;
> @@ -644,6 +645,7 @@ static int tls_split_open_record(struct sock *sk, struct tls_rec *from,
>  		if (sge->length > apply) {
>  			u32 len = sge->length - apply;
>  
> +			tmp_i = i;
>  			get_page(sg_page(sge));
>  			sg_set_page(&tmp, sg_page(sge), len,
>  				    sge->offset + apply);
> @@ -675,6 +677,10 @@ static int tls_split_open_record(struct sock *sk, struct tls_rec *from,
>  	nsge = sk_msg_elem(msg_npl, j);
>  	if (tmp.length) {
>  		memcpy(nsge, &tmp, sizeof(*nsge));
> +		if (WARN_ON_ONCE(tmp_i == NR_MSG_FRAG_IDS))
> +			__clear_bit(j, msg_npl->sg.copy);
> +		else
> +			sk_msg_sg_copy_one(msg_npl, j, msg_opl, tmp_i);

Not immediately obvious from the diff or commit message why this
special handling is needed.

>  		sk_msg_iter_var_next(j);
>  		nsge = sk_msg_elem(msg_npl, j);
>  	}
> @@ -682,6 +688,7 @@ static int tls_split_open_record(struct sock *sk, struct tls_rec *from,
>  	osge = sk_msg_elem(msg_opl, i);
>  	while (osge->length) {
>  		memcpy(nsge, osge, sizeof(*nsge));
> +		sk_msg_sg_copy_one(msg_npl, j, msg_opl, i);
>  		sg_unmark_end(nsge);
>  		sk_msg_iter_var_next(i);
>  		sk_msg_iter_var_next(j);

This patch looks a little short. I have a recollection of trying to fix
this and giving up after realizing how big the diff would be just to fix
all the cases within skmsg. Could you look harder for places that
shuffle skmsg entries around, not just the ones in TLS? Maybe I'm
misremembering but I thought there was a lot more of them...