[PATCH net-next 1/2] tls: rx: release the sock lock on locking timeout

[PATCH net-next 1/2] tls: rx: release the sock lock on locking timeout

[Jakub Kicinski]

Eric reports we should release the socket lock if the entire
"grab reader lock" operation has failed. The callers assume
they don't have to release it or otherwise unwind.

Reported-by: Eric Dumazet <edumazet@google.com>
Reported-by: syzbot+16e72110feb2b653ef27@syzkaller.appspotmail.com
Fixes: 4cbc325ed6b4 ("tls: rx: allow only one reader at a time")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/tls/tls_sw.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 518401997539..0fc24a5ce208 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1846,6 +1846,7 @@ static long tls_rx_reader_lock(struct sock *sk, struct tls_sw_context_rx *ctx,
 			       bool nonblock)
 {
 	long timeo;
+	int err;
 
 	lock_sock(sk);
 
@@ -1861,15 +1862,23 @@ static long tls_rx_reader_lock(struct sock *sk, struct tls_sw_context_rx *ctx,
 			      !READ_ONCE(ctx->reader_present), &wait);
 		remove_wait_queue(&ctx->wq, &wait);
 
-		if (!timeo)
-			return -EAGAIN;
-		if (signal_pending(current))
-			return sock_intr_errno(timeo);
+		if (timeo <= 0) {
+			err = -EAGAIN;
+			goto err_unlock;
+		}
+		if (signal_pending(current)) {
+			err = sock_intr_errno(timeo);
+			goto err_unlock;
+		}
 	}
 
 	WRITE_ONCE(ctx->reader_present, 1);
 
 	return timeo;
+
+err_unlock:
+	release_sock(sk);
+	return err;
 }
 
 static void tls_rx_reader_unlock(struct sock *sk, struct tls_sw_context_rx *ctx)
-- 
2.36.1

[PATCH net-next 2/2] selftests: tls: add a test for timeo vs lock

[Jakub Kicinski]

Add a test for recv timeout. Place it in the tls_err
group, so it only runs for TLS 1.2 and 1.3 but not
for every AEAD out there.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/net/tls.c | 32 +++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index 6ada4c28c188..2cbb12736596 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -1605,6 +1605,38 @@ TEST_F(tls_err, bad_cmsg)
 	EXPECT_EQ(errno, EBADMSG);
 }
 
+TEST_F(tls_err, timeo)
+{
+	struct timeval tv = { .tv_usec = 10000, };
+	char buf[128];
+	int ret;
+
+	if (self->notls)
+		SKIP(return, "no TLS support");
+
+	ret = setsockopt(self->cfd2, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv));
+	ASSERT_EQ(ret, 0);
+
+	ret = fork();
+	ASSERT_GE(ret, 0);
+
+	if (ret) {
+		usleep(1000); /* Give child a head start */
+
+		EXPECT_EQ(recv(self->cfd2, buf, sizeof(buf), 0), -1);
+		EXPECT_EQ(errno, EAGAIN);
+
+		EXPECT_EQ(recv(self->cfd2, buf, sizeof(buf), 0), -1);
+		EXPECT_EQ(errno, EAGAIN);
+
+		wait(&ret);
+	} else {
+		EXPECT_EQ(recv(self->cfd2, buf, sizeof(buf), 0), -1);
+		EXPECT_EQ(errno, EAGAIN);
+		exit(0);
+	}
+}
+
 TEST(non_established) {
 	struct tls12_crypto_info_aes_gcm_256 tls12;
 	struct sockaddr_in addr;
-- 
2.36.1

Re: [PATCH net-next 1/2] tls: rx: release the sock lock on locking timeout

[Eric Dumazet]

On Wed, Jul 20, 2022 at 10:37 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Eric reports we should release the socket lock if the entire
> "grab reader lock" operation has failed. The callers assume
> they don't have to release it or otherwise unwind.
>
> Reported-by: Eric Dumazet <edumazet@google.com>
> Reported-by: syzbot+16e72110feb2b653ef27@syzkaller.appspotmail.com
> Fixes: 4cbc325ed6b4 ("tls: rx: allow only one reader at a time")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  net/tls/tls_sw.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next 1/2] tls: rx: release the sock lock on locking timeout

[patchwork-bot+netdevbpf]

Hello:

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

On Wed, 20 Jul 2022 13:37:00 -0700 you wrote:
> Eric reports we should release the socket lock if the entire
> "grab reader lock" operation has failed. The callers assume
> they don't have to release it or otherwise unwind.
> 
> Reported-by: Eric Dumazet <edumazet@google.com>
> Reported-by: syzbot+16e72110feb2b653ef27@syzkaller.appspotmail.com
> Fixes: 4cbc325ed6b4 ("tls: rx: allow only one reader at a time")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> 
> [...]

Here is the summary with links:
  - [net-next,1/2] tls: rx: release the sock lock on locking timeout
    https://git.kernel.org/netdev/net-next/c/dde06aaa89b7
  - [net-next,2/2] selftests: tls: add a test for timeo vs lock
    https://git.kernel.org/netdev/net-next/c/842463f253ab

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