Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] can, slip: Protect tty->disc_data access with RCU
To:     Richard Palethorpe <rpalethorpe@suse.com>,
        linux-can@vger.kernel.org
Cc:     syzbot+017e491ae13c0068598a@syzkaller.appspotmail.com,
        Wolfgang Grandegger <wg@grandegger.com>,
        Marc Kleine-Budde <mkl@pengutronix.de>,
        "David S. Miller" <davem@davemloft.net>,
        Tyler Hall <tylerwhall@gmail.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, syzkaller@googlegroups.com
References: <0000000000002b81b70590a83ad7@google.com>
 <20200114143244.20739-1-rpalethorpe@suse.com>
From:   Eric Dumazet <eric.dumazet@gmail.com>
Message-ID: <19d5e4c6-72f4-631f-2ccd-b5df660a5ef6@gmail.com>
Date:   Tue, 14 Jan 2020 07:17:51 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.2.2
MIME-Version: 1.0
In-Reply-To: <20200114143244.20739-1-rpalethorpe@suse.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 1/14/20 6:32 AM, Richard Palethorpe wrote:
> write_wakeup can happen in parallel with close where tty->disc_data is set
> to NULL. So we a) need to check if tty->disc_data is NULL and b) ensure it
> is an atomic operation. Otherwise accessing tty->disc_data could result in
> a NULL pointer deref or access to some random location.
> 
> This problem was found by Syzkaller on slcan, but the same issue appears to
> exist in slip where slcan was copied from.
> 
> 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: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: syzkaller@googlegroups.com
> ---
> 
> Note, that mabye RCU should also applied to receive_buf as that also happens
> in interrupt context. So if the pointer assignment is split by the compiler
> then sl may point somewhere unexpected?
> 
>  drivers/net/can/slcan.c | 11 +++++++++--
>  drivers/net/slip/slip.c | 11 +++++++++--
>  2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
> index 2e57122f02fb..ee029aae69d4 100644
> --- a/drivers/net/can/slcan.c
> +++ b/drivers/net/can/slcan.c
> @@ -344,7 +344,14 @@ 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);
> +	rcu_read_unlock();

This rcu_read_lock()/rcu_read_unlock() pair is not protecting anything.

Right after rcu_read_unlock(), sl validity can not be guaranteed.

> +
> +	if (!sl)
> +		return;
>  
>  	schedule_work(&sl->tx_work);
>  }
> @@ -644,7 +651,7 @@ 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);


Where is the rcu grace period before freeing enforced ?

>  
> diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c
> index 2a91c192659f..dfed9f0b8646 100644
> --- a/drivers/net/slip/slip.c
> +++ b/drivers/net/slip/slip.c
> @@ -452,7 +452,14 @@ 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);
> +	rcu_read_unlock();

Same here.

> +
> +	if (!sl)
> +		return;
>  
>  	schedule_work(&sl->tx_work);
>  }
> @@ -882,7 +889,7 @@ 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);
>  
>