net/x25: Fix null-ptr-deref in x25_connect

PATCH RESEND Applied Martin Schiller 3 participants 4 messages Nov 9, 2020 – Nov 11, 2020

Subsystems (3)

Networking Network drivers X.25 stack

lore

Share & tools

lore

Martin Schiller

author maintainer

2020-11-09 06:54 UTC #0 lore raw

PATCH RESEND v2 1/1 net/x25: Fix null-ptr-deref in x25_connect

This fixes a regression for blocking connects introduced by commit 4becb7ee5b3d ("net/x25: Fix x25_neigh refcnt leak when x25 disconnect").

The x25->neighbour is already set to "NULL" by x25_disconnect() now, while a blocking connect is waiting in x25_wait_for_connection_establishment(). Therefore x25->neighbour must not be accessed here again and x25->state is also already set to X25_STATE_0 by x25_disconnect().

Fixes 4becb7ee5b3d ("net/x25: Fix x25_neigh refcnt leak when x25 disconnect")

Signed-off-by Martin Schiller <[email protected]>

---

Change from v1:
also handle interrupting signals correctly

---
 net/x25/af_x25.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/x25/af_x25.c +1 -1

index 0bbb283f23c9..046d3fee66a9 100644
@@ -825,7 +825,7 @@ static int x25_connect(struct socket *sock, struct sockaddr *uaddr,
 	sock->state = SS_CONNECTED;
 	rc = 0;
 out_put_neigh:
-	if (rc) {
+	if (rc && x25->neighbour) {
 		read_lock_bh(&x25_list_lock);
 		x25_neigh_put(x25->neighbour);
 		x25->neighbour = NULL;

Signature · 1 line

2.20.1

Xie He

2020-11-11 11:38 UTC #1 lore raw

Quoted 11 lines

This fixes a regression for blocking connects introduced by commit 4becb7ee5b3d ("net/x25: Fix x25_neigh refcnt leak when x25 disconnect").
The x25->neighbour is already set to "NULL" by x25_disconnect() now, while a blocking connect is waiting in x25_wait_for_connection_establishment(). Therefore x25->neighbour must not be accessed here again and x25->state is also already set to X25_STATE_0 by x25_disconnect().
Fixes 4becb7ee5b3d ("net/x25: Fix x25_neigh refcnt leak when x25 disconnect")
Signed-off-by Martin Schiller <[email protected]>

Oh. Sorry, I didn't see your patch. I just submitted another patch to fix the same problem.

I also found another problem introduced by the same regression commit, which I was also trying to fix in my patch.

See:
http://patchwork.ozlabs.org/project/netdev/patch/[email protected]/

Xie He

Reviewed-by on v2 1/1

2020-11-11 11:59 UTC #2 lore raw

In reply to Martin Schiller

net/x25/af_x25.c static int x25_connect(struct socket *sock, struct sockaddr *uaddr, · line 831

Quoted 9 lines

net/x25/af_x25.c +1 -1

@@ -825,7 +825,7 @@  static int x25_connect(struct socket *sock, struct sockaddr *uaddr,
 	sock->state = SS_CONNECTED;
 	rc = 0;
 out_put_neigh:
-	if (rc) {
+	if (rc && x25->neighbour) {
 		read_lock_bh(&x25_list_lock);
 		x25_neigh_put(x25->neighbour);
 		x25->neighbour = NULL;

Thanks! It's amazing to see we are trying to fix the same issue.

Reviewed-by Xie He <[email protected]>

Jakub Kicinski

maintainer Applied

2020-11-11 22:57 UTC #3 lore raw

Quoted 11 lines · On Wed, 11 Nov 2020 03:59:47 -0800 Xie He wrote:

net/x25/af_x25.c static int x25_connect(struct socket *sock, struct sockaddr *uaddr, · line 831
Quoted 9 lines
net/x25/af_x25.c +1 -1
@@ -825,7 +825,7 @@  static int x25_connect(struct socket *sock, struct sockaddr *uaddr,
 	sock->state = SS_CONNECTED;
 	rc = 0;
 out_put_neigh:
-	if (rc) {
+	if (rc && x25->neighbour) {
 		read_lock_bh(&x25_list_lock);
 		x25_neigh_put(x25->neighbour);
 		x25->neighbour = NULL;  
Reviewed-by Xie He <[email protected]>

Applied, thanks!