diff mbox series

net/handshake: Fix sock->file allocation

Message ID 168451609436.45209.15407022385441542980.stgit@oracle-102.nfsv4bat.org (mailing list archive)
State Accepted
Commit 18c40a1cc1d990c51381ef48cd93fdb31d5cd903
Delegated to: Netdev Maintainers
Headers show
Series net/handshake: Fix sock->file allocation | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers warning 4 maintainers not CCed: kuba@kernel.org edumazet@google.com davem@davemloft.net pabeni@redhat.com
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 129 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Chuck Lever May 19, 2023, 5:08 p.m. UTC 
From: Chuck Lever <chuck.lever@oracle.com>

	sock->file = sock_alloc_file(sock, O_NONBLOCK, NULL);
	^^^^                         ^^^^

sock_alloc_file() calls release_sock() on error but the left hand
side of the assignment dereferences "sock".  This isn't the bug and
I didn't report this earlier because there is an assert that it
doesn't fail.

net/handshake/handshake-test.c:221 handshake_req_submit_test4() error: dereferencing freed memory 'sock'
net/handshake/handshake-test.c:233 handshake_req_submit_test4() warn: 'req' was already freed.
net/handshake/handshake-test.c:254 handshake_req_submit_test5() error: dereferencing freed memory 'sock'
net/handshake/handshake-test.c:290 handshake_req_submit_test6() error: dereferencing freed memory 'sock'
net/handshake/handshake-test.c:321 handshake_req_cancel_test1() error: dereferencing freed memory 'sock'
net/handshake/handshake-test.c:355 handshake_req_cancel_test2() error: dereferencing freed memory 'sock'
net/handshake/handshake-test.c:367 handshake_req_cancel_test2() warn: 'req' was already freed.
net/handshake/handshake-test.c:395 handshake_req_cancel_test3() error: dereferencing freed memory 'sock'
net/handshake/handshake-test.c:407 handshake_req_cancel_test3() warn: 'req' was already freed.
net/handshake/handshake-test.c:451 handshake_req_destroy_test1() error: dereferencing freed memory 'sock'
net/handshake/handshake-test.c:463 handshake_req_destroy_test1() warn: 'req' was already freed.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/handshake/handshake-test.c |   42 +++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 14 deletions(-)

Comments

Chuck Lever May 19, 2023, 5:13 p.m. UTC | #1 
> On May 19, 2023, at 1:08 PM, Chuck Lever <cel@kernel.org> wrote:
> 
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> sock->file = sock_alloc_file(sock, O_NONBLOCK, NULL);
> ^^^^                         ^^^^
> 
> sock_alloc_file() calls release_sock() on error but the left hand
> side of the assignment dereferences "sock".  This isn't the bug and
> I didn't report this earlier because there is an assert that it
> doesn't fail.
> 
> net/handshake/handshake-test.c:221 handshake_req_submit_test4() error: dereferencing freed memory 'sock'
> net/handshake/handshake-test.c:233 handshake_req_submit_test4() warn: 'req' was already freed.
> net/handshake/handshake-test.c:254 handshake_req_submit_test5() error: dereferencing freed memory 'sock'
> net/handshake/handshake-test.c:290 handshake_req_submit_test6() error: dereferencing freed memory 'sock'
> net/handshake/handshake-test.c:321 handshake_req_cancel_test1() error: dereferencing freed memory 'sock'
> net/handshake/handshake-test.c:355 handshake_req_cancel_test2() error: dereferencing freed memory 'sock'
> net/handshake/handshake-test.c:367 handshake_req_cancel_test2() warn: 'req' was already freed.
> net/handshake/handshake-test.c:395 handshake_req_cancel_test3() error: dereferencing freed memory 'sock'
> net/handshake/handshake-test.c:407 handshake_req_cancel_test3() warn: 'req' was already freed.
> net/handshake/handshake-test.c:451 handshake_req_destroy_test1() error: dereferencing freed memory 'sock'
> net/handshake/handshake-test.c:463 handshake_req_destroy_test1() warn: 'req' was already freed.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>

Fixes: 88232ec1ec5e ("net/handshake: Add Kunit tests for the handshake consumer API")

> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> net/handshake/handshake-test.c |   42 +++++++++++++++++++++++++++-------------
> 1 file changed, 28 insertions(+), 14 deletions(-)
> 
> diff --git a/net/handshake/handshake-test.c b/net/handshake/handshake-test.c
> index e6adc5dec11a..daa1779063ed 100644
> --- a/net/handshake/handshake-test.c
> +++ b/net/handshake/handshake-test.c
> @@ -209,6 +209,7 @@ static void handshake_req_submit_test4(struct kunit *test)
> {
> struct handshake_req *req, *result;
> struct socket *sock;
> + struct file *filp;
> int err;
> 
> /* Arrange */
> @@ -218,9 +219,10 @@ static void handshake_req_submit_test4(struct kunit *test)
> err = __sock_create(&init_net, PF_INET, SOCK_STREAM, IPPROTO_TCP,
>    &sock, 1);
> KUNIT_ASSERT_EQ(test, err, 0);
> - sock->file = sock_alloc_file(sock, O_NONBLOCK, NULL);
> - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, sock->file);
> + filp = sock_alloc_file(sock, O_NONBLOCK, NULL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filp);
> KUNIT_ASSERT_NOT_NULL(test, sock->sk);
> + sock->file = filp;
> 
> err = handshake_req_submit(sock, req, GFP_KERNEL);
> KUNIT_ASSERT_EQ(test, err, 0);
> @@ -241,6 +243,7 @@ static void handshake_req_submit_test5(struct kunit *test)
> struct handshake_req *req;
> struct handshake_net *hn;
> struct socket *sock;
> + struct file *filp;
> struct net *net;
> int saved, err;
> 
> @@ -251,9 +254,10 @@ static void handshake_req_submit_test5(struct kunit *test)
> err = __sock_create(&init_net, PF_INET, SOCK_STREAM, IPPROTO_TCP,
>    &sock, 1);
> KUNIT_ASSERT_EQ(test, err, 0);
> - sock->file = sock_alloc_file(sock, O_NONBLOCK, NULL);
> - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, sock->file);
> + filp = sock_alloc_file(sock, O_NONBLOCK, NULL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filp);
> KUNIT_ASSERT_NOT_NULL(test, sock->sk);
> + sock->file = filp;
> 
> net = sock_net(sock->sk);
> hn = handshake_pernet(net);
> @@ -276,6 +280,7 @@ static void handshake_req_submit_test6(struct kunit *test)
> {
> struct handshake_req *req1, *req2;
> struct socket *sock;
> + struct file *filp;
> int err;
> 
> /* Arrange */
> @@ -287,9 +292,10 @@ static void handshake_req_submit_test6(struct kunit *test)
> err = __sock_create(&init_net, PF_INET, SOCK_STREAM, IPPROTO_TCP,
>    &sock, 1);
> KUNIT_ASSERT_EQ(test, err, 0);
> - sock->file = sock_alloc_file(sock, O_NONBLOCK, NULL);
> - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, sock->file);
> + filp = sock_alloc_file(sock, O_NONBLOCK, NULL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filp);
> KUNIT_ASSERT_NOT_NULL(test, sock->sk);
> + sock->file = filp;
> 
> /* Act */
> err = handshake_req_submit(sock, req1, GFP_KERNEL);
> @@ -307,6 +313,7 @@ static void handshake_req_cancel_test1(struct kunit *test)
> {
> struct handshake_req *req;
> struct socket *sock;
> + struct file *filp;
> bool result;
> int err;
> 
> @@ -318,8 +325,9 @@ static void handshake_req_cancel_test1(struct kunit *test)
>    &sock, 1);
> KUNIT_ASSERT_EQ(test, err, 0);
> 
> - sock->file = sock_alloc_file(sock, O_NONBLOCK, NULL);
> - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, sock->file);
> + filp = sock_alloc_file(sock, O_NONBLOCK, NULL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filp);
> + sock->file = filp;
> 
> err = handshake_req_submit(sock, req, GFP_KERNEL);
> KUNIT_ASSERT_EQ(test, err, 0);
> @@ -340,6 +348,7 @@ static void handshake_req_cancel_test2(struct kunit *test)
> struct handshake_req *req, *next;
> struct handshake_net *hn;
> struct socket *sock;
> + struct file *filp;
> struct net *net;
> bool result;
> int err;
> @@ -352,8 +361,9 @@ static void handshake_req_cancel_test2(struct kunit *test)
>    &sock, 1);
> KUNIT_ASSERT_EQ(test, err, 0);
> 
> - sock->file = sock_alloc_file(sock, O_NONBLOCK, NULL);
> - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, sock->file);
> + filp = sock_alloc_file(sock, O_NONBLOCK, NULL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filp);
> + sock->file = filp;
> 
> err = handshake_req_submit(sock, req, GFP_KERNEL);
> KUNIT_ASSERT_EQ(test, err, 0);
> @@ -380,6 +390,7 @@ static void handshake_req_cancel_test3(struct kunit *test)
> struct handshake_req *req, *next;
> struct handshake_net *hn;
> struct socket *sock;
> + struct file *filp;
> struct net *net;
> bool result;
> int err;
> @@ -392,8 +403,9 @@ static void handshake_req_cancel_test3(struct kunit *test)
>    &sock, 1);
> KUNIT_ASSERT_EQ(test, err, 0);
> 
> - sock->file = sock_alloc_file(sock, O_NONBLOCK, NULL);
> - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, sock->file);
> + filp = sock_alloc_file(sock, O_NONBLOCK, NULL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filp);
> + sock->file = filp;
> 
> err = handshake_req_submit(sock, req, GFP_KERNEL);
> KUNIT_ASSERT_EQ(test, err, 0);
> @@ -436,6 +448,7 @@ static void handshake_req_destroy_test1(struct kunit *test)
> {
> struct handshake_req *req;
> struct socket *sock;
> + struct file *filp;
> int err;
> 
> /* Arrange */
> @@ -448,8 +461,9 @@ static void handshake_req_destroy_test1(struct kunit *test)
>    &sock, 1);
> KUNIT_ASSERT_EQ(test, err, 0);
> 
> - sock->file = sock_alloc_file(sock, O_NONBLOCK, NULL);
> - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, sock->file);
> + filp = sock_alloc_file(sock, O_NONBLOCK, NULL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filp);
> + sock->file = filp;
> 
> err = handshake_req_submit(sock, req, GFP_KERNEL);
> KUNIT_ASSERT_EQ(test, err, 0);
> 
> 

--
Chuck Lever
patchwork-bot+netdevbpf@kernel.org May 23, 2023, 2:40 a.m. UTC | #2 
Hello:

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

On Fri, 19 May 2023 13:08:24 -0400 you wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> 	sock->file = sock_alloc_file(sock, O_NONBLOCK, NULL);
> 	^^^^                         ^^^^
> 
> sock_alloc_file() calls release_sock() on error but the left hand
> side of the assignment dereferences "sock".  This isn't the bug and
> I didn't report this earlier because there is an assert that it
> doesn't fail.
> 
> [...]

Here is the summary with links:
  - net/handshake: Fix sock->file allocation
    https://git.kernel.org/netdev/net/c/18c40a1cc1d9

You are awesome, thank you!
diff mbox series

Patch

diff --git a/net/handshake/handshake-test.c b/net/handshake/handshake-test.c
index e6adc5dec11a..daa1779063ed 100644
--- a/net/handshake/handshake-test.c
+++ b/net/handshake/handshake-test.c
@@ -209,6 +209,7 @@  static void handshake_req_submit_test4(struct kunit *test)
 {
 	struct handshake_req *req, *result;
 	struct socket *sock;
+	struct file *filp;
 	int err;
 
 	/* Arrange */
@@ -218,9 +219,10 @@  static void handshake_req_submit_test4(struct kunit *test)
 	err = __sock_create(&init_net, PF_INET, SOCK_STREAM, IPPROTO_TCP,
 			    &sock, 1);
 	KUNIT_ASSERT_EQ(test, err, 0);
-	sock->file = sock_alloc_file(sock, O_NONBLOCK, NULL);
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, sock->file);
+	filp = sock_alloc_file(sock, O_NONBLOCK, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filp);
 	KUNIT_ASSERT_NOT_NULL(test, sock->sk);
+	sock->file = filp;
 
 	err = handshake_req_submit(sock, req, GFP_KERNEL);
 	KUNIT_ASSERT_EQ(test, err, 0);
@@ -241,6 +243,7 @@  static void handshake_req_submit_test5(struct kunit *test)
 	struct handshake_req *req;
 	struct handshake_net *hn;
 	struct socket *sock;
+	struct file *filp;
 	struct net *net;
 	int saved, err;
 
@@ -251,9 +254,10 @@  static void handshake_req_submit_test5(struct kunit *test)
 	err = __sock_create(&init_net, PF_INET, SOCK_STREAM, IPPROTO_TCP,
 			    &sock, 1);
 	KUNIT_ASSERT_EQ(test, err, 0);
-	sock->file = sock_alloc_file(sock, O_NONBLOCK, NULL);
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, sock->file);
+	filp = sock_alloc_file(sock, O_NONBLOCK, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filp);
 	KUNIT_ASSERT_NOT_NULL(test, sock->sk);
+	sock->file = filp;
 
 	net = sock_net(sock->sk);
 	hn = handshake_pernet(net);
@@ -276,6 +280,7 @@  static void handshake_req_submit_test6(struct kunit *test)
 {
 	struct handshake_req *req1, *req2;
 	struct socket *sock;
+	struct file *filp;
 	int err;
 
 	/* Arrange */
@@ -287,9 +292,10 @@  static void handshake_req_submit_test6(struct kunit *test)
 	err = __sock_create(&init_net, PF_INET, SOCK_STREAM, IPPROTO_TCP,
 			    &sock, 1);
 	KUNIT_ASSERT_EQ(test, err, 0);
-	sock->file = sock_alloc_file(sock, O_NONBLOCK, NULL);
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, sock->file);
+	filp = sock_alloc_file(sock, O_NONBLOCK, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filp);
 	KUNIT_ASSERT_NOT_NULL(test, sock->sk);
+	sock->file = filp;
 
 	/* Act */
 	err = handshake_req_submit(sock, req1, GFP_KERNEL);
@@ -307,6 +313,7 @@  static void handshake_req_cancel_test1(struct kunit *test)
 {
 	struct handshake_req *req;
 	struct socket *sock;
+	struct file *filp;
 	bool result;
 	int err;
 
@@ -318,8 +325,9 @@  static void handshake_req_cancel_test1(struct kunit *test)
 			    &sock, 1);
 	KUNIT_ASSERT_EQ(test, err, 0);
 
-	sock->file = sock_alloc_file(sock, O_NONBLOCK, NULL);
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, sock->file);
+	filp = sock_alloc_file(sock, O_NONBLOCK, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filp);
+	sock->file = filp;
 
 	err = handshake_req_submit(sock, req, GFP_KERNEL);
 	KUNIT_ASSERT_EQ(test, err, 0);
@@ -340,6 +348,7 @@  static void handshake_req_cancel_test2(struct kunit *test)
 	struct handshake_req *req, *next;
 	struct handshake_net *hn;
 	struct socket *sock;
+	struct file *filp;
 	struct net *net;
 	bool result;
 	int err;
@@ -352,8 +361,9 @@  static void handshake_req_cancel_test2(struct kunit *test)
 			    &sock, 1);
 	KUNIT_ASSERT_EQ(test, err, 0);
 
-	sock->file = sock_alloc_file(sock, O_NONBLOCK, NULL);
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, sock->file);
+	filp = sock_alloc_file(sock, O_NONBLOCK, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filp);
+	sock->file = filp;
 
 	err = handshake_req_submit(sock, req, GFP_KERNEL);
 	KUNIT_ASSERT_EQ(test, err, 0);
@@ -380,6 +390,7 @@  static void handshake_req_cancel_test3(struct kunit *test)
 	struct handshake_req *req, *next;
 	struct handshake_net *hn;
 	struct socket *sock;
+	struct file *filp;
 	struct net *net;
 	bool result;
 	int err;
@@ -392,8 +403,9 @@  static void handshake_req_cancel_test3(struct kunit *test)
 			    &sock, 1);
 	KUNIT_ASSERT_EQ(test, err, 0);
 
-	sock->file = sock_alloc_file(sock, O_NONBLOCK, NULL);
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, sock->file);
+	filp = sock_alloc_file(sock, O_NONBLOCK, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filp);
+	sock->file = filp;
 
 	err = handshake_req_submit(sock, req, GFP_KERNEL);
 	KUNIT_ASSERT_EQ(test, err, 0);
@@ -436,6 +448,7 @@  static void handshake_req_destroy_test1(struct kunit *test)
 {
 	struct handshake_req *req;
 	struct socket *sock;
+	struct file *filp;
 	int err;
 
 	/* Arrange */
@@ -448,8 +461,9 @@  static void handshake_req_destroy_test1(struct kunit *test)
 			    &sock, 1);
 	KUNIT_ASSERT_EQ(test, err, 0);
 
-	sock->file = sock_alloc_file(sock, O_NONBLOCK, NULL);
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, sock->file);
+	filp = sock_alloc_file(sock, O_NONBLOCK, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filp);
+	sock->file = filp;
 
 	err = handshake_req_submit(sock, req, GFP_KERNEL);
 	KUNIT_ASSERT_EQ(test, err, 0);