Re: [PATCH net v2] tls: keep TX and RX hardware offload on the same device

[Rishikesh Jethwani]

On Sat, Oct 24, 2026 at 3:02 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 1 · High: 0 (1) · Medium: 0 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Critical] tls: type confusion and UAF in tls_device_down() when TX is
>   not TLS_HW
>
> Pre-existing issues:
> - [High] tls_set_sw_offload() (net/tls/tls_sw.c:2594) sets the shared
>   ctx->push_pending_record = tls_sw_push_pending_record unconditionally,…
>
> --
>
> tls: keep TX and RX hardware offload on the same device
>
> This patch adds tls_device_can_attach(), which is checked under
> device_offload_lock before each tls_dev_add() in tls_set_device_offload()
> and tls_set_device_offload_rx(). A second direction is only offloaded if
> it resolves to the netdev already bound to the context; otherwise setup
> falls back to software for that direction.
>
> > diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> > index f11d0528fc431..9df183d0dd972 100644
> > --- a/net/tls/tls_device.c
> > +++ b/net/tls/tls_device.c
>
> [ ... ]
>
> > @@ -1227,6 +1246,11 @@ int tls_set_device_offload_rx(struct sock *sk, struct tls_context *ctx)
> >               goto release_lock;
> >       }
> >
> > +     if (!tls_device_can_attach(ctx, sk, netdev)) {
> > +             rc = -EOPNOTSUPP;
> > +             goto release_lock;
> > +     }
> > +
>
> [Severity: Critical]
> This isn't introduced by this patch, but the patch now deliberately
> steers sockets into the state that hits it, so it seems worth raising
> here.
>
> When RX is on hardware and TX is declined by tls_device_can_attach(),
> do_tls_setsockopt_conf() falls back to TLS_SW for TX (or TX is simply
> never configured and stays TLS_BASE), while ctx stays on
> tls_device_list bound to the RX device. If that device later goes down,
> tls_device_down() does this for every matching ctx:
>
>         WRITE_ONCE(ctx->sk->sk_validate_xmit_skb,
>                    tls_validate_xmit_skb_sw);
>
> without looking at ctx->tx_conf. The tls_dev_del() calls further down
> are guarded by tx_conf == TLS_HW and rx_conf == TLS_HW, but this store
> is not.
>
> >From then on every skb with TCP payload on the socket goes through
> validate_xmit_skb() -> tls_validate_xmit_skb_sw() -> tls_sw_fallback(),
> which casts tls_ctx->priv_ctx_tx with tls_offload_ctx_tx() and passes
> the result to fill_sg_in(), which takes ctx->lock and walks
> ctx->records_list through tls_get_record().
>
> With tx_conf == TLS_BASE, priv_ctx_tx is NULL. Isn't this a NULL
> pointer dereference on the first data segment sent after the device
> goes down? With tx_conf == TLS_SW, priv_ctx_tx is the struct
> tls_sw_context_tx from init_ctx_tx(), so fill_sg_in() would take a
> spinlock and walk a list that are really unrelated fields of that
> structure.
>
> Close looks worse still. tls_sk_proto_close() frees priv_ctx_tx via
> tls_sw_free_ctx_tx() before calling ctx->sk_proto->close(), but with
> rx_conf == TLS_HW the tls_context and the sk_validate_xmit_skb hook stay
> around until tls_device_sk_destruct(). Data pushed out by tcp_close()
> or retransmitted afterwards would run tls_sw_fallback() on freed
> memory.
>
> The updated tls-offload.rst says a declined direction "uses software
> instead", and NETDEV_DOWN clearing the binding is now an explicit
> reason to decline TX, so the patch relies on this combination working.
>
> Should tls_device_down() only switch sk_validate_xmit_skb when TX is
> really device offloaded? ctx->tx_conf is only set by
> do_tls_setsockopt_conf() after tls_set_device_offload() has dropped
> device_offload_lock, so I'm not sure tx_conf alone is a reliable test
> there. Would comparing against tls_validate_xmit_skb, or having
> tls_validate_xmit_skb_sw() bail out when TX is not TLS_HW, work better?

Thanks, agreed this is real, and it predates this patch.
It's fixed separately in "tls: restrict device-down TX fallback to
hardware TX sockets"
https://lore.kernel.org/netdev/20261027015205.2169253-1-rjethwani@xxxxxxxxxxxxxxx/
That patch publishes tls_validate_xmit_skb and sets tx_conf = TLS_HW before
tls_set_device_offload() drops device_offload_lock. tls_device_down()
then switches
to tls_validate_xmit_skb_sw, and calls tls_dev_del(TX), only when
tx_conf is TLS_HW,
so software-TX and RX-only sockets keep their validator unchanged.
It also sets rx_conf under the lock to close the matching RX leak.
Since neither kind of socket gets the fallback validator, the
close-path use-after-free
you describe can't happen either.

> [Severity: High]
> This isn't a bug introduced by this patch, but it is related to the new
> tls-offload.rst wording that a declined direction "is handled in software
> as if offload had never been tried".
>
> Suppose TX is already on hardware and RX is rejected here. Then
> do_tls_setsockopt_conf() falls back to tls_set_sw_offload(sk, 0, ...),
> and that function sets the shared TX callback whatever the direction:
>
> net/tls/tls_sw.c:tls_set_sw_offload() {
>     ...
>         ctx->push_pending_record = tls_sw_push_pending_record;
>     ...
> }
>
> This replaces the callback that tls_set_device_offload() installed:
>
>         ctx->push_pending_record = tls_device_push_pending_record;
>
> The hardware RX success path also calls tls_set_sw_offload(sk, 0, NULL),
> so this could already happen before this patch.
>
> Now suppose device TX leaves an open record, for example because
> sendmsg() with MSG_MORE set pending_open_record_frags. A later sendmsg()
> with a TLS_SET_RECORD_TYPE cmsg then takes this path:
>
> tls_device_sendmsg()
>   tls_process_cmsg()
>     tls_handle_open_record()
>       ctx->push_pending_record() -> tls_sw_push_pending_record()
>
> tls_sk_proto_cleanup() reaches the same callback on close when
> sk_write_pending is set. tls_sw_push_pending_record() does:
>
>         struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
>         struct tls_rec *rec = ctx->open_rec;
>
> but priv_ctx_tx is really a struct tls_offload_context_tx here.
>
> Doesn't this read an unrelated field of tls_offload_context_tx as a
> struct tls_rec pointer?
>
> If that field is NULL, the open device record is not flushed before the
> record type changes. Data that is already buffered then goes out under
> the new record type. If the field is non-NULL, rec->msg_plaintext is
> read through a garbage pointer.
>
> This seems to go back to 3c4d7559159b (2017). Could tls_set_sw_offload()
> set push_pending_record only when tx is set? Then RX setup, hardware or
> software fallback, would leave the device TX callback in place.

Thanks, agreed this is real, and it predates this patch.
It's fixed separately in "tls: only install push_pending_record for a
TX context"
https://lore.kernel.org/netdev/20261027171537.2317025-1-rjethwani@xxxxxxxxxxxxxxx/

> [ ... ]
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020214500.972308-1-rjethwani%40purestorage.com