[PATCH net] tls: only install push_pending_record for a TX context

[PATCH net] tls: only install push_pending_record for a TX context

[Rishikesh Jethwani]

tls_set_sw_offload() assigns ctx->push_pending_record for both
directions, but the hook is a TX-side dispatcher. On a socket whose TX
side is HW offloaded, tls_set_device_offload() installs
tls_device_push_pending_record(); any later TLS_RX setsockopt overwrites
it with tls_sw_push_pending_record(), because tls_set_device_offload_rx()
itself calls tls_set_sw_offload(sk, 0, NULL) for the software half of a
successful HW RX offload, and so does the fallback when the RX offload
is declined (NIC with TX-only offload, tls_dev_add() failure).

From then on tls_handle_open_record() (a TLS_SET_RECORD_TYPE cmsg after
a MSG_MORE send, or the close path with a pending writer) runs
tls_sw_push_pending_record() against priv_ctx_tx, which is a
struct tls_offload_context_tx, not a struct tls_sw_context_tx, and
reads open_rec from inside the offload context. On x86_64 without
lock debugging that field overlays sg_tx_data[2].dma_address,
which is never set, so the push is silently skipped and the data
buffered by the MSG_MORE send is later sent under the cmsg's
record type, e.g. application data in a handshake
or alert record. With a layout where the field lands on a non-zero
value it is dereferenced and fed to bpf_exec_tx_verdict().

Only set the hook when initialising the TX side.

Found by code review while developing TLS 1.3 HW offload
rekey support.

Tested with HW TLS TX and RX offload on Mellanox ConnectX-6 Dx
(Crypto Enabled): after setting TLS_RX,
send() "AAAA" with MSG_MORE, then sendmsg() "BBBB" with a
TLS_SET_RECORD_TYPE cmsg for a handshake record. Without this patch
the peer receives a single handshake record "AAAABBBB"; with it, an
application data record "AAAA" followed by a handshake record "BBBB".

Fixes: e8f69799810c ("net/tls: Add generic NIC offload infrastructure")
Signed-off-by: Rishikesh Jethwani <rjethwani@purestorage.com>
---
This is independent of the TLS 1.3 HW offload KeyUpdate series, but
that series' TX rekey path (tls_device_start_rekey()) calls
ctx->push_pending_record() and depends on this fix.
---
 net/tls/tls_sw.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index d1ad31986cf2..b3fe153c3c68 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -2591,7 +2591,12 @@ int tls_set_sw_offload(struct sock *sk, int tx,
 		}
 	}
 
-	ctx->push_pending_record = tls_sw_push_pending_record;
+	/* The RX path shares tls_context with a TX side that may be HW
+	 * offloaded and rely on tls_device_push_pending_record(); only the TX
+	 * setup owns this hook.
+	 */
+	if (tx)
+		ctx->push_pending_record = tls_sw_push_pending_record;
 
 	/* setkey is the last operation that could fail during a
 	 * rekey. if it succeeds, we can start modifying the
-- 
2.50.1

Re: [PATCH net] tls: only install push_pending_record for a TX context

[Sabrina Dubroca]

2026-10-15, 22:26:04 -0600, Rishikesh Jethwani wrote:
> tls_set_sw_offload() assigns ctx->push_pending_record for both
> directions, but the hook is a TX-side dispatcher. On a socket whose TX
> side is HW offloaded, tls_set_device_offload() installs
> tls_device_push_pending_record(); any later TLS_RX setsockopt overwrites
> it with tls_sw_push_pending_record(), because tls_set_device_offload_rx()
> itself calls tls_set_sw_offload(sk, 0, NULL) for the software half of a
> successful HW RX offload, and so does the fallback when the RX offload
> is declined (NIC with TX-only offload, tls_dev_add() failure).

This makes sense. Some comments:

> From then on tls_handle_open_record() (a TLS_SET_RECORD_TYPE cmsg after
> a MSG_MORE send, or the close path with a pending writer) runs
> tls_sw_push_pending_record() against priv_ctx_tx, which is a
> struct tls_offload_context_tx, not a struct tls_sw_context_tx, and
> reads open_rec from inside the offload context. On x86_64 without
> lock debugging that field overlays sg_tx_data[2].dma_address,
> which is never set, so the push is silently skipped and the data

Do we have arches that support TLS offload but use dma_address, or
where the struct layout would allow something more harmful than "wrong
payload"? I'm worried that we could access some random address for the
open record.

But on its own, the wrong payload within a TLS stream seems bad
enough, I think it's worth a Cc: stable tag.

> buffered by the MSG_MORE send is later sent under the cmsg's
> record type, e.g. application data in a handshake
> or alert record. With a layout where the field lands on a non-zero
> value it is dereferenced and fed to bpf_exec_tx_verdict().
> 
> Only set the hook when initialising the TX side.
> 
> Found by code review while developing TLS 1.3 HW offload
> rekey support.
> 
> Tested with HW TLS TX and RX offload on Mellanox ConnectX-6 Dx
> (Crypto Enabled): after setting TLS_RX,
> send() "AAAA" with MSG_MORE, then sendmsg() "BBBB" with a
> TLS_SET_RECORD_TYPE cmsg for a handshake record. Without this patch
> the peer receives a single handshake record "AAAABBBB"; with it, an
> application data record "AAAA" followed by a handshake record "BBBB".

If we already had selftests for offload, I'd request a selftest for
this.

Would it be feasible to extract the non-1.3/non-rekey bits of the
selftests you have in your 1.3 offload series? Those would be valuable
on their own.

> Fixes: e8f69799810c ("net/tls: Add generic NIC offload infrastructure")
> Signed-off-by: Rishikesh Jethwani <rjethwani@purestorage.com>
> ---
> This is independent of the TLS 1.3 HW offload KeyUpdate series, but
> that series' TX rekey path (tls_device_start_rekey()) calls
> ctx->push_pending_record() and depends on this fix.
> ---
>  net/tls/tls_sw.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index d1ad31986cf2..b3fe153c3c68 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -2591,7 +2591,12 @@ int tls_set_sw_offload(struct sock *sk, int tx,
>  		}
>  	}
>  
> -	ctx->push_pending_record = tls_sw_push_pending_record;
> +	/* The RX path shares tls_context with a TX side that may be HW
> +	 * offloaded and rely on tls_device_push_pending_record(); only the TX
> +	 * setup owns this hook.
> +	 */

Please, no. If I see "if (tx) install_push_callback;", I don't wonder
why we install the callback or why we install it only for TX.

> +	if (tx)
> +		ctx->push_pending_record = tls_sw_push_pending_record;

-- 
Sabrina

Re: [PATCH net] tls: only install push_pending_record for a TX context

[Rishikesh Jethwani]

On Fri, Oct 16, 2026 at 2:58 AM Sabrina Dubroca <sd@queasysnail.net> wrote:
>
> 2026-10-15, 22:26:04 -0600, Rishikesh Jethwani wrote:
> > tls_set_sw_offload() assigns ctx->push_pending_record for both
> > directions, but the hook is a TX-side dispatcher. On a socket whose TX
> > side is HW offloaded, tls_set_device_offload() installs
> > tls_device_push_pending_record(); any later TLS_RX setsockopt overwrites
> > it with tls_sw_push_pending_record(), because tls_set_device_offload_rx()
> > itself calls tls_set_sw_offload(sk, 0, NULL) for the software half of a
> > successful HW RX offload, and so does the fallback when the RX offload
> > is declined (NIC with TX-only offload, tls_dev_add() failure).
>
> This makes sense. Some comments:
>
> > From then on tls_handle_open_record() (a TLS_SET_RECORD_TYPE cmsg after
> > a MSG_MORE send, or the close path with a pending writer) runs
> > tls_sw_push_pending_record() against priv_ctx_tx, which is a
> > struct tls_offload_context_tx, not a struct tls_sw_context_tx, and
> > reads open_rec from inside the offload context. On x86_64 without
> > lock debugging that field overlays sg_tx_data[2].dma_address,
> > which is never set, so the push is silently skipped and the data
>
> Do we have arches that support TLS offload but use dma_address, or
> where the struct layout would allow something more harmful than "wrong
> payload"? I'm worried that we could access some random address for the
> open record.

  The more harmful case is possible on x86_64 as well. sg_tx_data is
  initialized with sg_init_table() and filled by tls_push_record() for
  tls_push_sg(); it is never DMA-mapped, so I did not find an
  architecture-specific writer of its .dma_address fields.

  I checked the layouts using the compiler and this tree's headers. In the
  tested x86_64 configuration, open_rec is at offset 144, sg_tx_data starts
  at 64 and sizeof(struct scatterlist) is 32, so open_rec overlays
  sg_tx_data[2].dma_address (80 = 2 * 32 + 16), which stays zero and the
  push is skipped. Enabling CONFIG_DEBUG_SPINLOCK, CONFIG_DEBUG_LOCK_ALLOC
  and CONFIG_LOCKDEP (with CONFIG_LOCK_STAT disabled) moves the offsets to
  280 and 120, so open_rec overlaps sg_tx_data[5].page_link (160 = 5 * 32).
  Entries are not cleared between records, so once an earlier record on
  the socket had at least six frags, that page pointer plus scatterlist
  flag bits is interpreted as a struct tls_rec pointer. The first invalid
  dereference is msg_plaintext.sg.size, before bpf_exec_tx_verdict(). So
  the address is not random but a struct page pointer with flag bits; a
  non-zero value read there would make bpf_exec_tx_verdict() operate on a
  bogus record. I have not reproduced a kernel fault or established
  exploitability; the offsets and field writes are confirmed from the build
  and code.

> But on its own, the wrong payload within a TLS stream seems bad
> enough, I think it's worth a Cc: stable tag.
>
> > buffered by the MSG_MORE send is later sent under the cmsg's
> > record type, e.g. application data in a handshake
> > or alert record. With a layout where the field lands on a non-zero
> > value it is dereferenced and fed to bpf_exec_tx_verdict().
> >
> > Only set the hook when initialising the TX side.
> >
> > Found by code review while developing TLS 1.3 HW offload
> > rekey support.
> >
> > Tested with HW TLS TX and RX offload on Mellanox ConnectX-6 Dx
> > (Crypto Enabled): after setting TLS_RX,
> > send() "AAAA" with MSG_MORE, then sendmsg() "BBBB" with a
> > TLS_SET_RECORD_TYPE cmsg for a handshake record. Without this patch
> > the peer receives a single handshake record "AAAABBBB"; with it, an
> > application data record "AAAA" followed by a handshake record "BBBB".
>
> If we already had selftests for offload, I'd request a selftest for
> this.
>
> Would it be feasible to extract the non-1.3/non-rekey bits of the
> selftests you have in your 1.3 offload series? Those would be valuable
> on their own.

Thanks for the suggestion. I’ll submit v2 with the non-1.3, non-rekey portions
of the selftests extracted as standalone selftests.

> > Fixes: e8f69799810c ("net/tls: Add generic NIC offload infrastructure")
> > Signed-off-by: Rishikesh Jethwani <rjethwani@purestorage.com>
> > ---
> > This is independent of the TLS 1.3 HW offload KeyUpdate series, but
> > that series' TX rekey path (tls_device_start_rekey()) calls
> > ctx->push_pending_record() and depends on this fix.
> > ---
> >  net/tls/tls_sw.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> > index d1ad31986cf2..b3fe153c3c68 100644
> > --- a/net/tls/tls_sw.c
> > +++ b/net/tls/tls_sw.c
> > @@ -2591,7 +2591,12 @@ int tls_set_sw_offload(struct sock *sk, int tx,
> >               }
> >       }
> >
> > -     ctx->push_pending_record = tls_sw_push_pending_record;
> > +     /* The RX path shares tls_context with a TX side that may be HW
> > +      * offloaded and rely on tls_device_push_pending_record(); only the TX
> > +      * setup owns this hook.
> > +      */
>
> Please, no. If I see "if (tx) install_push_callback;", I don't wonder
> why we install the callback or why we install it only for TX.

I'll add Cc: stable@vger.kernel.org and remove the comment

> > +     if (tx)
> > +             ctx->push_pending_record = tls_sw_push_pending_record;
>
> --
> Sabrina