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

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

[Rishikesh Jethwani]

tls_device_down() installs tls_validate_xmit_skb_sw() on every socket with
hardware offload in either direction, but tls_sw_fallback() needs a
hardware TX context in priv_ctx_tx. An RX-only socket's NULL priv_ctx_tx
crashes the fallback on its next transmit, for instance after the device
comes back up:

  BUG: kernel NULL pointer dereference, address: 0000000000000008
  RIP: 0010:_raw_spin_lock_irqsave+0x31/0x60
   tls_sw_fallback+0x11b/0x450
   validate_xmit_skb.isra.0+0x14f/0x420
   __dev_queue_xmit+0x5e8/0xf30

A software-TX socket's priv_ctx_tx is a tls_sw_context_tx, which the
fallback would walk as a hardware context.

Only install the fallback on hardware TX sockets. Today tx_conf alone
cannot identify them from tls_device_down(): tls_set_device_offload()
drops device_offload_lock before it publishes the validator, and
do_tls_setsockopt_conf() stores tx_conf = TLS_HW later still, under
lock_sock, which tls_device_down() does not take. That leaves two
windows. A NETDEV_DOWN before the validator store installs the software
fallback, only for the hardware validator to overwrite it with
ctx->netdev already cleared. A NETDEV_DOWN after the validator store
leaves the fallback in place. Both read tx_conf as TLS_BASE and skip
tls_dev_del(TX) today, which the destructor never retries
(tls_device_queue_ctx_destruction() only queues it while ctx->netdev is
set), so the driver TX context leaks. Gating the fallback on tx_conf
alone would make the second window a regression: the hardware validator
would stay in place with ctx->netdev cleared, so
tls_is_skb_tx_device_offloaded() keeps reporting offloaded.

Set tx_conf to TLS_HW in tls_set_device_offload() before dropping
device_offload_lock, and publish the validator there too, so
tls_device_down() sees both or neither. rx_conf has the same window and
leaks the driver RX context, as tls_device_offload_cleanup_rx() skips
tls_dev_del() once ctx->netdev is NULL, so set it the same way in
tls_set_device_offload_rx().

The unconditional fallback install, and its overwrite by the hardware
validator, date from c55dcdd435aa; the TX and RX leaks from 4799ac81e52a.

The NULL dereference was reproduced on an RX-only socket, which now
survives an interface down/up cycle and keeps receiving TLS records. The
software-TX case and the setsockopt/NETDEV_DOWN races are from code
inspection only.

Fixes: c55dcdd435aa ("net/tls: Fix use-after-free after the TLS device goes down and up")
Fixes: 4799ac81e52a ("tls: Add rx inline crypto offload")
Signed-off-by: Rishikesh Jethwani <rjethwani@purestorage.com>
---
 net/tls/tls_device.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index 1a5f328d80a5..9e0863dd2f96 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);
 	dev_put(netdev);
 
 	return 0;
@@ -1258,6 +1260,11 @@ int tls_set_device_offload_rx(struct sock *sk, struct tls_context *ctx)
 		goto free_sw_resources;
 
 	tls_device_attach(ctx, sk, netdev);
+	/* do_tls_setsockopt_conf() stores rx_conf too, but only after
+	 * device_offload_lock is dropped. Set it here so tls_device_down()
+	 * sees TLS_HW and releases the driver RX context.
+	 */
+	ctx->rx_conf = TLS_HW;
 	up_read(&device_offload_lock);
 
 	dev_put(netdev);
@@ -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);
 
 		/* Stop the RX and TX resync.
 		 * tls_dev_resync must not be called after tls_dev_del.
@@ -1346,7 +1360,7 @@ static int tls_device_down(struct net_device *netdev)
 		synchronize_net();
 
 		/* Release the offload context on the driver side. */
-		if (ctx->tx_conf == TLS_HW)
+		if (tx_hw)
 			netdev->tlsdev_ops->tls_dev_del(netdev, ctx,
 							TLS_OFFLOAD_CTX_DIR_TX);
 		if (ctx->rx_conf == TLS_HW &&
-- 
2.50.1

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

[netdev-bot+sashiko]

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.

[ ... ]

> @@ -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.

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