* [PATCH net] ax25: Fix ax25 session cleanup problem in ax25_release
@ 2022-05-25 11:28 Duoming Zhou
2022-05-27 8:11 ` Thomas Osterried
0 siblings, 1 reply; 6+ messages in thread
From: Duoming Zhou @ 2022-05-25 11:28 UTC (permalink / raw)
To: netdev
Cc: jreuter, ralf, davem, edumazet, kuba, pabeni, linux-kernel,
linux-hams, thomas, Duoming Zhou
The timers of ax25 are used for correct session cleanup.
If we use ax25_release() to close ax25 sessions and
ax25_dev is not null, the del_timer_sync() functions in
ax25_release() will execute. As a result, the sessions
could not be cleaned up correctly, because the timers
have stopped.
This patch adds a device_up flag in ax25_dev in order to
judge whether the device is up. If there are sessions to
be cleaned up, the del_timer_sync() in ax25_release() will
not execute. As a result the sessions could be cleaned up
correctly.
Fixes: 82e31755e55f ("ax25: Fix UAF bugs in ax25 timers")
Reported-by: Thomas Osterried <thomas@osterried.de>
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
include/net/ax25.h | 1 +
net/ax25/af_ax25.c | 13 ++++++++-----
net/ax25/ax25_dev.c | 1 +
3 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/include/net/ax25.h b/include/net/ax25.h
index 0f9790c455b..a427a05672e 100644
--- a/include/net/ax25.h
+++ b/include/net/ax25.h
@@ -228,6 +228,7 @@ typedef struct ax25_dev {
ax25_dama_info dama;
#endif
refcount_t refcount;
+ bool device_up;
} ax25_dev;
typedef struct ax25_cb {
diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index 363d47f9453..47ce6b630cc 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -81,6 +81,7 @@ static void ax25_kill_by_device(struct net_device *dev)
if ((ax25_dev = ax25_dev_ax25dev(dev)) == NULL)
return;
+ ax25_dev->device_up = false;
spin_lock_bh(&ax25_list_lock);
again:
@@ -1053,11 +1054,13 @@ static int ax25_release(struct socket *sock)
ax25_destroy_socket(ax25);
}
if (ax25_dev) {
- del_timer_sync(&ax25->timer);
- del_timer_sync(&ax25->t1timer);
- del_timer_sync(&ax25->t2timer);
- del_timer_sync(&ax25->t3timer);
- del_timer_sync(&ax25->idletimer);
+ if (!ax25_dev->device_up) {
+ del_timer_sync(&ax25->timer);
+ del_timer_sync(&ax25->t1timer);
+ del_timer_sync(&ax25->t2timer);
+ del_timer_sync(&ax25->t3timer);
+ del_timer_sync(&ax25->idletimer);
+ }
dev_put_track(ax25_dev->dev, &ax25_dev->dev_tracker);
ax25_dev_put(ax25_dev);
}
diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
index d2a244e1c26..5451be15e07 100644
--- a/net/ax25/ax25_dev.c
+++ b/net/ax25/ax25_dev.c
@@ -62,6 +62,7 @@ void ax25_dev_device_up(struct net_device *dev)
ax25_dev->dev = dev;
dev_hold_track(dev, &ax25_dev->dev_tracker, GFP_ATOMIC);
ax25_dev->forward = NULL;
+ ax25_dev->device_up = true;
ax25_dev->values[AX25_VALUES_IPDEFMODE] = AX25_DEF_IPDEFMODE;
ax25_dev->values[AX25_VALUES_AXDEFMODE] = AX25_DEF_AXDEFMODE;
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net] ax25: Fix ax25 session cleanup problem in ax25_release
2022-05-25 11:28 [PATCH net] ax25: Fix ax25 session cleanup problem in ax25_release Duoming Zhou
@ 2022-05-27 8:11 ` Thomas Osterried
2022-05-27 11:29 ` Thomas Osterried
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Osterried @ 2022-05-27 8:11 UTC (permalink / raw)
To: Duoming Zhou
Cc: netdev, jreuter, ralf, davem, edumazet, kuba, pabeni,
linux-kernel, linux-hams, thomas
I Tested several cases: this patch works as expected.
Anyone else testet it?
vy 73,
- Thomas dl9sau
On Wed, May 25, 2022 at 07:28:50PM +0800, Duoming Zhou wrote:
> The timers of ax25 are used for correct session cleanup.
> If we use ax25_release() to close ax25 sessions and
> ax25_dev is not null, the del_timer_sync() functions in
> ax25_release() will execute. As a result, the sessions
> could not be cleaned up correctly, because the timers
> have stopped.
>
> This patch adds a device_up flag in ax25_dev in order to
> judge whether the device is up. If there are sessions to
> be cleaned up, the del_timer_sync() in ax25_release() will
> not execute. As a result the sessions could be cleaned up
> correctly.
>
> Fixes: 82e31755e55f ("ax25: Fix UAF bugs in ax25 timers")
> Reported-by: Thomas Osterried <thomas@osterried.de>
> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> ---
> include/net/ax25.h | 1 +
> net/ax25/af_ax25.c | 13 ++++++++-----
> net/ax25/ax25_dev.c | 1 +
> 3 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/ax25.h b/include/net/ax25.h
> index 0f9790c455b..a427a05672e 100644
> --- a/include/net/ax25.h
> +++ b/include/net/ax25.h
> @@ -228,6 +228,7 @@ typedef struct ax25_dev {
> ax25_dama_info dama;
> #endif
> refcount_t refcount;
> + bool device_up;
> } ax25_dev;
>
> typedef struct ax25_cb {
> diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
> index 363d47f9453..47ce6b630cc 100644
> --- a/net/ax25/af_ax25.c
> +++ b/net/ax25/af_ax25.c
> @@ -81,6 +81,7 @@ static void ax25_kill_by_device(struct net_device *dev)
>
> if ((ax25_dev = ax25_dev_ax25dev(dev)) == NULL)
> return;
> + ax25_dev->device_up = false;
>
> spin_lock_bh(&ax25_list_lock);
> again:
> @@ -1053,11 +1054,13 @@ static int ax25_release(struct socket *sock)
> ax25_destroy_socket(ax25);
> }
> if (ax25_dev) {
> - del_timer_sync(&ax25->timer);
> - del_timer_sync(&ax25->t1timer);
> - del_timer_sync(&ax25->t2timer);
> - del_timer_sync(&ax25->t3timer);
> - del_timer_sync(&ax25->idletimer);
> + if (!ax25_dev->device_up) {
> + del_timer_sync(&ax25->timer);
> + del_timer_sync(&ax25->t1timer);
> + del_timer_sync(&ax25->t2timer);
> + del_timer_sync(&ax25->t3timer);
> + del_timer_sync(&ax25->idletimer);
> + }
> dev_put_track(ax25_dev->dev, &ax25_dev->dev_tracker);
> ax25_dev_put(ax25_dev);
> }
> diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
> index d2a244e1c26..5451be15e07 100644
> --- a/net/ax25/ax25_dev.c
> +++ b/net/ax25/ax25_dev.c
> @@ -62,6 +62,7 @@ void ax25_dev_device_up(struct net_device *dev)
> ax25_dev->dev = dev;
> dev_hold_track(dev, &ax25_dev->dev_tracker, GFP_ATOMIC);
> ax25_dev->forward = NULL;
> + ax25_dev->device_up = true;
>
> ax25_dev->values[AX25_VALUES_IPDEFMODE] = AX25_DEF_IPDEFMODE;
> ax25_dev->values[AX25_VALUES_AXDEFMODE] = AX25_DEF_AXDEFMODE;
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] ax25: Fix ax25 session cleanup problem in ax25_release
2022-05-27 8:11 ` Thomas Osterried
@ 2022-05-27 11:29 ` Thomas Osterried
2022-05-27 14:23 ` duoming
2022-05-28 0:33 ` Jakub Kicinski
0 siblings, 2 replies; 6+ messages in thread
From: Thomas Osterried @ 2022-05-27 11:29 UTC (permalink / raw)
To: Duoming Zhou; +Cc: David S. Miller, Jakub Kicinski, Paolo Abeni, linux-hams
> Am 27.05.2022 um 10:11 schrieb Thomas Osterried <thomas@osterried.de>:
>
>
> I Tested several cases: this patch works as expected.
Unfortunately, there's an other case where sessions are not cleaned up:
Remote site connects:
bpq0: fm DL9SAU-15 to DL9SAU-1 ctl SABM+ 12:56:30.624845
bpq0: fm DL9SAU-1 to DL9SAU-15 ctl UA- 12:56:30.626000
At DL9SAU-1:
# netstat --ax25
Active AX.25 sockets
Dest Source Device State Vr/Vs Send-Q Recv-Q
DL9SAU-15 DL9SAU-1 bpq0 ESTABLISHED 000/000 0 0
# ifconfig bpq0 down
# netstat --ax25
Active AX.25 sockets
Dest Source Device State Vr/Vs Send-Q Recv-Q
DL9SAU-15 DL9SAU-1 ??? LISTENING 000/000 0 0
#
Waited some time. No change.
# rmmod bpqether
# netstat --ax25
Active AX.25 sockets
Dest Source Device State Vr/Vs Send-Q Recv-Q
DL9SAU-15 DL9SAU-1 ??? LISTENING 000/000 0 0
This is the condition:
ax25_kill_by_device()
...
if (!sk) {
spin_unlock_bh(&ax25_list_lock);
ax25_disconnect(s, ENETUNREACH);
s->ax25_dev = NULL;
...
=> ax25_disconnect():
if (reason == ENETUNREACH) {
del_timer_sync(&ax25->timer);
-> Nothing cleans the ax25_cb up.
ax25_release() is not part of that kind of sessions.
ax25_disconect() stopped timers.
If you agree, that no concurrent process is able to re-use this ax25_cb,
and because all timers are stoppeed, the cleanup with ax25_cb_del(s);
should be safe.
My successfull test was this:
diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index 363d47f94532..de417b974c07 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -91,6 +92,7 @@ static void ax25_kill_by_device(struct net_device *dev)
spin_unlock_bh(&ax25_list_lock);
ax25_disconnect(s, ENETUNREACH);
s->ax25_dev = NULL;
+ ax25_cb_del(s);
spin_lock_bh(&ax25_list_lock);
goto again;
}
vy 73,
- Thomas dl9sau
>
>
> On Wed, May 25, 2022 at 07:28:50PM +0800, Duoming Zhou wrote:
>> The timers of ax25 are used for correct session cleanup.
>> If we use ax25_release() to close ax25 sessions and
>> ax25_dev is not null, the del_timer_sync() functions in
>> ax25_release() will execute. As a result, the sessions
>> could not be cleaned up correctly, because the timers
>> have stopped.
>>
>> This patch adds a device_up flag in ax25_dev in order to
>> judge whether the device is up. If there are sessions to
>> be cleaned up, the del_timer_sync() in ax25_release() will
>> not execute. As a result the sessions could be cleaned up
>> correctly.
>>
>> Fixes: 82e31755e55f ("ax25: Fix UAF bugs in ax25 timers")
>> Reported-by: Thomas Osterried <thomas@osterried.de>
>> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
>> ---
>> include/net/ax25.h | 1 +
>> net/ax25/af_ax25.c | 13 ++++++++-----
>> net/ax25/ax25_dev.c | 1 +
>> 3 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/net/ax25.h b/include/net/ax25.h
>> index 0f9790c455b..a427a05672e 100644
>> --- a/include/net/ax25.h
>> +++ b/include/net/ax25.h
>> @@ -228,6 +228,7 @@ typedef struct ax25_dev {
>> ax25_dama_info dama;
>> #endif
>> refcount_t refcount;
>> + bool device_up;
>> } ax25_dev;
>>
>> typedef struct ax25_cb {
>> diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
>> index 363d47f9453..47ce6b630cc 100644
>> --- a/net/ax25/af_ax25.c
>> +++ b/net/ax25/af_ax25.c
>> @@ -81,6 +81,7 @@ static void ax25_kill_by_device(struct net_device *dev)
>>
>> if ((ax25_dev = ax25_dev_ax25dev(dev)) == NULL)
>> return;
>> + ax25_dev->device_up = false;
>>
>> spin_lock_bh(&ax25_list_lock);
>> again:
>> @@ -1053,11 +1054,13 @@ static int ax25_release(struct socket *sock)
>> ax25_destroy_socket(ax25);
>> }
>> if (ax25_dev) {
>> - del_timer_sync(&ax25->timer);
>> - del_timer_sync(&ax25->t1timer);
>> - del_timer_sync(&ax25->t2timer);
>> - del_timer_sync(&ax25->t3timer);
>> - del_timer_sync(&ax25->idletimer);
>> + if (!ax25_dev->device_up) {
>> + del_timer_sync(&ax25->timer);
>> + del_timer_sync(&ax25->t1timer);
>> + del_timer_sync(&ax25->t2timer);
>> + del_timer_sync(&ax25->t3timer);
>> + del_timer_sync(&ax25->idletimer);
>> + }
>> dev_put_track(ax25_dev->dev, &ax25_dev->dev_tracker);
>> ax25_dev_put(ax25_dev);
>> }
>> diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
>> index d2a244e1c26..5451be15e07 100644
>> --- a/net/ax25/ax25_dev.c
>> +++ b/net/ax25/ax25_dev.c
>> @@ -62,6 +62,7 @@ void ax25_dev_device_up(struct net_device *dev)
>> ax25_dev->dev = dev;
>> dev_hold_track(dev, &ax25_dev->dev_tracker, GFP_ATOMIC);
>> ax25_dev->forward = NULL;
>> + ax25_dev->device_up = true;
>>
>> ax25_dev->values[AX25_VALUES_IPDEFMODE] = AX25_DEF_IPDEFMODE;
>> ax25_dev->values[AX25_VALUES_AXDEFMODE] = AX25_DEF_AXDEFMODE;
>> --
>> 2.17.1
>>
>
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net] ax25: Fix ax25 session cleanup problem in ax25_release
2022-05-27 11:29 ` Thomas Osterried
@ 2022-05-27 14:23 ` duoming
2022-05-28 0:33 ` Jakub Kicinski
1 sibling, 0 replies; 6+ messages in thread
From: duoming @ 2022-05-27 14:23 UTC (permalink / raw)
To: Thomas Osterried; +Cc: David S. Miller, Jakub Kicinski, Paolo Abeni, linux-hams
Hello,
On Fri, 27 May 2022 13:29:30 +0200 Thomas wrote:
> > I Tested several cases: this patch works as expected.
>
> Unfortunately, there's an other case where sessions are not cleaned up:
>
> Remote site connects:
> bpq0: fm DL9SAU-15 to DL9SAU-1 ctl SABM+ 12:56:30.624845
> bpq0: fm DL9SAU-1 to DL9SAU-15 ctl UA- 12:56:30.626000
>
>
> At DL9SAU-1:
>
> # netstat --ax25
> Active AX.25 sockets
> Dest Source Device State Vr/Vs Send-Q Recv-Q
> DL9SAU-15 DL9SAU-1 bpq0 ESTABLISHED 000/000 0 0
>
> # ifconfig bpq0 down
> # netstat --ax25
> Active AX.25 sockets
> Dest Source Device State Vr/Vs Send-Q Recv-Q
> DL9SAU-15 DL9SAU-1 ??? LISTENING 000/000 0 0
> #
>
> Waited some time. No change.
>
> # rmmod bpqether
> # netstat --ax25
> Active AX.25 sockets
> Dest Source Device State Vr/Vs Send-Q Recv-Q
> DL9SAU-15 DL9SAU-1 ??? LISTENING 000/000 0 0
>
>
> This is the condition:
> ax25_kill_by_device()
> ...
> if (!sk) {
> spin_unlock_bh(&ax25_list_lock);
> ax25_disconnect(s, ENETUNREACH);
> s->ax25_dev = NULL;
> ...
>
> => ax25_disconnect():
> if (reason == ENETUNREACH) {
> del_timer_sync(&ax25->timer);
>
>
>
> -> Nothing cleans the ax25_cb up.
> ax25_release() is not part of that kind of sessions.
> ax25_disconect() stopped timers.
>
>
> If you agree, that no concurrent process is able to re-use this ax25_cb,
> and because all timers are stoppeed, the cleanup with ax25_cb_del(s);
> should be safe.
I agree, the ax25_cb could not be re-used.
> My successfull test was this:
>
> diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
> index 363d47f94532..de417b974c07 100644
> --- a/net/ax25/af_ax25.c
> +++ b/net/ax25/af_ax25.c
> @@ -91,6 +92,7 @@ static void ax25_kill_by_device(struct net_device *dev)
> spin_unlock_bh(&ax25_list_lock);
> ax25_disconnect(s, ENETUNREACH);
> s->ax25_dev = NULL;
> + ax25_cb_del(s);
> spin_lock_bh(&ax25_list_lock);
> goto again;
> }
>
I agree, but we should also add ax25_cb_del(s) in the following place:
@@ -104,6 +106,7 @@ static void ax25_kill_by_device(struct net_device *dev)
ax25_dev_put(ax25_dev);
}
release_sock(sk);
+ ax25_cb_del(s);
spin_lock_bh(&ax25_list_lock);
sock_put(sk);
/* The entry could have been deleted from the
Best regards,
Duoming Zhou
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] ax25: Fix ax25 session cleanup problem in ax25_release
2022-05-27 11:29 ` Thomas Osterried
2022-05-27 14:23 ` duoming
@ 2022-05-28 0:33 ` Jakub Kicinski
2022-05-28 13:29 ` duoming
1 sibling, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2022-05-28 0:33 UTC (permalink / raw)
To: Thomas Osterried; +Cc: Duoming Zhou, David S. Miller, Paolo Abeni, linux-hams
On Fri, 27 May 2022 13:29:30 +0200 Thomas Osterried wrote:
> > I Tested several cases: this patch works as expected.
> If you agree, that no concurrent process is able to re-use this ax25_cb,
> and because all timers are stoppeed, the cleanup with ax25_cb_del(s);
> should be safe.
>
>
> My successfull test was this:
>
> diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
> index 363d47f94532..de417b974c07 100644
> --- a/net/ax25/af_ax25.c
> +++ b/net/ax25/af_ax25.c
> @@ -91,6 +92,7 @@ static void ax25_kill_by_device(struct net_device *dev)
> spin_unlock_bh(&ax25_list_lock);
> ax25_disconnect(s, ENETUNREACH);
> s->ax25_dev = NULL;
> + ax25_cb_del(s);
> spin_lock_bh(&ax25_list_lock);
> goto again;
> }
Thanks a lot for the testing and analysis! We'll give v2 [1] another
day or two on the list and merge it into net.
[1]
https://lore.kernel.org/all/20220527151822.23217-1-duoming@zju.edu.cn/
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] ax25: Fix ax25 session cleanup problem in ax25_release
2022-05-28 0:33 ` Jakub Kicinski
@ 2022-05-28 13:29 ` duoming
0 siblings, 0 replies; 6+ messages in thread
From: duoming @ 2022-05-28 13:29 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Thomas Osterried, David S. Miller, Paolo Abeni, linux-hams
Hello,
On Fri, 27 May 2022 13:29:30 +0200 Thomas Osterried wrote:
> > > I Tested several cases: this patch works as expected.
> > If you agree, that no concurrent process is able to re-use this ax25_cb,
> > and because all timers are stoppeed, the cleanup with ax25_cb_del(s);
> > should be safe.
> >
> >
> > My successfull test was this:
> >
> > diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
> > index 363d47f94532..de417b974c07 100644
> > --- a/net/ax25/af_ax25.c
> > +++ b/net/ax25/af_ax25.c
> > @@ -91,6 +92,7 @@ static void ax25_kill_by_device(struct net_device *dev)
> > spin_unlock_bh(&ax25_list_lock);
> > ax25_disconnect(s, ENETUNREACH);
> > s->ax25_dev = NULL;
> > + ax25_cb_del(s);
> > spin_lock_bh(&ax25_list_lock);
> > goto again;
> > }
The above code also exists a race condition:
(Thread 1) | (Thread 2)
ax25_kill_by_device() | ax25_rcv()
... | ...
if (!sk) { |
ax25_cb_del() | ax25_destroy_socket()
if (!hlist_unhashed(..)) | ax25_cb_del()
... | if (!hlist_unhashed(..))
hlist_del_init() |
ax25_cb_put(ax25) | ...
| ax25_cb_put(ax25)
| ...
If we use ax25_cb_del() to delete ax25 node from the hlist, the ax25_cb_put() should only be
called once. But the above picture shows that ax25_cb_put() is called twice and the refcount
of the same ax25_cb is decreased twice. As a result, it will cause "refcount_t: underflow".
The following is a solution that has been tested.
@@ -62,12 +62,12 @@ static void ax25_free_sock(struct sock *sk)
*/
static void ax25_cb_del(ax25_cb *ax25)
{
+ spin_lock_bh(&ax25_list_lock);
if (!hlist_unhashed(&ax25->ax25_node)) {
- spin_lock_bh(&ax25_list_lock);
hlist_del_init(&ax25->ax25_node);
- spin_unlock_bh(&ax25_list_lock);
ax25_cb_put(ax25);
}
+ spin_unlock_bh(&ax25_list_lock);
}
Best regards,
Duoming Zhou
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-05-28 13:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-05-25 11:28 [PATCH net] ax25: Fix ax25 session cleanup problem in ax25_release Duoming Zhou
2022-05-27 8:11 ` Thomas Osterried
2022-05-27 11:29 ` Thomas Osterried
2022-05-27 14:23 ` duoming
2022-05-28 0:33 ` Jakub Kicinski
2022-05-28 13:29 ` duoming
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox