* [PATCH] handshake: Do not crash if handshake is destroyed
@ 2022-02-02 17:13 Denis Kenzior
0 siblings, 0 replies; 2+ messages in thread
From: Denis Kenzior @ 2022-02-02 17:13 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 5187 bytes --]
Commit 4d2176df2985 ("handshake: Allow event handler to free handshake")
introduced a re-entrancy guard so that handshake_state objects that are
destroyed as a result of the event do not cause a crash. It rightly
used a temporary object to store the passed in handshake. Unfortunately
this caused variable shadowing which resulted in crashes fixed by commit
d22b174a7318 ("handshake: use _hs directly in handshake_event").
However, since the temporary was no longer used, this fix itself caused
a crash:
#0 0x00005555f0ba8b3d in eapol_handle_ptk_1_of_4 (sm=sm(a)entry=0x5555f2b4a920, ek=0x5555f2b62588, ek(a)entry=0x16, unencrypted=unencrypted(a)entry=false) at src/eapol.c:1236
1236 handshake_event(sm->handshake,
(gdb) bt
#0 0x00005555f0ba8b3d in eapol_handle_ptk_1_of_4 (sm=sm(a)entry=0x5555f2b4a920, ek=0x5555f2b62588, ek(a)entry=0x16, unencrypted=unencrypted(a)entry=false) at src/eapol.c:1236
#1 0x00005555f0bab118 in eapol_key_handle (unencrypted=<optimized out>, frame=<optimized out>, sm=0x5555f2b4a920) at src/eapol.c:2343
#2 eapol_rx_packet (proto=<optimized out>, from=<optimized out>, frame=<optimized out>, unencrypted=<optimized out>, user_data=0x5555f2b4a920) at src/eapol.c:2665
#3 0x00005555f0bac497 in __eapol_rx_packet (ifindex=62, src=src(a)entry=0x5555f2b62574 "x\212 J\207\267", proto=proto(a)entry=34958, frame=frame(a)entry=0x5555f2b62588 "\002\003",
len=len(a)entry=121, noencrypt=noencrypt(a)entry=false) at src/eapol.c:3017
#4 0x00005555f0b8c617 in netdev_control_port_frame_event (netdev=0x5555f2b64450, msg=0x5555f2b62588) at src/netdev.c:5574
#5 netdev_unicast_notify (msg=msg(a)entry=0x5555f2b619a0, user_data=<optimized out>) at src/netdev.c:5613
#6 0x00007f60084c9a51 in dispatch_unicast_watches (msg=0x5555f2b619a0, id=<optimized out>, genl=0x5555f2b3fc80) at ell/genl.c:954
#7 process_unicast (nlmsg=0x7fff61abeac0, genl=0x5555f2b3fc80) at ell/genl.c:973
#8 received_data (io=<optimized out>, user_data=0x5555f2b3fc80) at ell/genl.c:1098
#9 0x00007f60084c61bd in io_callback (fd=<optimized out>, events=1, user_data=0x5555f2b3fd20) at ell/io.c:120
#10 0x00007f60084c536d in l_main_iterate (timeout=<optimized out>) at ell/main.c:478
#11 0x00007f60084c543e in l_main_run () at ell/main.c:525
#12 l_main_run () at ell/main.c:507
#13 0x00007f60084c5670 in l_main_run_with_signal (callback=callback(a)entry=0x5555f0b89150 <signal_handler>, user_data=user_data(a)entry=0x0) at ell/main.c:647
#14 0x00005555f0b886a4 in main (argc=<optimized out>, argv=<optimized out>) at src/main.c:532
This happens when the driver does not support rekeying, which causes iwd to
attempt a disconnect and re-connect. The disconnect action is
taken during the event callback and destroys the underlying eapol state
machine. Since a temporary isn't used, attempting to dereference
sm->handshake results in a crash.
Fix this by introducing a UNIQUE_ID macro which should prevent shadowing
and using a temporary variable as originally intended.
Fixes: d22b174a7318 ("handshake: use _hs directly in handshake_event")
Fixes: 4d2176df2985 ("handshake: Allow event handler to free handshake")
Reported-By: Toke Høiland-Jørgensen <toke(a)toke.dk>
---
src/handshake.h | 38 +++++++++++++++++++++-----------------
src/util.h | 4 ++++
2 files changed, 25 insertions(+), 17 deletions(-)
diff --git a/src/handshake.h b/src/handshake.h
index 7136087a1a4a..7f597b06078b 100644
--- a/src/handshake.h
+++ b/src/handshake.h
@@ -164,23 +164,27 @@ struct handshake_state {
handshake_event_func_t event_func;
};
-#define handshake_event(_hs, event, ...) \
- (__extension__ ({ \
- bool freed = false; \
- \
- if ((_hs)->event_func && !(_hs)->in_event) { \
- (_hs)->in_event = true; \
- (_hs)->event_func((_hs), event, (_hs)->user_data, \
- ##__VA_ARGS__); \
- \
- if (!(_hs)->in_event) { \
- handshake_state_free((_hs)); \
- freed = true; \
- } else \
- (_hs)->in_event = false; \
- } \
- freed; \
- }))
+#define HSID(x) UNIQUE_ID(handshake_, x)
+
+#define handshake_event(_hs, event, ...) \
+ ({ \
+ bool HSID(freed) = false; \
+ typeof(_hs) HSID(hs) = (_hs); \
+ \
+ if (HSID(hs)->event_func && !HSID(hs)->in_event) { \
+ HSID(hs)->in_event = true; \
+ HSID(hs)->event_func(HSID(hs), (event), \
+ HSID(hs)->user_data, \
+ ##__VA_ARGS__); \
+ \
+ if (!HSID(hs)->in_event) { \
+ handshake_state_free(HSID(hs)); \
+ HSID(freed) = true; \
+ } else \
+ HSID(hs)->in_event = false; \
+ } \
+ HSID(freed); \
+ })
void handshake_state_free(struct handshake_state *s);
diff --git a/src/util.h b/src/util.h
index b55a7ea7cd6b..75c29039b04a 100644
--- a/src/util.h
+++ b/src/util.h
@@ -26,6 +26,10 @@
#include <stdint.h>
#include <unistd.h>
+#define ___PASTE(a, b) a ## b
+#define __PASTE(a, b) ___PASTE(a, b)
+#define UNIQUE_ID(x, id) __PASTE(__unique_prefix_, __PASTE(x, id))
+
#define align_len(len, boundary) (((len)+(boundary)-1) & ~((boundary)-1))
#define MAC "%02x:%02x:%02x:%02x:%02x:%02x"
--
2.32.0
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] handshake: Do not crash if handshake is destroyed
@ 2022-02-03 10:49
0 siblings, 0 replies; 2+ messages in thread
From: @ 2022-02-03 10:49 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 3465 bytes --]
Denis Kenzior <denkenz(a)gmail.com> writes:
> Commit 4d2176df2985 ("handshake: Allow event handler to free handshake")
> introduced a re-entrancy guard so that handshake_state objects that are
> destroyed as a result of the event do not cause a crash. It rightly
> used a temporary object to store the passed in handshake. Unfortunately
> this caused variable shadowing which resulted in crashes fixed by commit
> d22b174a7318 ("handshake: use _hs directly in handshake_event").
> However, since the temporary was no longer used, this fix itself caused
> a crash:
>
> #0 0x00005555f0ba8b3d in eapol_handle_ptk_1_of_4 (sm=sm(a)entry=0x5555f2b4a920, ek=0x5555f2b62588, ek(a)entry=0x16, unencrypted=unencrypted(a)entry=false) at src/eapol.c:1236
> 1236 handshake_event(sm->handshake,
> (gdb) bt
> #0 0x00005555f0ba8b3d in eapol_handle_ptk_1_of_4 (sm=sm(a)entry=0x5555f2b4a920, ek=0x5555f2b62588, ek(a)entry=0x16, unencrypted=unencrypted(a)entry=false) at src/eapol.c:1236
> #1 0x00005555f0bab118 in eapol_key_handle (unencrypted=<optimized out>, frame=<optimized out>, sm=0x5555f2b4a920) at src/eapol.c:2343
> #2 eapol_rx_packet (proto=<optimized out>, from=<optimized out>, frame=<optimized out>, unencrypted=<optimized out>, user_data=0x5555f2b4a920) at src/eapol.c:2665
> #3 0x00005555f0bac497 in __eapol_rx_packet (ifindex=62, src=src(a)entry=0x5555f2b62574 "x\212 J\207\267", proto=proto(a)entry=34958, frame=frame(a)entry=0x5555f2b62588 "\002\003",
> len=len(a)entry=121, noencrypt=noencrypt(a)entry=false) at src/eapol.c:3017
> #4 0x00005555f0b8c617 in netdev_control_port_frame_event (netdev=0x5555f2b64450, msg=0x5555f2b62588) at src/netdev.c:5574
> #5 netdev_unicast_notify (msg=msg(a)entry=0x5555f2b619a0, user_data=<optimized out>) at src/netdev.c:5613
> #6 0x00007f60084c9a51 in dispatch_unicast_watches (msg=0x5555f2b619a0, id=<optimized out>, genl=0x5555f2b3fc80) at ell/genl.c:954
> #7 process_unicast (nlmsg=0x7fff61abeac0, genl=0x5555f2b3fc80) at ell/genl.c:973
> #8 received_data (io=<optimized out>, user_data=0x5555f2b3fc80) at ell/genl.c:1098
> #9 0x00007f60084c61bd in io_callback (fd=<optimized out>, events=1, user_data=0x5555f2b3fd20) at ell/io.c:120
> #10 0x00007f60084c536d in l_main_iterate (timeout=<optimized out>) at ell/main.c:478
> #11 0x00007f60084c543e in l_main_run () at ell/main.c:525
> #12 l_main_run () at ell/main.c:507
> #13 0x00007f60084c5670 in l_main_run_with_signal (callback=callback(a)entry=0x5555f0b89150 <signal_handler>, user_data=user_data(a)entry=0x0) at ell/main.c:647
> #14 0x00005555f0b886a4 in main (argc=<optimized out>, argv=<optimized out>) at src/main.c:532
>
> This happens when the driver does not support rekeying, which causes iwd to
> attempt a disconnect and re-connect. The disconnect action is
> taken during the event callback and destroys the underlying eapol state
> machine. Since a temporary isn't used, attempting to dereference
> sm->handshake results in a crash.
>
> Fix this by introducing a UNIQUE_ID macro which should prevent shadowing
> and using a temporary variable as originally intended.
>
> Fixes: d22b174a7318 ("handshake: use _hs directly in handshake_event")
> Fixes: 4d2176df2985 ("handshake: Allow event handler to free handshake")
> Reported-By: Toke Høiland-Jørgensen <toke(a)toke.dk>
With this patch, the crashes are gone - thanks!
Tested-by: Toke Høiland-Jørgensen <toke(a)toke.dk>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-02-03 10:49 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-02-03 10:49 [PATCH] handshake: Do not crash if handshake is destroyed
-- strict thread matches above, loose matches on Subject: below --
2022-02-02 17:13 Denis Kenzior
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.