* [PATCH] drivers: net: slip: fix NPD bug in sl_tx_timeout()
@ 2022-04-05 13:22 Duoming Zhou
2022-04-06 8:45 ` Jiri Slaby
2022-04-07 6:10 ` patchwork-bot+netdevbpf
0 siblings, 2 replies; 5+ messages in thread
From: Duoming Zhou @ 2022-04-05 13:22 UTC (permalink / raw)
To: davem
Cc: kuba, pabeni, netdev, linux-kernel, gregkh, alexander.deucher,
broonie, jirislaby, Duoming Zhou
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.
Signed-off-by: Duoming Zhou <duoming@zju.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 ?
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drivers: net: slip: fix NPD bug in sl_tx_timeout()
2022-04-05 13:22 [PATCH] drivers: net: slip: fix NPD bug in sl_tx_timeout() Duoming Zhou
@ 2022-04-06 8:45 ` Jiri Slaby
2022-04-06 12:03 ` duoming
2022-04-07 6:10 ` patchwork-bot+netdevbpf
1 sibling, 1 reply; 5+ messages in thread
From: Jiri Slaby @ 2022-04-06 8:45 UTC (permalink / raw)
To: Duoming Zhou, davem
Cc: kuba, pabeni, netdev, linux-kernel, gregkh, alexander.deucher, broonie
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@kernel.org>
> Signed-off-by: Duoming Zhou <duoming@zju.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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Re: [PATCH] drivers: net: slip: fix NPD bug in sl_tx_timeout()
2022-04-06 8:45 ` Jiri Slaby
@ 2022-04-06 12:03 ` duoming
0 siblings, 0 replies; 5+ messages in thread
From: duoming @ 2022-04-06 12:03 UTC (permalink / raw)
To: Jiri Slaby
Cc: davem, kuba, pabeni, netdev, gregkh, alexander.deucher, broonie,
linux-kernel
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@kernel.org>
>
Thanks for your time and approve my patch.
Best regards,
Duoming Zhou
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drivers: net: slip: fix NPD bug in sl_tx_timeout()
2022-04-05 13:22 [PATCH] drivers: net: slip: fix NPD bug in sl_tx_timeout() Duoming Zhou
2022-04-06 8:45 ` Jiri Slaby
@ 2022-04-07 6:10 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-04-07 6:10 UTC (permalink / raw)
To: Duoming Zhou
Cc: davem, kuba, pabeni, netdev, linux-kernel, gregkh,
alexander.deucher, broonie, jirislaby
Hello:
This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:
On Tue, 5 Apr 2022 21:22:06 +0800 you 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.
>
> [...]
Here is the summary with links:
- drivers: net: slip: fix NPD bug in sl_tx_timeout()
https://git.kernel.org/netdev/net/c/ec4eb8a86ade
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] drivers: net: slip: fix NPD bug in sl_tx_timeout()
@ 2022-04-05 14:33 Duoming Zhou
0 siblings, 0 replies; 5+ messages in thread
From: Duoming Zhou @ 2022-04-05 14:33 UTC (permalink / raw)
To: linux-kernel; +Cc: Duoming Zhou
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.
Signed-off-by: Duoming Zhou <duoming@zju.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 ?
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-04-07 6:10 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-05 13:22 [PATCH] drivers: net: slip: fix NPD bug in sl_tx_timeout() Duoming Zhou
2022-04-06 8:45 ` Jiri Slaby
2022-04-06 12:03 ` duoming
2022-04-07 6:10 ` patchwork-bot+netdevbpf
2022-04-05 14:33 Duoming Zhou
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.