ConnMan upgrade to 1.38 #74
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LaakkonenJussi
force-pushed
the
connman_upgrade_to_1.38
branch
from
f172b03
to
c51af33
Compare
In disconnect_callback() wifi->network is used, but it seems that callback can arrive without wifi->network being set. The call to connman_network_set_connected(wifi->network, false) will then be called and deref that null pointer, making connman crash. So, don't call connman_network_set_connected() if wifi->network is null. (cherry picked from commit de5220c)
We can drop start_online_check() function declaration now by reordering the helper functions and move them in the right order. (cherry picked from commit 529f7a5) ConnMan 1.38 upgrade note: - We had this all except the function prototype removal. Keep in sync.
Instead parsing the error message in every callback parse it centrally and just parse the error code down. (cherry picked from commit 08aa7ad)
Don't print 'Error' in all non successful cases. Instead print helpful information such as 'TECH is not available'. (cherry picked from commit dfd3f2a)
The background scanning is intended for potential roaming purpose. An RSSI of -45 is a really good signal level and should not trigger a background scan intended for roaming. -65 is the beginning of wifi trouble and most likely we may want to move to a better BSS. (cherry picked from commit de56229)
Move nameserver duplicate check down into the if statemenet. This avoids to check twice if the variable nameserver is not NULL. (cherry picked from commit 1cb8a06)
While matching on the MAC address is working for plain interfaces, it wont work for managing VLAN interfaces. The VLAN interfaces share the same MAC address with the parent interface. The argument that the MAC address is the only stable way to identify a device is a bit weak because the MAC can be changed via udev or other means. Furthermore, with systemd's feature of stable interface name it makes things a lot easier for embedded system with a pre-provisioning rootfs. Such devices have different MAC address but all of them have the same interface name. (cherry picked from commit a719719)
In musl > 1.1.21 freeaddrinfo() implementation changed and was causing a segmentation fault. (cherry picked from commit 594b7cb)
(cherry picked from commit a99ad5d)
(cherry picked from commit 2589fda)
Methods of reproducing:
in loop
1) connmanctl tether wifi on my_ssid my_pasword
2) conencting client
3) connmanctl tether wifi off
con[14819.539062] tether: port 1(wlan0) entered disabled state
nmand2[3831]: ../git/src/technology.c:set_property() property Tethering
connmand2[3831]: ../git/plugins/wifi.c:tech_set_tethering()
connmand2[3831]: ../git/src/technology.c:connman_technology_tethering_notify() technology 0xb57006e0 enabled 0
connmand2[3831]: ../git/src/tethering.c:__connman_tethering_set_disabled() enabled 0
=================================================================
==3831==ERROR: AddressSanitizer: heap-use-after-free on address 0xb490c370 at pc 0x41c2e9c0 bp 0xbedf7494 sp 0xbedf7060
READ of size 2 at 0xb490c370 thread T0
#0 0x41c2e9bf (/usr/lib/libasan.so.5+0x41c2e9bf)
0xb490c370 is located 0 bytes inside of 18-byte region [0xb490c370,0xb490c382)
freed by thread T0 here:
#0 0x41c73ee7 in free (/usr/lib/libasan.so.5+0x41c73ee7)
#1 0x42877473 (/usr/lib/libglib-2.0.so.0+0x42877473)
previously allocated by thread T0 here:
#0 0x41c7421b in malloc (/usr/lib/libasan.so.5+0x41c7421b)
#1 0x42890b8b in g_malloc (/usr/lib/libglib-2.0.so.0+0x42890b8b)
sailfishos#2 0x9d3a7 in sta_authorized ../git/plugins/wifi.c:3004
sailfishos#3 0xa79eb in callback_sta_authorized ../git/gsupplicant/supplicant.c:626
sailfishos#4 0xc3dd7 in signal_sta_authorized ../git/gsupplicant/supplicant.c:2779
sailfishos#5 0xceb2f in g_supplicant_filter ../git/gsupplicant/supplicant.c:3620
sailfishos#6 0x419fb123 in dbus_connection_dispatch (/usr/lib/libdbus-1.so.3+0x419fb123)
sailfishos#7 0xb2501d17 (<unknown module>)
SUMMARY: AddressSanitizer: heap-use-after-free (/usr/lib/libasan.so.5+0x41c2e9bf)
Shadow bytes around the buggy address:
0x36921810: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x36921820: fa fa fa fa fa fa fa fa fa fa fa fa fd fd fd fd
0x36921830: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x36921840: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x36921850: fa fa fa fa fa fa 00 00 00 fa fa fa fa fa fa fa
=>0x36921860: fa fa 00 00 00 00 fa fa fa fa fa fa fa fa[fd]fd
0x36921870: fd fa fa fa 00 00 00 00 fa fa fa fa fa fa fa fa
0x36921880: fa fa fa fa fa fa 00 00 00 04 fa fa fa fa fa fa
0x36921890: fa fa fa fa fa fa fa fa fd fd fd fd fa fa fa fa
0x369218a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x369218b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
==3831==ABORTING
(cherry picked from commit 799334d)
../git/gweb/gresolv.c:331:7: runtime error: left shift of 169 by 24
places cannot be represented in type 'int'
connmand2[3417]: eth0 {add} route 192.168.2.0 gw 0.0.0.0 scope 253 <LINK>
(cherry picked from commit cacafef)
Replace screenscraping with libopenconnect in the plugin. (cherry picked from commit d891cf5)
(cherry picked from commit 2974287)
When a service switch from a autoconf link local address (169.254.0.0/16) to an address configured by DCHP, timeserver was not notified and thus was not reloaded its resolver's nameserver. As a consequence, hostnames of NTP server were not resolved. (cherry picked from commit da51f2d)
Instead searching for libmnl and nftables in one test, split into two seperate tests. We need libmnl for WireGuard too. (cherry picked from commit 4e1546b)
Add an empty new plugin for WireGuard support. (cherry picked from commit dc9d71a)
mnlg.c and mnlg.h are a copy from iproute2. The call to nl_dump_ext_ack() and nl_dump_ext_ack_done() have been removed from the code to avoid additional dependencies. git://git.kernel.org/pub/scm/network/iproute2/iproute2.git d035cc1b4e83e2589ea2115cdc2fa7c6d3693a5a The helpers are needed for the WireGuard VPN plugin. (cherry picked from commit 3f156e3)
Add a copy of the embeddable WireGuard library code. The embedded libmnl and mngl code have been removed. https://github.com/WireGuard/WireGuard tag 20190531 (cherry picked from commit 0079aba)
Add VPN_FLAG_NO_DAEMON to skip forking and monitoring an external task. Since the state machine inside vpn.c is driven by the external events we need to fake the event via update_provider_state(). We can't directly progress the state machine because we are still processing an D-Bus message after calling vpn_driver->connect(). So defer the call to update_provider_state via the idle loop. (cherry picked from commit 12f095a)
(cherry picked from commit 244c2a6)
Implement a bare minimum for WireGuard support. The parser only understands minium, e.g. only one peer. To create and manage the WireGuard devices the upstream example code is used. (cherry picked from commit 95b2514)
Use POSIX APIs to implement a simple replacement for GDateTime. The Glib code uses gettimeofday() to implement g_date_time_new_now(), so use the same interface to avoid breakage. Furthermore, the Glib code writes the timestamps with sub seconds values, e.g. "2019-10-20T14:45:31.079935Z". The new code only writes with second precision. That means the string parser needs to be able to deal with the old time and new format. (cherry picked from commit 6447251)
The g_date_time_new_from_iso8601() was introduced with GLib v2.56. We don't want to update our version dependency. Instead use our replacement API. (cherry picked from commit 0d90703)
GTimeVal has been deprecated. Replace it with our own replacement API. (cherry picked from commit e515af3)
The g_date_time_new_from_iso8601() was introduced with GLib v2.56. We don't want to update our version dependency. So let's use good old POSIX APIs instead. (cherry picked from commit 1ccb76d)
(cherry picked from commit 3a1398a)
Use freeaddrinfo after rp has been used. rp points to an element in results. (cherry picked from commit 4b8b2e3)
The info data structure should be properly initialized. #0 0x00007ffff7bd98f7 in __memmove_avx_unaligned () from /lib64/libc.so.6 #1 0x00007ffff7fa4925 in mnl_attr_put () from /usr/lib64/libmnl.so.0 sailfishos#2 0x00007ffff7fa496f in mnl_attr_put_check () from /usr/lib64/libmnl.so.0 sailfishos#3 0x000000000040dd95 in wg_set_device (dev=0x480c70) at vpn/plugins/libwireguard.c:341 sailfishos#4 0x0000000000410dd7 in wg_connect (provider=0x47e100, task=0x0, if_name=0x0, cb=0x0, dbus_sender=0x0, user_data=0x0) at vpn/plugins/wireguard.c:336 sailfishos#5 0x0000000000413092 in vpn_connect (provider=0x47e100, cb=0x41e946 <connect_cb>, dbus_sender=0x481dd4 ":1.46", user_data=0x475a10) at vpn/plugins/vpn.c:632 sailfishos#6 0x000000000041ec52 in __vpn_provider_connect (provider=0x47e100, msg=0x475a10) at vpn/vpn-provider.c:1206 sailfishos#7 0x000000000041d488 in do_connect (conn=0x473ce0, msg=0x475a10, data=0x47e100) at vpn/vpn-provider.c:505 sailfishos#8 0x000000000041d4da in do_connect2 (conn=0x473ce0, msg=0x475a10, data=0x47e100) at vpn/vpn-provider.c:515 sailfishos#9 0x0000000000435691 in process_message (connection=0x473ce0, message=0x475a10, method=0x43bfe0 <connection_methods+160>, iface_user_data=0x47e100) at gdbus/object.c:259 sailfishos#10 0x00000000004371a5 in generic_message (connection=0x473ce0, message=0x475a10, user_data=0x47f340) at gdbus/object.c:1071 sailfishos#11 0x00007ffff7e4fc4d in ?? () from /usr/lib64/libdbus-1.so.3 sailfishos#12 0x00007ffff7e407a4 in dbus_connection_dispatch () from /usr/lib64/libdbus-1.so.3 sailfishos#13 0x000000000043306b in message_dispatch (data=0x473ce0) at gdbus/mainloop.c:72 sailfishos#14 0x00007ffff7eca9f7 in ?? () from /usr/lib64/libglib-2.0.so.0 sailfishos#15 0x00007ffff7ecdf88 in g_main_context_dispatch () from /usr/lib64/libglib-2.0.so.0 sailfishos#16 0x00007ffff7ece310 in ?? () from /usr/lib64/libglib-2.0.so.0 sailfishos#17 0x00007ffff7ece5e3 in g_main_loop_run () from /usr/lib64/libglib-2.0.so.0 sailfishos#18 0x0000000000419f6f in main (argc=1, argv=0x7fffffffec28) at vpn/main.c:275 (cherry picked from commit c8ed849)
There is no setter for the VPN type member. Remove it since it has no functional use. (cherry picked from commit aa11712)
The private data should be cleared on disconnect to avoid an invalid access. For example the user could call disconnect twice which leads to a crash. (cherry picked from commit e355e20)
Currently all connection attempts are reported as CONNMAN_NETWORK_ERROR_CONNECT_FAIL. When the user enters an wrong passphrase the upper layers wont reset the passphrase unless CONNMAN_NETWORK_ERROR_INVALID_KEY is reported. This prevents the user to enter a new passphrase. Unfortunately, when the passphrase is correct but the connect attempt fails with 'Failed', ConnMan will reset the passphrase. But this behavoir is way better then not being able to provide the correct passphrase. Reported-by: Maxime Roussin-Bélanger <maxime.roussinbelanger@gmail.com> (cherry picked from commit 262d475)
The connman_network_set_strength() setter is not propagating the new value. Reported-by: Maxime Roussin-Bélanger <maxime.roussinbelanger@gmail.com> (cherry picked from commit 5dc2cd1)
Older kernel headers are missing definition for NETLINK_CAP_ACK and NETLINK_EXT_ACK. Add them so that ConnMan can be build on older kernels as well. Furthermore, the WireGuard project has been backported to older kernels such as the LTS v4.4 kernel. It's far from ideal but those definition don't hurt too much so we add them here for the time being. Reported by Christian Hewitt. (cherry picked from commit 575190f)
Document the useles effort to handle small memory allocations. Even
the small test program below shows glibc's malloc wont return a NULL allocation. The
program will be killed by the OOM.
int main(int argc, char *argv[])
{
while (1) {
if (!malloc(16))
exit(3);
}
return 0;
}
$ ./malloc
[1] 25788 killed ./malloc
$ echo $?
137
[ 2729.844036] Out of memory: Killed process 25745 (malloc) total-vm:15131480kB, anon-rss:14977108kB, file-rss:948kB, shmem-rss:0kB
[ 2730.091798] oom_reaper: reaped process 25745 (malloc), now anon-rss:0kB, file-rss:0kB, shmem-rss:0kB
Linux userland system programming is different to embedded systems
where any sized malloc can and *will* fail.
(cherry picked from commit 1223965)
(cherry picked from commit 1ee420a)
Bumped version to 1.38 also in spec.
Some of our commits had some different locations for functions when pushed to upstream. This syncs the code.
Some of our commits had some different order for the functions when pushed to upstream. This syncs the code.
Enable wireguard build and package it. Add libmnl-devel to build dependencies of ConnMan and libmnl as a dependency for wireguard plugin.
[gdbus] Replace snprintf() with g_strdup_printf(). JB#51243 snprintf returns the number of characters that would have been written if n had been sufficiently large. If the format string is longer than the (size - offset), the snprintf will return a value larger than the (size-offset). In normal cases, DBUS_MAXIMUM_MATCH_RULE_LENGTH(1024) is large, but an attacker can make malicious, large-scale inputs. Note, g_strdup() and g_strdup_printf() will call abort if no memory can't be allocated. Therefore we don't need to check for NULL pointers. Reported by jybarnes21 (cherry picked from commit ff95ad8)
[wireguard] Regular reresolve endpoint address. JB#51243 In case the WireGuard endpoint is hosted on a dynamic IP address, the endpoint might change during runtime. Reresolve the endpoint on a regular basis and update the WireGuard device when it IP address has changed. Reported by Christian Hewitt (cherry picked from commit 90592f7)
[wireguard] Fix struct sockaddr usage. JB#51243 wg_dns_reresolve_cb() tries to read 16 bytes from the addr variable in case of IPv6 but struct sockaddr has size of 8 bytes. Introduce a helper struct containing IPv4 and IPv6 sockaddr as superset of all IP based sockaddr. This helper struct is identically to the 'union endpoint' in wireguard.h. But we don't want to touch this header file as it is 100% copy from the upstream WireGuard project. Fixes: 90592f7 ("wireguard: Regular reresolve endpoint address") (cherry picked from commit ce76787)
Adding broadcast address for VPNs is not permitted by "ip route" and it messes up the use of getifaddrs() for getting the destination address. Adding broadcast address for P2P connections is also unnecessary. (cherry picked from commit 255115d with wireguard bits only)
[vpn] Export vpn_ipconfig_foreach as linker symbol. JB#51243 Commit 95b2514 ("vpn: Add WireGuard support") introduced a plugin dependency to __vpn_ipconfig_foreach. Because the linker does not export the prefixed functions, we need to rename it to vpn_ipconfig_foreach in order to export the linker symbol. (cherry picked from commit 9ccac2d)
[wireguard] Copy interfance names obeying lengths rules. JB#51243 gcc points out the destination buffer has the same size the specified bound for the string. warning: ‘__builtin_strncpy’ specified bound 16 equals destination size [-Wstringop-truncation] Let's make sure we do not overflow the buffer (should not happen as the names are provide by the kernel and hence should fit). (cherry picked from commit 90f0e41)
LaakkonenJussi
force-pushed
the
connman_upgrade_to_1.38
branch
from
c51af33
to
bfcaa2b
Compare
[wireguard] Add saving of provider properties. JB#51243 Save all provider properties to provider configuration file. This follows the example defined in doc/vpn-config-format.txt
[vpn] Always call the vpn provider connect callback. JB#51243 In order to track the state and the errors properly add the callback to every VPN connect call. This was missing for the no daemon type VPNs. The errors now propagate on this to higher level as well, and does not make the VPN autoconnection based approaches confused and stuck.
[wireguard] Use positive errors for vpn provider connect_cb. JB#51243 The vpn-provider.c:connect_cb() expects the errors to be positive ints. Remove the sign when calling the cb. And add debug to see what is really causing the error. For example, when parsing fails.
LaakkonenJussi
force-pushed
the
connman_upgrade_to_1.38
branch
from
5fb82af
to
b60aeb2
Compare
mlehtima
approved these changes
Jul 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Upgrade ConnMan to 1.38. Most of the changes are for WireGuard (new) and iwd. There are some changes to parts we do not use. Only notable change is eaa871b to change how ipaddress is cleared.