Re: [PATCH net v2] tls: restrict device-down TX fallback to hardware TX sockets

[Rishikesh Jethwani]

On Thu, Oct 29, 2026 at 6:52 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 (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).
>
> - [High] The patch fixes two NETDEV_DOWN/setsockopt windows.
>
> Pre-existing issues:
> - [High] This bug was not introduced by this patch, but it is a use-
>   after-free write that stays reachable for hardware-TX sockets.
>
> --
>
> tls: restrict device-down TX fallback to hardware TX sockets
>
> With this patch, tls_device_down() installs tls_validate_xmit_skb_sw()
> only on sockets that have hardware TX offload. This fixes a NULL
> dereference on RX-only sockets. The patch also sets tx_conf/rx_conf to
> TLS_HW and publishes the HW validator under device_offload_lock, so
> tls_device_down() sees a consistent state and releases the driver
> contexts.
>
> > diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> > index 1a5f328d80a50..9e0863dd2f96f 100644
> > --- a/net/tls/tls_device.c
> > +++ b/net/tls/tls_device.c
> > @@ -1176,13 +1176,15 @@ int tls_set_device_offload(struct sock *sk)
> >               goto release_lock;
> >
> >       tls_device_attach(ctx, sk, netdev);
> > -     up_read(&device_offload_lock);
> >
> >       /* following this assignment tls_is_skb_tx_device_offloaded
> >        * will return true and the context might be accessed
> > -      * by the netdev's xmit function.
> > +      * by the netdev's xmit function. Publish it and tx_conf under
> > +      * device_offload_lock so tls_device_down() sees both or neither.
> >        */
> >       smp_store_release(&sk->sk_validate_xmit_skb, tls_validate_xmit_skb);
> > +     ctx->tx_conf = TLS_HW;
> > +     up_read(&device_offload_lock);
>
> [Severity: High]
> Is there still a third path that leaves the leaked driver contexts and the
> stale HW validator that the commit message describes?
>
> Take a socket where one direction is offloaded first, the device goes
> down and comes back up, and then the other direction is offloaded:
>
>   setsockopt(TLS_RX) -> tls_set_device_offload_rx()
>     tls_device_attach()   /* sk_destruct = tls_device_sk_destruct */
>
>   NETDEV_DOWN -> tls_device_down()
>     rcu_assign_pointer(ctx->netdev, NULL);
>     list_move_tail(&ctx->list, &tls_device_down_list);
>     /* sk->sk_destruct is left as is */
>
>   NETDEV_UP
>
>   setsockopt(TLS_TX) -> tls_set_device_offload()
>     tls_dev_add(TX) succeeds
>     tls_device_attach()   /* no-op */
>     smp_store_release(&sk->sk_validate_xmit_skb, tls_validate_xmit_skb);
>     ctx->tx_conf = TLS_HW;
>
> tls_device_attach() only checks this:
>
>         if (sk->sk_destruct != tls_device_sk_destruct) {
>
> So ctx->netdev stays NULL, no dev_hold() is taken, and the ctx stays off
> tls_device_list.
>
> Neither tls_set_device_offload() nor tls_set_device_offload_rx() looks at
> ctx->netdev before calling tls_dev_add(). They only check priv_ctx_* and
> IFF_UP. The same state can also happen inside a single TX setsockopt, if
> the down/up falls between get_netdev_for_sock() and
> down_read(&device_offload_lock).
>
> The result is sk_validate_xmit_skb == tls_validate_xmit_skb with
> ctx->netdev == NULL. The commit message describes that state as a
> regression.
>
> In tls_validate_xmit_skb(), the device never equals NULL, so every data
> skb goes through tls_sw_fallback(). complete_skb() then sets
> nskb->sk = sk, so tls_is_skb_tx_device_offloaded() still returns true.
>
> mlx5e_ktls_handle_tx_skb() accepts a NULL tls_ctx->netdev:
>
>         if (WARN_ON_ONCE(tls_netdev && tls_netdev != netdev))
>                 goto err_out;
>
> Would the NIC then apply inline TLS to records that the software fallback
> already encrypted, and corrupt the stream? GCM encryption is a keystream
> XOR, so a second pass might even send plaintext on the wire. That part
> has not been verified.
>
> At teardown, tls_device_queue_ctx_destruction() computes:
>
>         async_cleanup = netdev && ctx->tx_conf == TLS_HW;
>
> That is false here. tls_dev_del(TX) is never called, and the offload ctx
> that holds driver_state is freed. Later NETDEV_DOWNs don't see the ctx
> either, because it is on tls_device_down_list.
>
> Does this leak the driver TX context?
>
> The RX mirror looks similar: TX HW offload, ifdown, ifup, then
> setsockopt(TLS_RX). tls_set_device_offload_rx() sets rx_conf = TLS_HW on
> a ctx whose ctx->netdev is NULL. tls_device_offload_cleanup_rx() skips
> tls_dev_del() when ctx->netdev is NULL.
>
> Does this leak the driver RX context?
>
> On mlx5, mlx5e_ktls_add_rx() stores priv_rx->sk without taking a
> reference, and async work dereferences it later. A use-after-free may
> therefore be possible here (not verified).
>
> Also, with the new tx_hw gate in tls_device_down(), RX-only sockets no
> longer crash on their first transmit after a down/up cycle. That makes
> the RX-first sequence above easy to reach.
>
> Could tls_set_device_offload() and tls_set_device_offload_rx() check,
> under device_offload_lock and before tls_dev_add(), whether the ctx is
> already device-attached with rcu_dereference_protected(ctx->netdev) !=
> netdev? That would include the NULL that tls_device_down() leaves behind.
> In that case they could refuse hardware offload and fall back to
> software. The other option is to fully re-attach the ctx.

This series applies on top of 1c8a70075235 ("tls: keep TX and RX
hardware offload on the same device"), which is already in net. That
commit adds tls_device_can_attach(), which both tls_set_device_offload()
and tls_set_device_offload_rx() call under device_offload_lock before
tls_dev_add(). Once tls_device_sk_destruct is installed, it refuses
hardware offload unless ctx->netdev matches the current netdev. That
includes the NULL left by tls_device_down(). The requested direction
then falls back to software, so neither ordering reaches the state you
describe. Without that commit, the sequence was possible before this
patch too.

The window inside a single setsockopt only matters when another
direction is already attached, which is the same case. For the first
direction the ctx is not on tls_device_list yet, so tls_device_down()
never touches it, and the later attach goes to an up device.

> [ ... ]
>
> > @@ -1326,10 +1333,17 @@ static int tls_device_down(struct net_device *netdev)
> >       spin_unlock_irqrestore(&tls_device_lock, flags);
> >
> >       list_for_each_entry_safe(ctx, tmp, &list, list) {
> > +             bool tx_hw = ctx->tx_conf == TLS_HW;
> > +
> >               /* Stop offloaded TX and switch to the fallback.
> >                * tls_is_skb_tx_device_offloaded will return false.
> > +              * Only HW TX sockets have the context the fallback needs.
> > +              * The single read above keeps the fallback and tls_dev_del()
> > +              * below in agreement.
> >                */
> > -             WRITE_ONCE(ctx->sk->sk_validate_xmit_skb, tls_validate_xmit_skb_sw);
> > +             if (tx_hw)
> > +                     WRITE_ONCE(ctx->sk->sk_validate_xmit_skb,
> > +                                tls_validate_xmit_skb_sw);
>
> [Severity: High]
> This isn't a bug introduced by this patch, but can this write land in a
> socket that has already been freed?
>
> tls_device_down() only pins the tls_context:
>
>         if (ctx_netdev != netdev ||
>             !refcount_inc_not_zero(&ctx->refcount))
>                 continue;
>
> It then drops tls_device_lock. For every context after the first, the
> WRITE_ONCE() through ctx->sk runs after one synchronize_net() for each
> earlier context.
>
> Meanwhile, the socket's final sk_free() can run:
>
>   __sk_destruct()
>     tls_device_sk_destruct()
>       tls_device_queue_ctx_destruction()
>         refcount 2 -> 1, returns early
>     sk_prot_free()
>
> The TX-only close and destruct paths don't take device_offload_lock, and
> tls_device_down() holds no socket reference. So the socket can be freed
> while the ctx is still alive.
>
> The TCP slab is SLAB_TYPESAFE_BY_RCU, so by then the memory may belong to
> a new, unrelated TCP socket. That socket would get
> tls_validate_xmit_skb_sw installed and call tls_sw_fallback() with a NULL
> or foreign icsk_ulp_data.
>
> The patch keeps this ctx->sk dereference for every context with
> tx_conf == TLS_HW.

Yes. This bug has existed since c55dcdd435aa ("net/tls: Fix
use-after-free after the TLS device goes down and up"). That commit
started pinning only the tls_context and dropping tls_device_lock before
the per-context loop. The refcount_dec_and_test() branch at the end of
the loop already expects sk_destruct to run in the middle of it.

The sockets that can hit this have TX offloaded in hardware: their close
and destruct paths take no device_offload_lock. Closing a socket with RX
in hardware blocks in tls_device_offload_cleanup_rx() behind down_write
until tls_device_down() finishes. This patch limits the write to the
tx_conf == TLS_HW sockets, which are exactly the ones that can hit it,
but the write is still unsafe for them.

v3 will add a fix that installs the fallback validator in the first
loop, while tls_device_lock is held. If a context is still on
tls_device_list and refcount_inc_not_zero() succeeds there,
tls_device_queue_ctx_destruction() has not yet run its locked section.
It takes the same lock before __sk_destruct() frees the socket, so
ctx->sk is valid while the lock is held. The second loop keeps its
tx_conf == TLS_HW check for tls_dev_del(TX). This patch now sets tx_conf
to TLS_HW only under device_offload_lock, which tls_device_down() holds
for writing, so both checks agree. That is also why the fix goes on top
of this patch.

While checking the tls_dev_del() callbacks for the same problem, I found
that chcr_ktls_dev_del() releases its IPv6 CLIP entry through
tx_info->sk->sk_v6_rcv_saddr. That read happens in this window, and also
on normal close, where tls_device_tx_del_task() can run after the socket
is freed. I'll send a separate ch_ktls fix that stores the address at
tls_dev_add() time.

pw-bot: cr

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