From 38030d7cb77963ba84cdbe034806e2b81245339f Mon Sep 17 00:00:00 2001 From: Jakub Kicinski Date: Tue, 21 May 2019 19:02:00 -0700 Subject: [PATCH 1/3] net/tls: avoid NULL-deref on resync during device removal When netdev with active kTLS sockets in unregistered notifier callback walks the offloaded sockets and cleans up offload state. RX data may still be processed, however, and if resync was requested prior to device removal we would hit a NULL pointer dereference on ctx->netdev use. Make sure resync is under the device offload lock and NULL-check the netdev pointer. This should be safe, because the pointer is set to NULL either in the netdev notifier (under said lock) or when socket is completely dead and no resync can happen. The other access to ctx->netdev in tls_validate_xmit_skb() does not dereference the pointer, it just checks it against other device pointer, so it should be pretty safe (perhaps we can add a READ_ONCE/WRITE_ONCE there, if paranoid). Fixes: 4799ac81e52a ("tls: Add rx inline crypto offload") Signed-off-by: Jakub Kicinski Reviewed-by: Dirk van der Merwe Signed-off-by: David S. Miller --- net/tls/tls_device.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c index ca54a7c7ec81a4..aa33e4accc32ce 100644 --- a/net/tls/tls_device.c +++ b/net/tls/tls_device.c @@ -553,8 +553,8 @@ void tls_device_write_space(struct sock *sk, struct tls_context *ctx) void handle_device_resync(struct sock *sk, u32 seq, u64 rcd_sn) { struct tls_context *tls_ctx = tls_get_ctx(sk); - struct net_device *netdev = tls_ctx->netdev; struct tls_offload_context_rx *rx_ctx; + struct net_device *netdev; u32 is_req_pending; s64 resync_req; u32 req_seq; @@ -568,10 +568,15 @@ void handle_device_resync(struct sock *sk, u32 seq, u64 rcd_sn) is_req_pending = resync_req; if (unlikely(is_req_pending) && req_seq == seq && - atomic64_try_cmpxchg(&rx_ctx->resync_req, &resync_req, 0)) - netdev->tlsdev_ops->tls_dev_resync_rx(netdev, sk, - seq + TLS_HEADER_SIZE - 1, - rcd_sn); + atomic64_try_cmpxchg(&rx_ctx->resync_req, &resync_req, 0)) { + seq += TLS_HEADER_SIZE - 1; + down_read(&device_offload_lock); + netdev = tls_ctx->netdev; + if (netdev) + netdev->tlsdev_ops->tls_dev_resync_rx(netdev, sk, seq, + rcd_sn); + up_read(&device_offload_lock); + } } static int tls_device_reencrypt(struct sock *sk, struct sk_buff *skb) From 3686637e507b48525fcea6fb91e1988bdbc14530 Mon Sep 17 00:00:00 2001 From: Jakub Kicinski Date: Tue, 21 May 2019 19:02:01 -0700 Subject: [PATCH 2/3] net/tls: fix state removal with feature flags off TLS offload drivers shouldn't (and currently don't) block the TLS offload feature changes based on whether there are active offloaded connections or not. This seems to be a good idea, because we want the admin to be able to disable the TLS offload at any time, and there is no clean way of disabling it for active connections (TX side is quite problematic). So if features are cleared existing connections will stay offloaded until they close, and new connections will not attempt offload to a given device. However, the offload state removal handling is currently broken if feature flags get cleared while there are active TLS offloads. RX side will completely bail from cleanup, even on normal remove path, leaving device state dangling, potentially causing issues when the 5-tuple is reused. It will also fail to release the netdev reference. Remove the RX-side warning message, in next release cycle it should be printed when features are disabled, rather than when connection dies, but for that we need a more efficient method of finding connection of a given netdev (a'la BPF offload code). Fixes: 4799ac81e52a ("tls: Add rx inline crypto offload") Signed-off-by: Jakub Kicinski Reviewed-by: Dirk van der Merwe Signed-off-by: David S. Miller --- net/tls/tls_device.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c index aa33e4accc32ce..07650446e8920d 100644 --- a/net/tls/tls_device.c +++ b/net/tls/tls_device.c @@ -939,12 +939,6 @@ void tls_device_offload_cleanup_rx(struct sock *sk) if (!netdev) goto out; - if (!(netdev->features & NETIF_F_HW_TLS_RX)) { - pr_err_ratelimited("%s: device is missing NETIF_F_HW_TLS_RX cap\n", - __func__); - goto out; - } - netdev->tlsdev_ops->tls_dev_del(netdev, tls_ctx, TLS_OFFLOAD_CTX_DIR_RX); From c3f4a6c39cf269a40d45f813c05fa830318ad875 Mon Sep 17 00:00:00 2001 From: Jakub Kicinski Date: Tue, 21 May 2019 19:02:02 -0700 Subject: [PATCH 3/3] net/tls: don't ignore netdev notifications if no TLS features On device surprise removal path (the notifier) we can't bail just because the features are disabled. They may have been enabled during the lifetime of the device. This bug leads to leaking netdev references and use-after-frees if there are active connections while device features are cleared. Fixes: e8f69799810c ("net/tls: Add generic NIC offload infrastructure") Signed-off-by: Jakub Kicinski Reviewed-by: Dirk van der Merwe Signed-off-by: David S. Miller --- net/tls/tls_device.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c index 07650446e8920d..b95c408fd77145 100644 --- a/net/tls/tls_device.c +++ b/net/tls/tls_device.c @@ -997,7 +997,8 @@ static int tls_dev_event(struct notifier_block *this, unsigned long event, { struct net_device *dev = netdev_notifier_info_to_dev(ptr); - if (!(dev->features & (NETIF_F_HW_TLS_RX | NETIF_F_HW_TLS_TX))) + if (!dev->tlsdev_ops && + !(dev->features & (NETIF_F_HW_TLS_RX | NETIF_F_HW_TLS_TX))) return NOTIFY_DONE; switch (event) {