All of lore.kernel.org
 help / color / mirror / Atom feed
* [Patch net] ax25: fix a use-after-free in ax25_fillin_cb()
@ 2018-12-29 21:56 Cong Wang
  2018-12-29 21:56 ` [Patch net] net/wan: fix a double free in x25_asy_open_tty() Cong Wang
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Cong Wang @ 2018-12-29 21:56 UTC (permalink / raw)
  To: netdev; +Cc: Cong Wang

There are multiple issues here:

1. After freeing dev->ax25_ptr, we need to set it to NULL otherwise
   we may use a dangling pointer.

2. There is a race between ax25_setsockopt() and device notifier as
   reported by syzbot. Close it by holding RTNL lock.

3. We need to test if dev->ax25_ptr is NULL before using it.

Reported-and-tested-by: syzbot+ae6bb869cbed29b29040@syzkaller.appspotmail.com
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/ax25/af_ax25.c  | 11 +++++++++--
 net/ax25/ax25_dev.c |  2 ++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index c603d33d5410..5d01edf8d819 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -653,15 +653,22 @@ static int ax25_setsockopt(struct socket *sock, int level, int optname,
 			break;
 		}
 
-		dev = dev_get_by_name(&init_net, devname);
+		rtnl_lock();
+		dev = __dev_get_by_name(&init_net, devname);
 		if (!dev) {
+			rtnl_unlock();
 			res = -ENODEV;
 			break;
 		}
 
 		ax25->ax25_dev = ax25_dev_ax25dev(dev);
+		if (!ax25->ax25_dev) {
+			rtnl_unlock();
+			res = -ENODEV;
+			break;
+		}
 		ax25_fillin_cb(ax25, ax25->ax25_dev);
-		dev_put(dev);
+		rtnl_unlock();
 		break;
 
 	default:
diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
index 9a3a301e1e2f..d92195cd7834 100644
--- a/net/ax25/ax25_dev.c
+++ b/net/ax25/ax25_dev.c
@@ -116,6 +116,7 @@ void ax25_dev_device_down(struct net_device *dev)
 	if ((s = ax25_dev_list) == ax25_dev) {
 		ax25_dev_list = s->next;
 		spin_unlock_bh(&ax25_dev_lock);
+		dev->ax25_ptr = NULL;
 		dev_put(dev);
 		kfree(ax25_dev);
 		return;
@@ -125,6 +126,7 @@ void ax25_dev_device_down(struct net_device *dev)
 		if (s->next == ax25_dev) {
 			s->next = ax25_dev->next;
 			spin_unlock_bh(&ax25_dev_lock);
+			dev->ax25_ptr = NULL;
 			dev_put(dev);
 			kfree(ax25_dev);
 			return;
-- 
2.19.2

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Patch net] net/wan: fix a double free in x25_asy_open_tty()
  2018-12-29 21:56 [Patch net] ax25: fix a use-after-free in ax25_fillin_cb() Cong Wang
@ 2018-12-29 21:56 ` Cong Wang
  2018-12-31  4:23   ` David Miller
  2018-12-29 21:56 ` [Patch net] netrom: fix locking in nr_find_socket() Cong Wang
  2018-12-30 22:08 ` [Patch net] ax25: fix a use-after-free in ax25_fillin_cb() David Miller
  2 siblings, 1 reply; 6+ messages in thread
From: Cong Wang @ 2018-12-29 21:56 UTC (permalink / raw)
  To: netdev; +Cc: Cong Wang

When x25_asy_open() fails, it already cleans up by itself,
so its caller doesn't need to free the memory again.

It seems we still have to call x25_asy_free() to clear the SLF_INUSE
bit, so just set these pointers to NULL after kfree().

Reported-and-tested-by: syzbot+5e5e969e525129229052@syzkaller.appspotmail.com
Fixes: 3b780bed3138 ("x25_asy: Free x25_asy on x25_asy_open() failure.")
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 drivers/net/wan/x25_asy.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/wan/x25_asy.c b/drivers/net/wan/x25_asy.c
index 1098263ab862..46c3d983b7b7 100644
--- a/drivers/net/wan/x25_asy.c
+++ b/drivers/net/wan/x25_asy.c
@@ -485,8 +485,10 @@ static int x25_asy_open(struct net_device *dev)
 
 	/* Cleanup */
 	kfree(sl->xbuff);
+	sl->xbuff = NULL;
 noxbuff:
 	kfree(sl->rbuff);
+	sl->rbuff = NULL;
 norbuff:
 	return -ENOMEM;
 }
-- 
2.19.2

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Patch net] netrom: fix locking in nr_find_socket()
  2018-12-29 21:56 [Patch net] ax25: fix a use-after-free in ax25_fillin_cb() Cong Wang
  2018-12-29 21:56 ` [Patch net] net/wan: fix a double free in x25_asy_open_tty() Cong Wang
@ 2018-12-29 21:56 ` Cong Wang
  2018-12-31  4:24   ` David Miller
  2018-12-30 22:08 ` [Patch net] ax25: fix a use-after-free in ax25_fillin_cb() David Miller
  2 siblings, 1 reply; 6+ messages in thread
From: Cong Wang @ 2018-12-29 21:56 UTC (permalink / raw)
  To: netdev; +Cc: Cong Wang

nr_find_socket(), nr_find_peer() and nr_find_listener() lock the
sock after finding it in the global list. However, the call path
requires BH disabled for the sock lock consistently.

Actually the locking is unnecessary at this point, we can just hold
the sock refcnt to make sure it is not gone after we unlock the global
list, and lock it later only when needed.

Reported-and-tested-by: syzbot+f621cda8b7e598908efa@syzkaller.appspotmail.com
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/netrom/af_netrom.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/net/netrom/af_netrom.c b/net/netrom/af_netrom.c
index 03f37c4e64fe..1d3144d19903 100644
--- a/net/netrom/af_netrom.c
+++ b/net/netrom/af_netrom.c
@@ -153,7 +153,7 @@ static struct sock *nr_find_listener(ax25_address *addr)
 	sk_for_each(s, &nr_list)
 		if (!ax25cmp(&nr_sk(s)->source_addr, addr) &&
 		    s->sk_state == TCP_LISTEN) {
-			bh_lock_sock(s);
+			sock_hold(s);
 			goto found;
 		}
 	s = NULL;
@@ -174,7 +174,7 @@ static struct sock *nr_find_socket(unsigned char index, unsigned char id)
 		struct nr_sock *nr = nr_sk(s);
 
 		if (nr->my_index == index && nr->my_id == id) {
-			bh_lock_sock(s);
+			sock_hold(s);
 			goto found;
 		}
 	}
@@ -198,7 +198,7 @@ static struct sock *nr_find_peer(unsigned char index, unsigned char id,
 
 		if (nr->your_index == index && nr->your_id == id &&
 		    !ax25cmp(&nr->dest_addr, dest)) {
-			bh_lock_sock(s);
+			sock_hold(s);
 			goto found;
 		}
 	}
@@ -224,7 +224,7 @@ static unsigned short nr_find_next_circuit(void)
 		if (i != 0 && j != 0) {
 			if ((sk=nr_find_socket(i, j)) == NULL)
 				break;
-			bh_unlock_sock(sk);
+			sock_put(sk);
 		}
 
 		id++;
@@ -920,6 +920,7 @@ int nr_rx_frame(struct sk_buff *skb, struct net_device *dev)
 	}
 
 	if (sk != NULL) {
+		bh_lock_sock(sk);
 		skb_reset_transport_header(skb);
 
 		if (frametype == NR_CONNACK && skb->len == 22)
@@ -929,6 +930,7 @@ int nr_rx_frame(struct sk_buff *skb, struct net_device *dev)
 
 		ret = nr_process_rx_frame(sk, skb);
 		bh_unlock_sock(sk);
+		sock_put(sk);
 		return ret;
 	}
 
@@ -960,10 +962,12 @@ int nr_rx_frame(struct sk_buff *skb, struct net_device *dev)
 	    (make = nr_make_new(sk)) == NULL) {
 		nr_transmit_refusal(skb, 0);
 		if (sk)
-			bh_unlock_sock(sk);
+			sock_put(sk);
 		return 0;
 	}
 
+	bh_lock_sock(sk);
+
 	window = skb->data[20];
 
 	skb->sk             = make;
@@ -1016,6 +1020,7 @@ int nr_rx_frame(struct sk_buff *skb, struct net_device *dev)
 		sk->sk_data_ready(sk);
 
 	bh_unlock_sock(sk);
+	sock_put(sk);
 
 	nr_insert_socket(make);
 
-- 
2.19.2

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [Patch net] ax25: fix a use-after-free in ax25_fillin_cb()
  2018-12-29 21:56 [Patch net] ax25: fix a use-after-free in ax25_fillin_cb() Cong Wang
  2018-12-29 21:56 ` [Patch net] net/wan: fix a double free in x25_asy_open_tty() Cong Wang
  2018-12-29 21:56 ` [Patch net] netrom: fix locking in nr_find_socket() Cong Wang
@ 2018-12-30 22:08 ` David Miller
  2 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2018-12-30 22:08 UTC (permalink / raw)
  To: xiyou.wangcong; +Cc: netdev

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Sat, 29 Dec 2018 13:56:36 -0800

> There are multiple issues here:
> 
> 1. After freeing dev->ax25_ptr, we need to set it to NULL otherwise
>    we may use a dangling pointer.
> 
> 2. There is a race between ax25_setsockopt() and device notifier as
>    reported by syzbot. Close it by holding RTNL lock.
> 
> 3. We need to test if dev->ax25_ptr is NULL before using it.
> 
> Reported-and-tested-by: syzbot+ae6bb869cbed29b29040@syzkaller.appspotmail.com
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied and queued up for -stable, thanks Cong.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Patch net] net/wan: fix a double free in x25_asy_open_tty()
  2018-12-29 21:56 ` [Patch net] net/wan: fix a double free in x25_asy_open_tty() Cong Wang
@ 2018-12-31  4:23   ` David Miller
  0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2018-12-31  4:23 UTC (permalink / raw)
  To: xiyou.wangcong; +Cc: netdev

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Sat, 29 Dec 2018 13:56:37 -0800

> When x25_asy_open() fails, it already cleans up by itself,
> so its caller doesn't need to free the memory again.
> 
> It seems we still have to call x25_asy_free() to clear the SLF_INUSE
> bit, so just set these pointers to NULL after kfree().
> 
> Reported-and-tested-by: syzbot+5e5e969e525129229052@syzkaller.appspotmail.com
> Fixes: 3b780bed3138 ("x25_asy: Free x25_asy on x25_asy_open() failure.")
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied and queued up for -stable, thanks Cong.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Patch net] netrom: fix locking in nr_find_socket()
  2018-12-29 21:56 ` [Patch net] netrom: fix locking in nr_find_socket() Cong Wang
@ 2018-12-31  4:24   ` David Miller
  0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2018-12-31  4:24 UTC (permalink / raw)
  To: xiyou.wangcong; +Cc: netdev

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Sat, 29 Dec 2018 13:56:38 -0800

> nr_find_socket(), nr_find_peer() and nr_find_listener() lock the
> sock after finding it in the global list. However, the call path
> requires BH disabled for the sock lock consistently.
> 
> Actually the locking is unnecessary at this point, we can just hold
> the sock refcnt to make sure it is not gone after we unlock the global
> list, and lock it later only when needed.
> 
> Reported-and-tested-by: syzbot+f621cda8b7e598908efa@syzkaller.appspotmail.com
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied and queued up for -stable.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-12-31  4:25 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-29 21:56 [Patch net] ax25: fix a use-after-free in ax25_fillin_cb() Cong Wang
2018-12-29 21:56 ` [Patch net] net/wan: fix a double free in x25_asy_open_tty() Cong Wang
2018-12-31  4:23   ` David Miller
2018-12-29 21:56 ` [Patch net] netrom: fix locking in nr_find_socket() Cong Wang
2018-12-31  4:24   ` David Miller
2018-12-30 22:08 ` [Patch net] ax25: fix a use-after-free in ax25_fillin_cb() David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.