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

[Rishikesh Jethwani <rjethwani@purestorage.com>]

tls_device_down() installs tls_validate_xmit_skb_sw() on every socket
offloaded 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 tls_sw_context_tx would likewise be walked as a
hardware context.

Only install the fallback when tx_conf is TLS_HW. For that check to be
reliable, tls_set_device_offload() must publish the validator and
tx_conf before dropping device_offload_lock. Today it drops the lock
first, and do_tls_setsockopt_conf() sets tx_conf later, under lock_sock
only. A NETDEV_DOWN in that window sees TLS_BASE: with the new check it
would leave the hardware validator in place with ctx->netdev cleared,
and it already skips tls_dev_del(TX), leaking the driver TX context,
since the destructor only queues it while ctx->netdev is set. 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 under the lock in tls_set_device_offload_rx() too.

Also do the switch in the first loop, under tls_device_lock. The second
loop runs after that lock is dropped, and its reference pins the
tls_context but not ctx->sk. A socket with only TX offloaded can be
closed meanwhile: tls_device_queue_ctx_destruction() drops its reference
and returns early, __sk_destruct() frees the socket, and the write lands
in freed SLAB_TYPESAFE_BY_RCU memory. A context whose refcount can still
be raised under tls_device_lock has not been through
tls_device_queue_ctx_destruction(), which takes that lock, so ctx->sk is
valid there. Later contexts now switch before earlier ones are torn
down, which the fallback allows.

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, the setsockopt/NETDEV_DOWN races and the
use-after-free are from code inspection.

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 | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index 4cdfbb57ecff..8cfad667c43c 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c
@@ -1195,13 +1195,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. Set 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;
@@ -1282,6 +1284,10 @@ int tls_set_device_offload_rx(struct sock *sk, struct tls_context *ctx)
 		goto free_sw_resources;
 
 	tls_device_attach(ctx, sk, netdev);
+	/* Set under device_offload_lock so tls_device_down() also
+	 * releases the driver RX context.
+	 */
+	ctx->rx_conf = TLS_HW;
 	up_read(&device_offload_lock);
 
 	dev_put(netdev);
@@ -1345,16 +1351,20 @@ static int tls_device_down(struct net_device *netdev)
 		    !refcount_inc_not_zero(&ctx->refcount))
 			continue;
 
+		/* Stop offloaded TX and switch to the fallback.
+		 * tls_is_skb_tx_device_offloaded will return false.
+		 * Only HW TX sockets have the context it needs. Do it under
+		 * tls_device_lock: the reference pins ctx, not ctx->sk.
+		 */
+		if (ctx->tx_conf == TLS_HW)
+			WRITE_ONCE(ctx->sk->sk_validate_xmit_skb,
+				   tls_validate_xmit_skb_sw);
+
 		list_move(&ctx->list, &list);
 	}
 	spin_unlock_irqrestore(&tls_device_lock, flags);
 
 	list_for_each_entry_safe(ctx, tmp, &list, list)	{
-		/* Stop offloaded TX and switch to the fallback.
-		 * tls_is_skb_tx_device_offloaded will return false.
-		 */
-		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.
 		 */
-- 
2.50.1