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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx and remove the comment

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