[PATCH net 1/2] tls: rx: device: bound the frag walk

[PATCH net 1/2] tls: rx: device: bound the frag walk

[Jakub Kicinski]

We can't do skb_walk_frags() on the input skbs, because
the input skbs is really just a pointer to the tcp read
queue. We need to bound the "is decrypted" check by the
amount of data in the message.

Note that the walk in tls_device_reencrypt() is after a
CoW so the skb there is safe to walk. Actually in the
current implementation it can't have frags at all, but
whatever, maybe one day it will.

Reported-by: Tariq Toukan <tariqt@nvidia.com>
Fixes: 84c61fe1a75b ("tls: rx: do not use the standard strparser")
Tested-by: Ran Rozenstein <ranro@nvidia.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/tls/tls_device.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index e3e6cf75aa03..6ed41474bdf8 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c
@@ -984,11 +984,17 @@ int tls_device_decrypted(struct sock *sk, struct tls_context *tls_ctx)
 	int is_decrypted = skb->decrypted;
 	int is_encrypted = !is_decrypted;
 	struct sk_buff *skb_iter;
+	int left;
 
+	left = rxm->full_len - skb->len;
 	/* Check if all the data is decrypted already */
-	skb_walk_frags(skb, skb_iter) {
+	skb_iter = skb_shinfo(skb)->frag_list;
+	while (skb_iter && left > 0) {
 		is_decrypted &= skb_iter->decrypted;
 		is_encrypted &= !skb_iter->decrypted;
+
+		left -= skb_iter->len;
+		skb_iter = skb_iter->next;
 	}
 
 	trace_tls_device_decrypted(sk, tcp_sk(sk)->copied_seq - rxm->full_len,
-- 
2.37.1

[PATCH net 2/2] tls: rx: device: don't try to copy too much on detach

[Jakub Kicinski]

Another device offload bug, we use the length of the output
skb as an indication of how much data to copy. But that skb
is sized to offset + record length, and we start from offset.
So we end up double-counting the offset which leads to
skb_copy_bits() returning -EFAULT.

Reported-by: Tariq Toukan <tariqt@nvidia.com>
Fixes: 84c61fe1a75b ("tls: rx: do not use the standard strparser")
Tested-by: Ran Rozenstein <ranro@nvidia.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/tls/tls_strp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tls/tls_strp.c b/net/tls/tls_strp.c
index f0b7c9122fba..9b79e334dbd9 100644
--- a/net/tls/tls_strp.c
+++ b/net/tls/tls_strp.c
@@ -41,7 +41,7 @@ static struct sk_buff *tls_strp_msg_make_copy(struct tls_strparser *strp)
 	struct sk_buff *skb;
 	int i, err, offset;
 
-	skb = alloc_skb_with_frags(0, strp->anchor->len, TLS_PAGE_ORDER,
+	skb = alloc_skb_with_frags(0, strp->stm.full_len, TLS_PAGE_ORDER,
 				   &err, strp->sk->sk_allocation);
 	if (!skb)
 		return NULL;
-- 
2.37.1

Re: [PATCH net 1/2] tls: rx: device: bound the frag walk

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Tue,  9 Aug 2022 10:55:43 -0700 you wrote:
> We can't do skb_walk_frags() on the input skbs, because
> the input skbs is really just a pointer to the tcp read
> queue. We need to bound the "is decrypted" check by the
> amount of data in the message.
> 
> Note that the walk in tls_device_reencrypt() is after a
> CoW so the skb there is safe to walk. Actually in the
> current implementation it can't have frags at all, but
> whatever, maybe one day it will.
> 
> [...]

Here is the summary with links:
  - [net,1/2] tls: rx: device: bound the frag walk
    https://git.kernel.org/netdev/net/c/86b259f6f888
  - [net,2/2] tls: rx: device: don't try to copy too much on detach
    https://git.kernel.org/netdev/net/c/d800a7b3577b

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html