netdev - Re: [PATCH] drivers: net: slip: fix NPD bug in sl_tx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <631ac05c-6807-11ff-c940-cd27bcc3a925@kernel.org>
Date:   Wed, 6 Apr 2022 10:45:05 +0200
From:   Jiri Slaby <jirislaby@...nel.org>
To:     Duoming Zhou <duoming@....edu.cn>, davem@...emloft.net
Cc:     kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.or, gregkh@...uxfoundation.org,
        alexander.deucher@....com, broonie@...nel.org
Subject: Re: [PATCH] drivers: net: slip: fix NPD bug in sl_tx_timeout()

On 05. 04. 22, 15:22, Duoming Zhou wrote:
> When a slip driver is detaching, the slip_close() will act to
> cleanup necessary resources and sl->tty is set to NULL in
> slip_close(). Meanwhile, the packet we transmit is blocked,
> sl_tx_timeout() will be called. Although slip_close() and
> sl_tx_timeout() use sl->lock to synchronize, we don`t judge
> whether sl->tty equals to NULL in sl_tx_timeout() and the
> null pointer dereference bug will happen.
> 
>     (Thread 1)                 |      (Thread 2)
>                                | slip_close()
>                                |   spin_lock_bh(&sl->lock)
>                                |   ...
> ...                           |   sl->tty = NULL //(1)
> sl_tx_timeout()               |   spin_unlock_bh(&sl->lock)
>    spin_lock(&sl->lock);       |
>    ...                         |   ...
>    tty_chars_in_buffer(sl->tty)|
>      if (tty->ops->..) //(2)   |
>      ...                       |   synchronize_rcu()
> 
> We set NULL to sl->tty in position (1) and dereference sl->tty
> in position (2).
> 
> This patch adds check in sl_tx_timeout(). If sl->tty equals to
> NULL, sl_tx_timeout() will goto out.

It makes sense. unregister_netdev() (to kill the timer) is called only 
later in slip_close().

Reviewed-by: Jiri Slaby <jirislaby@...nel.org>

> Signed-off-by: Duoming Zhou <duoming@....edu.cn>
> ---
>   drivers/net/slip/slip.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c
> index 88396ff99f0..6865d32270e 100644
> --- a/drivers/net/slip/slip.c
> +++ b/drivers/net/slip/slip.c
> @@ -469,7 +469,7 @@ static void sl_tx_timeout(struct net_device *dev, unsigned int txqueue)
>   	spin_lock(&sl->lock);
>   
>   	if (netif_queue_stopped(dev)) {
> -		if (!netif_running(dev))
> +		if (!netif_running(dev) || !sl->tty)
>   			goto out;
>   
>   		/* May be we must check transmitter timeout here ?


-- 
js
suse labs