* [PATCH] ax25: Fix memory leaks caused by ax25_cb_del()
@ 2022-03-09 15:06 Duoming Zhou
2022-03-10 13:05 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Duoming Zhou @ 2022-03-09 15:06 UTC (permalink / raw)
To: linux-hams
Cc: netdev, linux-kernel, jreuter, kuba, davem, ralf, thomas, Duoming Zhou
The previous commit d01ffb9eee4a ("ax25: add refcount in ax25_dev to
avoid UAF bugs") and commit feef318c855a ("ax25: fix UAF bugs of
net_device caused by rebinding operation") increase the refcounts of
ax25_dev and net_device in ax25_bind() and decrease the matching refcounts
in ax25_kill_by_device() in order to prevent UAF bugs. But there are memory
leaks.
If we use ax25_bind() to increase the refcounts of ax25_dev and
net_device, then, use ax25_cb_del() invoked by ax25_destroy_socket()
to delete ax25_cb in hlist before calling ax25_kill_by_device(), the
decrements of refcounts in ax25_kill_by_device() will not be executed,
because ax25_cb is deleted from the hlist.
This patch adds two flags in ax25_dev in order to prevent memory leaks.
If we bind successfully, then, use ax25_cb_del() to delete ax25_cb,
the two "test_bit" condition checks in ax25_kill_by_device() could
pass and the refcounts could be decreased properly.
Fixes: d01ffb9eee4a ("ax25: add refcount in ax25_dev to avoid UAF bugs")
Fixes: feef318c855a ("ax25: fix UAF bugs of net_device caused by rebinding operation")
Reported-by: Thomas Osterried <thomas@osterried.de>
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
include/net/ax25.h | 7 +++++--
net/ax25/af_ax25.c | 10 ++++++----
net/ax25/ax25_dev.c | 3 ++-
3 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/include/net/ax25.h b/include/net/ax25.h
index 8221af1811d..50b3eacada0 100644
--- a/include/net/ax25.h
+++ b/include/net/ax25.h
@@ -157,7 +157,9 @@ enum {
#define AX25_DEF_PACLEN 256 /* Paclen=256 */
#define AX25_DEF_PROTOCOL AX25_PROTO_STD_SIMPLEX /* Standard AX.25 */
#define AX25_DEF_DS_TIMEOUT 180000 /* DAMA timeout 3 minutes */
-
+#define AX25_DEV_INIT 0
+#define AX25_DEV_KILL 1
+#define AX25_DEV_BIND 2
typedef struct ax25_uid_assoc {
struct hlist_node uid_node;
refcount_t refcount;
@@ -240,8 +242,9 @@ typedef struct ax25_dev {
ax25_dama_info dama;
#endif
refcount_t refcount;
+ unsigned long kill_flag;
+ unsigned long bind_flag;
} ax25_dev;
-
typedef struct ax25_cb {
struct hlist_node ax25_node;
ax25_address source_addr, dest_addr;
diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index 6bd09718077..5519448378d 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -86,6 +86,7 @@ static void ax25_kill_by_device(struct net_device *dev)
again:
ax25_for_each(s, &ax25_list) {
if (s->ax25_dev == ax25_dev) {
+ set_bit(AX25_DEV_KILL, &ax25_dev->kill_flag);
sk = s->sk;
if (!sk) {
spin_unlock_bh(&ax25_list_lock);
@@ -114,9 +115,12 @@ static void ax25_kill_by_device(struct net_device *dev)
goto again;
}
}
+ if(!test_bit(AX25_DEV_KILL, &ax25_dev->kill_flag) && test_bit(AX25_DEV_BIND, &ax25_dev->bind_flag)) {
+ dev_put_track(ax25_dev->dev, &ax25_dev->dev_tracker);
+ ax25_dev_put(ax25_dev);
+ }
spin_unlock_bh(&ax25_list_lock);
}
-
/*
* Handle device status changes.
*/
@@ -1132,13 +1136,11 @@ static int ax25_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
done:
ax25_cb_add(ax25);
sock_reset_flag(sk, SOCK_ZAPPED);
-
+ set_bit(AX25_DEV_BIND, &ax25_dev->bind_flag);
out:
release_sock(sk);
-
return err;
}
-
/*
* FIXME: nonblock behaviour looks like it may have a bug.
*/
diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
index d2a244e1c26..7c40914e5c9 100644
--- a/net/ax25/ax25_dev.c
+++ b/net/ax25/ax25_dev.c
@@ -77,7 +77,8 @@ void ax25_dev_device_up(struct net_device *dev)
ax25_dev->values[AX25_VALUES_PACLEN] = AX25_DEF_PACLEN;
ax25_dev->values[AX25_VALUES_PROTOCOL] = AX25_DEF_PROTOCOL;
ax25_dev->values[AX25_VALUES_DS_TIMEOUT]= AX25_DEF_DS_TIMEOUT;
-
+ ax25_dev->kill_flag = AX25_DEV_INIT;
+ ax25_dev->bind_flag = AX25_DEV_INIT;
#if defined(CONFIG_AX25_DAMA_SLAVE) || defined(CONFIG_AX25_DAMA_MASTER)
ax25_ds_setup_timer(ax25_dev);
#endif
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] ax25: Fix memory leaks caused by ax25_cb_del()
2022-03-09 15:06 [PATCH] ax25: Fix memory leaks caused by ax25_cb_del() Duoming Zhou
@ 2022-03-10 13:05 ` Dan Carpenter
2022-03-10 16:01 ` 周多明
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2022-03-10 13:05 UTC (permalink / raw)
To: Duoming Zhou
Cc: linux-hams, netdev, linux-kernel, jreuter, kuba, davem, ralf, thomas
This is a very frustrating patch because you make a lot of unnecessary
white space changes and you didn't run checkpatch on your patch.
The whole approach feels like the wrong thing...
I have read your commit message, but I don't understand why we can't
just use normal refcounting. It sounds like there is a layering
violation somewhere?
Even if we go with this approach ->kill_flag and ->bind_flag should be
booleans. It makes no sense to have a unsigned long where only BIT(2)
can be set.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Re: [PATCH] ax25: Fix memory leaks caused by ax25_cb_del()
2022-03-10 13:05 ` Dan Carpenter
@ 2022-03-10 16:01 ` 周多明
0 siblings, 0 replies; 3+ messages in thread
From: 周多明 @ 2022-03-10 16:01 UTC (permalink / raw)
To: Dan Carpenter
Cc: linux-hams, netdev, linux-kernel, jreuter, kuba, davem, ralf, thomas
Hello,
Thank you very much for pointing the wrong places in my patch.
> This is a very frustrating patch because you make a lot of unnecessary
> white space changes and you didn't run checkpatch on your patch.
>
> The whole approach feels like the wrong thing...
I will fix it.
> I have read your commit message, but I don't understand why we can't
> just use normal refcounting. It sounds like there is a layering
> violation somewhere?
The root cause of refcount leak is shown below:
(Thread 1) | (Thread 2)
ax25_bind() |
... |
ax25_addr_ax25dev() |
ax25_dev_hold() //(1) |
... |
dev_hold_track() //(2) |
... | ax25_destroy_socket()
| ax25_cb_del()
| ...
| spin_lock_bh(&ax25_list_lock);
| hlist_del_init(&ax25->ax25_node); //(3)
| spin_unlock_bh(&ax25_list_lock);
(thread 3)
ax25_kill_by_device() |
spin_lock_bh(&ax25_list_lock); |
ax25_for_each(s, &ax25_list) { |
if (s->ax25_dev == ax25_dev) //(4) |
... |
(the following code could not execute) |
Firstly, we use ax25_bind() to increase the refcount of ax25_dev in
position (1) and increase the refcount of net_device in position (2).
Then, we use ax25_cb_del() invoked by ax25_destroy_socket()
to delete ax25_cb in hlist in position (3) before calling ax25_kill_by_device().
Finally, the decrements of refcounts in ax25_kill_by_device() will not be executed,
because no s->ax25_dev equals to ax25_dev in position (4).
My patch adds two flags in ax25_dev in order to prevent reference count leaks.
If the above condition happens, the two "test_bit" checks in ax25_kill_by_device()
could pass and the refcounts could be decreased properly.
> Even if we go with this approach ->kill_flag and ->bind_flag should be
> booleans. It makes no sense to have a unsigned long where only BIT(2)
> can be set.
I will change kill_flag and bind_flag to booleans.
Best wishes,
Duoming Zhou
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-03-10 16:02 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-09 15:06 [PATCH] ax25: Fix memory leaks caused by ax25_cb_del() Duoming Zhou
2022-03-10 13:05 ` Dan Carpenter
2022-03-10 16:01 ` 周多明
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.