[X,B,D,E,F,SRU,1/1] can, slip: Protect tty->disc_data in write_wakeup and close with RCU
diff mbox series

Message ID 20200206103751.28983-2-po-hsu.lin@canonical.com
State New
Headers show
Series
  • Fix for pty03 test in LTP
Related show

Commit Message

Po-Hsu Lin Feb. 6, 2020, 10:37 a.m. UTC
From: Richard Palethorpe <rpalethorpe@suse.com>

BugLink: https://bugs.launchpad.net/bugs/1862114

write_wakeup can happen in parallel with close/hangup where tty->disc_data
is set to NULL and the netdevice is freed thus also freeing
disc_data. write_wakeup accesses disc_data so we must prevent close from
freeing the netdev while write_wakeup has a non-NULL view of
tty->disc_data.

We also need to make sure that accesses to disc_data are atomic. Which can
all be done with RCU.

This problem was found by Syzkaller on SLCAN, but the same issue is
reproducible with the SLIP line discipline using an LTP test based on the
Syzkaller reproducer.

A fix which didn't use RCU was posted by Hillf Danton.

Fixes: 661f7fda21b1 ("slip: Fix deadlock in write_wakeup")
Fixes: a8e83b17536a ("slcan: Port write_wakeup deadlock fix from slip")
Reported-by: syzbot+017e491ae13c0068598a@syzkaller.appspotmail.com
Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
Cc: Wolfgang Grandegger <wg@grandegger.com>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Tyler Hall <tylerwhall@gmail.com>
Cc: linux-can@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: syzkaller@googlegroups.com
Signed-off-by: David S. Miller <davem@davemloft.net>
(cherry picked from commit 0ace17d56824165c7f4c68785d6b58971db954dd)
Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com>
---
 drivers/net/can/slcan.c | 12 ++++++++++--
 drivers/net/slip/slip.c | 12 ++++++++++--
 2 files changed, 20 insertions(+), 4 deletions(-)

Comments

Thadeu Lima de Souza Cascardo Feb. 6, 2020, 10:42 a.m. UTC | #1
Acked-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Colin King Feb. 6, 2020, 10:52 a.m. UTC | #2
On 06/02/2020 10:37, Po-Hsu Lin wrote:
> From: Richard Palethorpe <rpalethorpe@suse.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1862114
> 
> write_wakeup can happen in parallel with close/hangup where tty->disc_data
> is set to NULL and the netdevice is freed thus also freeing
> disc_data. write_wakeup accesses disc_data so we must prevent close from
> freeing the netdev while write_wakeup has a non-NULL view of
> tty->disc_data.
> 
> We also need to make sure that accesses to disc_data are atomic. Which can
> all be done with RCU.
> 
> This problem was found by Syzkaller on SLCAN, but the same issue is
> reproducible with the SLIP line discipline using an LTP test based on the
> Syzkaller reproducer.
> 
> A fix which didn't use RCU was posted by Hillf Danton.
> 
> Fixes: 661f7fda21b1 ("slip: Fix deadlock in write_wakeup")
> Fixes: a8e83b17536a ("slcan: Port write_wakeup deadlock fix from slip")
> Reported-by: syzbot+017e491ae13c0068598a@syzkaller.appspotmail.com
> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> Cc: Wolfgang Grandegger <wg@grandegger.com>
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Tyler Hall <tylerwhall@gmail.com>
> Cc: linux-can@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: syzkaller@googlegroups.com
> Signed-off-by: David S. Miller <davem@davemloft.net>
> (cherry picked from commit 0ace17d56824165c7f4c68785d6b58971db954dd)
> Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com>
> ---
>  drivers/net/can/slcan.c | 12 ++++++++++--
>  drivers/net/slip/slip.c | 12 ++++++++++--
>  2 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
> index cf0769a..b2e5bca 100644
> --- a/drivers/net/can/slcan.c
> +++ b/drivers/net/can/slcan.c
> @@ -343,9 +343,16 @@ static void slcan_transmit(struct work_struct *work)
>   */
>  static void slcan_write_wakeup(struct tty_struct *tty)
>  {
> -	struct slcan *sl = tty->disc_data;
> +	struct slcan *sl;
> +
> +	rcu_read_lock();
> +	sl = rcu_dereference(tty->disc_data);
> +	if (!sl)
> +		goto out;
>  
>  	schedule_work(&sl->tx_work);
> +out:
> +	rcu_read_unlock();
>  }
>  
>  /* Send a can_frame to a TTY queue. */
> @@ -640,10 +647,11 @@ static void slcan_close(struct tty_struct *tty)
>  		return;
>  
>  	spin_lock_bh(&sl->lock);
> -	tty->disc_data = NULL;
> +	rcu_assign_pointer(tty->disc_data, NULL);
>  	sl->tty = NULL;
>  	spin_unlock_bh(&sl->lock);
>  
> +	synchronize_rcu();
>  	flush_work(&sl->tx_work);
>  
>  	/* Flush network side */
> diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c
> index 2a91c19..61d7e0d 100644
> --- a/drivers/net/slip/slip.c
> +++ b/drivers/net/slip/slip.c
> @@ -452,9 +452,16 @@ static void slip_transmit(struct work_struct *work)
>   */
>  static void slip_write_wakeup(struct tty_struct *tty)
>  {
> -	struct slip *sl = tty->disc_data;
> +	struct slip *sl;
> +
> +	rcu_read_lock();
> +	sl = rcu_dereference(tty->disc_data);
> +	if (!sl)
> +		goto out;
>  
>  	schedule_work(&sl->tx_work);
> +out:
> +	rcu_read_unlock();
>  }
>  
>  static void sl_tx_timeout(struct net_device *dev)
> @@ -882,10 +889,11 @@ static void slip_close(struct tty_struct *tty)
>  		return;
>  
>  	spin_lock_bh(&sl->lock);
> -	tty->disc_data = NULL;
> +	rcu_assign_pointer(tty->disc_data, NULL);
>  	sl->tty = NULL;
>  	spin_unlock_bh(&sl->lock);
>  
> +	synchronize_rcu();
>  	flush_work(&sl->tx_work);
>  
>  	/* VSV = very important to remove timers */
> 

Looks good to me.

Acked-by: Colin Ian King <colin.king@canonical.com>

Patch
diff mbox series

diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index cf0769a..b2e5bca 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -343,9 +343,16 @@  static void slcan_transmit(struct work_struct *work)
  */
 static void slcan_write_wakeup(struct tty_struct *tty)
 {
-	struct slcan *sl = tty->disc_data;
+	struct slcan *sl;
+
+	rcu_read_lock();
+	sl = rcu_dereference(tty->disc_data);
+	if (!sl)
+		goto out;
 
 	schedule_work(&sl->tx_work);
+out:
+	rcu_read_unlock();
 }
 
 /* Send a can_frame to a TTY queue. */
@@ -640,10 +647,11 @@  static void slcan_close(struct tty_struct *tty)
 		return;
 
 	spin_lock_bh(&sl->lock);
-	tty->disc_data = NULL;
+	rcu_assign_pointer(tty->disc_data, NULL);
 	sl->tty = NULL;
 	spin_unlock_bh(&sl->lock);
 
+	synchronize_rcu();
 	flush_work(&sl->tx_work);
 
 	/* Flush network side */
diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c
index 2a91c19..61d7e0d 100644
--- a/drivers/net/slip/slip.c
+++ b/drivers/net/slip/slip.c
@@ -452,9 +452,16 @@  static void slip_transmit(struct work_struct *work)
  */
 static void slip_write_wakeup(struct tty_struct *tty)
 {
-	struct slip *sl = tty->disc_data;
+	struct slip *sl;
+
+	rcu_read_lock();
+	sl = rcu_dereference(tty->disc_data);
+	if (!sl)
+		goto out;
 
 	schedule_work(&sl->tx_work);
+out:
+	rcu_read_unlock();
 }
 
 static void sl_tx_timeout(struct net_device *dev)
@@ -882,10 +889,11 @@  static void slip_close(struct tty_struct *tty)
 		return;
 
 	spin_lock_bh(&sl->lock);
-	tty->disc_data = NULL;
+	rcu_assign_pointer(tty->disc_data, NULL);
 	sl->tty = NULL;
 	spin_unlock_bh(&sl->lock);
 
+	synchronize_rcu();
 	flush_work(&sl->tx_work);
 
 	/* VSV = very important to remove timers */