Bug 270649
| Summary: | hostapd and wpa_supplicant use uninitialized ptr if interface disappears | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Robert Morris <rtm> | ||||
| Component: | wireless | Assignee: | Cy Schubert <cy> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Some People | CC: | bz, cy, emaste, freebsd | ||||
| Priority: | --- | ||||||
| Version: | CURRENT | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
did you submit this upstream, too? or is this only happening in our code? (In reply to Mina Galić from comment #1) I have not tried (or reported) this upstream. I think the upstream code doesn't have this problem, since it says packet = pcap_next(...); here's all the commits that file was recently touched in https://freshbsd.org/freebsd/src/branch/main?q=file.name%3A%22contrib%2Fwpa%2Fsrc%2Fl2_packet%2Fl2_packet_freebsd.c%22 (In reply to Mina Galić from comment #4) Both of those commits solved a SIGSEGV and a CPU loop when the device is removed (disappeared). This is a different problem than the previous problems where the device was physically removed. Created attachment 241318 [details]
Fix uninitialized packet pointer on error
Can you try this patch please.
The packet pointer will remain uninitialized when pcap_next_ex() returns an error. This fixes commit 6e5d01124fd4.
(In reply to Cy Schubert from comment #7) This patch makes the problem go away for me. A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=953efa5b200f060564a090ab71b3d7f614a35e3f commit 953efa5b200f060564a090ab71b3d7f614a35e3f Author: Cy Schubert <cy@FreeBSD.org> AuthorDate: 2023-04-06 04:07:15 +0000 Commit: Cy Schubert <cy@FreeBSD.org> CommitDate: 2023-04-06 14:27:24 +0000 wpa_supplicant/hostapd: Fix uninitialized packet pointer on error The packet pointer (called packet) will remain uninitialized when pcap_next_ex() returns an error. This occurs when the wlan interface is shut down using ifconfig destroy. Adding a NULL assignment to packet duplicates what pcap_next() does. The reason we use pcap_next_ex() in this instance is because with pacp_next() when we receive a null pointer if there was an error or if no packets were read. With pcap_next_ex() we can differentiate between an error and legitimately no packets were received. PR: 270649 Reported by: Robert Morris <rtm@lcs.mit.edu> Fixes: 6e5d01124fd4 MFC after: 3 days contrib/wpa/src/l2_packet/l2_packet_freebsd.c | 1 + 1 file changed, 1 insertion(+) are there any plans for pushing our changes upstream? No, not this change. The three commits, maybe. I don't know if removing a USB WiFi device under Linux or OpenBSD (one of the w1fi.org developers is an OpenBSD committer) results in the same hard CPU loop as it does under FreeBSD. The problem is the pcap_next() returns a pointer to a packet. That pointer can be NULL if there's an error, like the device is removed, or it can also be NULL if there is no packet when polled. pcap_next_ex() differentiates between an error and no data. I can try to push the patches (squashed) to our upstream but they may not accept them. The last change we submitted was rejected by them. I'll mark this fixed when it is MFCed. A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=264e0365e8d8e491d1205f4b6489efca2d563eed commit 264e0365e8d8e491d1205f4b6489efca2d563eed Author: Cy Schubert <cy@FreeBSD.org> AuthorDate: 2023-04-06 04:07:15 +0000 Commit: Cy Schubert <cy@FreeBSD.org> CommitDate: 2023-04-09 03:52:27 +0000 wpa_supplicant/hostapd: Fix uninitialized packet pointer on error The packet pointer (called packet) will remain uninitialized when pcap_next_ex() returns an error. This occurs when the wlan interface is shut down using ifconfig destroy. Adding a NULL assignment to packet duplicates what pcap_next() does. The reason we use pcap_next_ex() in this instance is because with pacp_next() when we receive a null pointer if there was an error or if no packets were read. With pcap_next_ex() we can differentiate between an error and legitimately no packets were received. PR: 270649 Reported by: Robert Morris <rtm@lcs.mit.edu> Fixes: 6e5d01124fd4 (cherry picked from commit 953efa5b200f060564a090ab71b3d7f614a35e3f) contrib/wpa/src/l2_packet/l2_packet_freebsd.c | 1 + 1 file changed, 1 insertion(+) A commit in branch stable/12 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=4a99b854723bb28817b01eb7bf485a072a02ae5a commit 4a99b854723bb28817b01eb7bf485a072a02ae5a Author: Cy Schubert <cy@FreeBSD.org> AuthorDate: 2023-04-06 04:07:15 +0000 Commit: Cy Schubert <cy@FreeBSD.org> CommitDate: 2023-04-09 03:52:35 +0000 wpa_supplicant/hostapd: Fix uninitialized packet pointer on error The packet pointer (called packet) will remain uninitialized when pcap_next_ex() returns an error. This occurs when the wlan interface is shut down using ifconfig destroy. Adding a NULL assignment to packet duplicates what pcap_next() does. The reason we use pcap_next_ex() in this instance is because with pacp_next() when we receive a null pointer if there was an error or if no packets were read. With pcap_next_ex() we can differentiate between an error and legitimately no packets were received. PR: 270649 Reported by: Robert Morris <rtm@lcs.mit.edu> Fixes: 6e5d01124fd4 (cherry picked from commit 953efa5b200f060564a090ab71b3d7f614a35e3f) contrib/wpa/src/l2_packet/l2_packet_freebsd.c | 1 + 1 file changed, 1 insertion(+) A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=e7f23d81ae4b522701ab73482a3bb3e3a76f6e67 commit e7f23d81ae4b522701ab73482a3bb3e3a76f6e67 Author: Cy Schubert <cy@FreeBSD.org> AuthorDate: 2023-09-12 05:17:18 +0000 Commit: Cy Schubert <cy@FreeBSD.org> CommitDate: 2023-09-12 05:51:16 +0000 net/hostapd: Fix uninitialized packet pointer on error The packet pointer (called packet) will remain uninitialized when pcap_next_ex() returns an error. This occurs when the wlan interface is shut down using ifconfig destroy. Adding a NULL assignment to packet duplicates what pcap_next() does. The reason we use pcap_next_ex() in this instance is because with pacp_next() when we receive a null pointer if there was an error or if no packets were read. With pcap_next_ex() we can differentiate between an error and legitimately no packets were received. PR: 270649, 273696 Obtained from: src 953efa5b200f Reported by: Robert Morris <rtm@lcs.mit.edu> MFH: 2023Q3 net/hostapd/Makefile | 2 +- net/hostapd/files/patch-src_l2__packet_l2__packet__freebsd.c | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=2150c312db2fe116e2ce5024645163948334d1e7 commit 2150c312db2fe116e2ce5024645163948334d1e7 Author: Cy Schubert <cy@FreeBSD.org> AuthorDate: 2023-09-12 05:17:36 +0000 Commit: Cy Schubert <cy@FreeBSD.org> CommitDate: 2023-09-12 05:51:19 +0000 net/hostapd-devel: Fix uninitialized packet pointer on error The packet pointer (called packet) will remain uninitialized when pcap_next_ex() returns an error. This occurs when the wlan interface is shut down using ifconfig destroy. Adding a NULL assignment to packet duplicates what pcap_next() does. The reason we use pcap_next_ex() in this instance is because with pacp_next() when we receive a null pointer if there was an error or if no packets were read. With pcap_next_ex() we can differentiate between an error and legitimately no packets were received. PR: 270649, 273696 Obtained from: src 953efa5b200f Reported by: Robert Morris <rtm@lcs.mit.edu> MFH: 2023Q3 net/hostapd-devel/Makefile | 2 +- net/hostapd-devel/files/patch-src_l2__packet_l2__packet__freebsd.c | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=89484a70b0d26f483df30e43945b22a0df1be941 commit 89484a70b0d26f483df30e43945b22a0df1be941 Author: Cy Schubert <cy@FreeBSD.org> AuthorDate: 2023-09-12 05:15:41 +0000 Commit: Cy Schubert <cy@FreeBSD.org> CommitDate: 2023-09-12 05:51:10 +0000 security/wpa_supplicant: Fix uninitialized packet pointer on error The packet pointer (called packet) will remain uninitialized when pcap_next_ex() returns an error. This occurs when the wlan interface is shut down using ifconfig destroy. Adding a NULL assignment to packet duplicates what pcap_next() does. The reason we use pcap_next_ex() in this instance is because with pacp_next() when we receive a null pointer if there was an error or if no packets were read. With pcap_next_ex() we can differentiate between an error and legitimately no packets were received. PR: 270649, 273696 Obtained from: src 953efa5b200f Reported by: Robert Morris <rtm@lcs.mit.edu> MFH: 2023Q3 security/wpa_supplicant/Makefile | 2 +- .../wpa_supplicant/files/patch-src_l2__packet_l2__packet__freebsd.c | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=a872b8a14f51721830232b127cc6ac27663a903d commit a872b8a14f51721830232b127cc6ac27663a903d Author: Cy Schubert <cy@FreeBSD.org> AuthorDate: 2023-09-12 05:17:05 +0000 Commit: Cy Schubert <cy@FreeBSD.org> CommitDate: 2023-09-12 05:51:13 +0000 security/wpa_supplicant-devel: Fix uninitialized packet pointer on error The packet pointer (called packet) will remain uninitialized when pcap_next_ex() returns an error. This occurs when the wlan interface is shut down using ifconfig destroy. Adding a NULL assignment to packet duplicates what pcap_next() does. The reason we use pcap_next_ex() in this instance is because with pacp_next() when we receive a null pointer if there was an error or if no packets were read. With pcap_next_ex() we can differentiate between an error and legitimately no packets were received. PR: 270649, 273696 Obtained from: src 953efa5b200f Reported by: Robert Morris <rtm@lcs.mit.edu> MFH: 2023Q3 security/wpa_supplicant-devel/Makefile | 2 +- .../patch-src_l2__packet_l2__packet__freebsd.c | 28 +++++++++++++++++++--- 2 files changed, 26 insertions(+), 4 deletions(-) A commit in branch 2023Q3 references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=0b110f1e3e172f3ec29a1ff548ef30adfaa82277 commit 0b110f1e3e172f3ec29a1ff548ef30adfaa82277 Author: Cy Schubert <cy@FreeBSD.org> AuthorDate: 2023-09-12 05:17:05 +0000 Commit: Cy Schubert <cy@FreeBSD.org> CommitDate: 2023-09-15 14:07:43 +0000 security/wpa_supplicant-devel: Fix uninitialized packet pointer on error The packet pointer (called packet) will remain uninitialized when pcap_next_ex() returns an error. This occurs when the wlan interface is shut down using ifconfig destroy. Adding a NULL assignment to packet duplicates what pcap_next() does. The reason we use pcap_next_ex() in this instance is because with pacp_next() when we receive a null pointer if there was an error or if no packets were read. With pcap_next_ex() we can differentiate between an error and legitimately no packets were received. PR: 270649, 273696 Obtained from: src 953efa5b200f Reported by: Robert Morris <rtm@lcs.mit.edu> (cherry picked from commit a872b8a14f51721830232b127cc6ac27663a903d) security/wpa_supplicant-devel/Makefile | 2 +- .../patch-src_l2__packet_l2__packet__freebsd.c | 28 +++++++++++++++++++--- 2 files changed, 26 insertions(+), 4 deletions(-) A commit in branch 2023Q3 references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=6f4c310eecc8e585f5df84ddd2b88cf76f51e543 commit 6f4c310eecc8e585f5df84ddd2b88cf76f51e543 Author: Cy Schubert <cy@FreeBSD.org> AuthorDate: 2023-09-12 05:17:18 +0000 Commit: Cy Schubert <cy@FreeBSD.org> CommitDate: 2023-09-15 14:07:44 +0000 net/hostapd: Fix uninitialized packet pointer on error The packet pointer (called packet) will remain uninitialized when pcap_next_ex() returns an error. This occurs when the wlan interface is shut down using ifconfig destroy. Adding a NULL assignment to packet duplicates what pcap_next() does. The reason we use pcap_next_ex() in this instance is because with pacp_next() when we receive a null pointer if there was an error or if no packets were read. With pcap_next_ex() we can differentiate between an error and legitimately no packets were received. PR: 270649, 273696 Obtained from: src 953efa5b200f Reported by: Robert Morris <rtm@lcs.mit.edu> (cherry picked from commit e7f23d81ae4b522701ab73482a3bb3e3a76f6e67) net/hostapd/Makefile | 2 +- net/hostapd/files/patch-src_l2__packet_l2__packet__freebsd.c | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) A commit in branch 2023Q3 references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=8ac8e6a2a8d6b2c88ede98f89c7f3b7f7a066c9a commit 8ac8e6a2a8d6b2c88ede98f89c7f3b7f7a066c9a Author: Cy Schubert <cy@FreeBSD.org> AuthorDate: 2023-09-12 05:17:36 +0000 Commit: Cy Schubert <cy@FreeBSD.org> CommitDate: 2023-09-15 14:07:44 +0000 net/hostapd-devel: Fix uninitialized packet pointer on error The packet pointer (called packet) will remain uninitialized when pcap_next_ex() returns an error. This occurs when the wlan interface is shut down using ifconfig destroy. Adding a NULL assignment to packet duplicates what pcap_next() does. The reason we use pcap_next_ex() in this instance is because with pacp_next() when we receive a null pointer if there was an error or if no packets were read. With pcap_next_ex() we can differentiate between an error and legitimately no packets were received. PR: 270649, 273696 Obtained from: src 953efa5b200f Reported by: Robert Morris <rtm@lcs.mit.edu> (cherry picked from commit 2150c312db2fe116e2ce5024645163948334d1e7) net/hostapd-devel/Makefile | 2 +- net/hostapd-devel/files/patch-src_l2__packet_l2__packet__freebsd.c | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) A commit in branch 2023Q3 references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=45703ac1172cc56d8f2b3bebf57309c87b7ee85f commit 45703ac1172cc56d8f2b3bebf57309c87b7ee85f Author: Cy Schubert <cy@FreeBSD.org> AuthorDate: 2023-09-12 05:15:41 +0000 Commit: Cy Schubert <cy@FreeBSD.org> CommitDate: 2023-09-15 14:07:43 +0000 security/wpa_supplicant: Fix uninitialized packet pointer on error The packet pointer (called packet) will remain uninitialized when pcap_next_ex() returns an error. This occurs when the wlan interface is shut down using ifconfig destroy. Adding a NULL assignment to packet duplicates what pcap_next() does. The reason we use pcap_next_ex() in this instance is because with pacp_next() when we receive a null pointer if there was an error or if no packets were read. With pcap_next_ex() we can differentiate between an error and legitimately no packets were received. PR: 270649, 273696 Obtained from: src 953efa5b200f Reported by: Robert Morris <rtm@lcs.mit.edu> (cherry picked from commit 89484a70b0d26f483df30e43945b22a0df1be941) security/wpa_supplicant/Makefile | 2 +- .../wpa_supplicant/files/patch-src_l2__packet_l2__packet__freebsd.c | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) |
l2_packet_receive() in contrib/wpa/src/l2_packet/l2_packet_freebsd.c: const u_char *packet; if (pcap_next_ex(pcap, &hdr, &packet) == -1) { wpa_printf(MSG_ERROR, "Error reading packet, has device disappeared?"); eloop_terminate(); } if (!l2->rx_callback || !packet || hdr->caplen < sizeof(*ethhdr)) return; ethhdr = (struct l2_ethhdr *) packet; l2->rx_callback(l2->rx_callback_ctx, ethhdr->h_source, buf, len); Since packet is not initialized, and pcap_next_ex() doesn't set packet if there's an error, packet can be left containing non-NULL garbage, which l2->rx_callback() tries to use. This happens if the wlanX interface is shut down with ifconfig destroy. Here's a backtrace: #0 ap_get_sta (hapd=0x412026b0, sta=0xc32fe8354dfa3e76 <error: Cannot access memory at address 0xc32fe8354dfa3e76>) at /usr/rtm/symbsd/src/contrib/wpa/src/ap/sta_info.c:73 #1 0x000000000015eca0 in hostapd_event_eapol_rx (hapd=0x412026b0, src=0xc32fe8354dfa3e76 <error: Cannot access memory at address 0xc32fe8354dfa3e76>, data=0xc32fe8354dfa3e7e <error: Cannot access memory at address 0xc32fe8354dfa3e7e>, data_len=224) at /usr/rtm/symbsd/src/contrib/wpa/src/ap/drv_callbacks.c:1541 #2 wpa_supplicant_event (ctx=0x412026b0, event=<optimized out>, data=<optimized out>) at /usr/rtm/symbsd/src/contrib/wpa/src/ap/drv_callbacks.c:1938 #3 0x0000000000156850 in drv_event_eapol_rx (ctx=0x412026b0, src=<optimized out>, data=<optimized out>, data_len=<optimized out>) at /usr/rtm/symbsd/src/contrib/wpa/src/drivers/driver.h:6085 #4 handle_read (ctx=<optimized out>, src_addr=0xc32fe8354dfa3e76 <error: Cannot access memory at address 0xc32fe8354dfa3e76>, buf=0xc32fe8354dfa3e7e <error: Cannot access memory at address 0xc32fe8354dfa3e7e>, len=224) at /usr/rtm/symbsd/src/contrib/wpa/src/drivers/driver_bsd.c:1028 #5 0x0000000000180b78 in l2_packet_receive (sock=<optimized out>, eloop_ctx=0x41203410, sock_ctx=<optimized out>) at /usr/rtm/symbsd/src/contrib/wpa/src/l2_packet/l2_packet_freebsd.c:102 #6 0x00000000001bace6 in eloop_sock_table_dispatch (fds=0x41209260, table=<optimized out>) at /usr/rtm/symbsd/src/contrib/wpa/src/utils/eloop.c:603 #7 eloop_run () at /usr/rtm/symbsd/src/contrib/wpa/src/utils/eloop.c:1233 #8 0x000000000015784e in hostapd_global_run (ifaces=<optimized out>, daemonize=<optimized out>, pid_file=<optimized out>) at /usr/rtm/symbsd/src/contrib/wpa/hostapd/main.c:445 #9 0x000000000015740a in main (argc=<optimized out>, argv=0x3fffffeb50) at /usr/rtm/symbsd/src/contrib/wpa/hostapd/main.c:892