netdev - Re: 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: <64c159c9.387c5.17ffec218b7.Coremail.duoming@zju.edu.cn>
Date:   Wed, 6 Apr 2022 20:03:27 +0800 (GMT+08:00)
From:   duoming@....edu.cn
To:     "Jiri Slaby" <jirislaby@...nel.org>
Cc:     davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
        netdev@...r.kernel.org, gregkh@...uxfoundation.org,
        alexander.deucher@....com, broonie@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: Re: [PATCH] drivers: net: slip: fix NPD bug in sl_tx_timeout()

Hello,

On Wed, 6 Apr 2022 10:45:05 +0200, Jiri Slaby 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>
> 

Thanks for your time and approve my patch.

Best regards,
Duoming Zhou