| Message ID | 1423188434-5830-1-git-send-email-masashi.honma@gmail.com |
|---|---|
| State | Not Applicable |
| Headers | show |
On Fri, Feb 06, 2015 at 11:07:14AM +0900, Masashi Honma wrote: > The valgrind-3.10.0 outputs following message on Ubuntu 14.10 64bit. > > ==2942== Syscall param socketcall.bind(my_addr.sa_data) points to uninitialised byte(s) > ==2942== at 0x5ED3577: bind (syscall-template.S:81) > ==2942== by 0x4AB2FE: l2_packet_init (l2_packet_linux.c:211) This looks very strange.. > This does not occur on Ubuntu 14.10 32bit. So this looks padding problem. > The size of struct sockaddr_ll is 20 bytes. This value is dividable by 4 but not > 8. This patch replace the struct by struct sockaddr_storage which is designed by > RFC 2553 to cover all types of sockaddr and have enough padding for 64bit. I was able to reproduce this on Ubuntu 14.10 64-bit, but don't see it on Ubuntu 14.04 64-bit which seems to be using the same version of valgrind. gcc version is different, though. It looks like adding just two bytes to the sockaddr_ll buffer is enough to make valgrind not complain. I'm not sure how padding could really cause this, though, taken into account how valgrind code for this is implemented in pre_mem_read_sockaddr(). That implementation is not aware of sockaddr_ll (maybe it would be a good idea to make it aware of that..), but it seems to be checking the area following sa_family (i.e., sll_family in sockaddr_ll) in a way that should not go beyond the 20-byte buffer regardless of how the fields are padded. There is a note there to point out possibility of false positives due to padding bytes, but that would not really be applicable here since the l2_packet_linux.c implementation clear the full buffer with memset (and well, there is not really any padding octets in sockaddr_ll, i.e., all those 20 bytes are within the real data fields). > diff --git a/src/l2_packet/l2_packet_linux.c b/src/l2_packet/l2_packet_linux.c > @@ -87,7 +87,10 @@ int l2_packet_get_own_addr(struct l2_packet_data *l2, u8 *addr) > int l2_packet_send(struct l2_packet_data *l2, const u8 *dst_addr, u16 proto, > const u8 *buf, size_t len) > { > + struct sockaddr_storage storage; > + struct sockaddr_ll *ll; I'm not completely sure what is triggering the issue, but the code here in l2_packet_linux.c looks just fine as-is and it would be unfortunate if this type of workarounds are needed just to make valgrind happy. I'd rather figure out what is the real reason behind this bogus warning and fix that rather than change l2_packet_linux.c unless someone can point a clear bug there.
On Sat, Feb 07, 2015 at 01:41:34PM +0200, Jouni Malinen wrote: > It looks like adding just two bytes to the sockaddr_ll buffer is enough > to make valgrind not complain. I'm not sure how padding could really > cause this, though, taken into account how valgrind code for this is > implemented in pre_mem_read_sockaddr(). That implementation is not aware > of sockaddr_ll (maybe it would be a good idea to make it aware of > that..), but it seems to be checking the area following sa_family (i.e., > sll_family in sockaddr_ll) in a way that should not go beyond the > 20-byte buffer regardless of how the fields are padded. Well.. I was obviously looking at the trunk version of Valgrind rather than 3.10.0 that is used in Ubuntu 14.10. This was indeed broken and fixed after that release: https://bugs.kde.org/show_bug.cgi?id=342221 I confirmed that the issue does not show up with the current Valgrind trunk snapshot. As such, I'm dropping this l2_packet patch since this was caused by a Valgrind bug that has already been fixed (but not yet included in a formal release).
2015-02-07 23:58 GMT+09:00 Jouni Malinen <j@w1.fi>: > On Sat, Feb 07, 2015 at 01:41:34PM +0200, Jouni Malinen wrote: > > It looks like adding just two bytes to the sockaddr_ll buffer is enough > > to make valgrind not complain. I'm not sure how padding could really > > cause this, though, taken into account how valgrind code for this is > > implemented in pre_mem_read_sockaddr(). That implementation is not aware > > of sockaddr_ll (maybe it would be a good idea to make it aware of > > that..), but it seems to be checking the area following sa_family (i.e., > > sll_family in sockaddr_ll) in a way that should not go beyond the > > 20-byte buffer regardless of how the fields are padded. > > Well.. I was obviously looking at the trunk version of Valgrind rather > than 3.10.0 that is used in Ubuntu 14.10. This was indeed broken and > fixed after that release: > https://bugs.kde.org/show_bug.cgi?id=342221 > > I confirmed that the issue does not show up with the current Valgrind > trunk snapshot. As such, I'm dropping this l2_packet patch since this > was caused by a Valgrind bug that has already been fixed (but not yet > included in a formal release). > Thank you and sorry for false report.
diff --git a/src/l2_packet/l2_packet_linux.c b/src/l2_packet/l2_packet_linux.c index 68b2008..bef08e2 100644 --- a/src/l2_packet/l2_packet_linux.c +++ b/src/l2_packet/l2_packet_linux.c @@ -87,7 +87,10 @@ int l2_packet_get_own_addr(struct l2_packet_data *l2, u8 *addr) int l2_packet_send(struct l2_packet_data *l2, const u8 *dst_addr, u16 proto, const u8 *buf, size_t len) { + struct sockaddr_storage storage; + struct sockaddr_ll *ll; int ret; + if (l2 == NULL) return -1; if (l2->l2_hdr) { @@ -96,15 +99,15 @@ int l2_packet_send(struct l2_packet_data *l2, const u8 *dst_addr, u16 proto, wpa_printf(MSG_ERROR, "l2_packet_send - send: %s", strerror(errno)); } else { - struct sockaddr_ll ll; - os_memset(&ll, 0, sizeof(ll)); - ll.sll_family = AF_PACKET; - ll.sll_ifindex = l2->ifindex; - ll.sll_protocol = htons(proto); - ll.sll_halen = ETH_ALEN; - os_memcpy(ll.sll_addr, dst_addr, ETH_ALEN); - ret = sendto(l2->fd, buf, len, 0, (struct sockaddr *) &ll, - sizeof(ll)); + os_memset(&storage, 0, sizeof(storage)); + ll = (struct sockaddr_ll *) &storage; + ll->sll_family = AF_PACKET; + ll->sll_ifindex = l2->ifindex; + ll->sll_protocol = htons(proto); + ll->sll_halen = ETH_ALEN; + os_memcpy(ll->sll_addr, dst_addr, ETH_ALEN); + ret = sendto(l2->fd, buf, len, 0, (struct sockaddr *) ll, + sizeof(*ll)); if (ret < 0) { wpa_printf(MSG_ERROR, "l2_packet_send - sendto: %s", strerror(errno)); @@ -174,7 +177,8 @@ struct l2_packet_data * l2_packet_init( { struct l2_packet_data *l2; struct ifreq ifr; - struct sockaddr_ll ll; + struct sockaddr_storage storage; + struct sockaddr_ll *ll; l2 = os_zalloc(sizeof(struct l2_packet_data)); if (l2 == NULL) @@ -204,11 +208,12 @@ struct l2_packet_data * l2_packet_init( } l2->ifindex = ifr.ifr_ifindex; - os_memset(&ll, 0, sizeof(ll)); - ll.sll_family = PF_PACKET; - ll.sll_ifindex = ifr.ifr_ifindex; - ll.sll_protocol = htons(protocol); - if (bind(l2->fd, (struct sockaddr *) &ll, sizeof(ll)) < 0) { + os_memset(&storage, 0, sizeof(storage)); + ll = (struct sockaddr_ll *) &storage; + ll->sll_family = PF_PACKET; + ll->sll_ifindex = ifr.ifr_ifindex; + ll->sll_protocol = htons(protocol); + if (bind(l2->fd, (struct sockaddr *) ll, sizeof(*ll)) < 0) { wpa_printf(MSG_ERROR, "%s: bind[PF_PACKET]: %s", __func__, strerror(errno)); close(l2->fd);
The valgrind-3.10.0 outputs following message on Ubuntu 14.10 64bit. ==2942== Syscall param socketcall.bind(my_addr.sa_data) points to uninitialised byte(s) ==2942== at 0x5ED3577: bind (syscall-template.S:81) ==2942== by 0x4AB2FE: l2_packet_init (l2_packet_linux.c:211) ==2942== by 0x485147: wpa_supplicant_update_mac_addr (wpa_supplicant.c:3017) ==2942== by 0x48888F: wpa_supplicant_driver_init (wpa_supplicant.c:3078) ==2942== by 0x489A05: wpa_supplicant_init_iface (wpa_supplicant.c:4028) ==2942== by 0x489A05: wpa_supplicant_add_iface (wpa_supplicant.c:4226) ==2942== by 0x41DAB9: main (main.c:325) ==2942== Address 0xfff000274 is on thread 1's stack ==2942== in frame #1, created by l2_packet_init (l2_packet_linux.c:174) ==2942== ==5631== Syscall param socketcall.sendto(to.sa_data) points to uninitialised byte(s) ==5631== at 0x5ED3953: __sendto_nocancel (syscall-template.S:81) ==5631== by 0x4AB061: l2_packet_send (l2_packet_linux.c:106) ==5631== by 0x434BBE: wpa_sm_ether_send (wpa_i.h:181) ==5631== by 0x434BBE: wpa_eapol_key_send (wpa.c:72) ==5631== by 0x435145: wpa_supplicant_send_2_of_4 (wpa.c:401) ==5631== by 0x419346: wpa_supplicant_process_1_of_4 (wpa.c:516) ==5631== by 0x43668C: wpa_sm_rx_eapol (wpa.c:1958) ==5631== by 0x48E6A4: wpa_supplicant_event_assoc (events.c:2046) ==5631== by 0x48E6A4: wpa_supplicant_event (events.c:3039) ==5631== by 0x4A4B59: mlme_event_assoc (driver_nl80211_event.c:260) ==5631== by 0x4A6AF5: do_process_drv_event (driver_nl80211_event.c:1751) ==5631== by 0x4A6AF5: process_global_event (driver_nl80211_event.c:1878) ==5631== by 0x53834CE: nl_recvmsgs_report (in /lib/x86_64-linux-gnu/libnl-3.so.200.19.0) ==5631== by 0x5383898: nl_recvmsgs (in /lib/x86_64-linux-gnu/libnl-3.so.200.19.0) ==5631== by 0x494297: wpa_driver_nl80211_event_receive (driver_nl80211.c:1313) ==5631== Address 0xffefff384 is on thread 1's stack ==5631== in frame #1, created by l2_packet_send (l2_packet_linux.c:89) This does not occur on Ubuntu 14.10 32bit. So this looks padding problem. The size of struct sockaddr_ll is 20 bytes. This value is dividable by 4 but not 8. This patch replace the struct by struct sockaddr_storage which is designed by RFC 2553 to cover all types of sockaddr and have enough padding for 64bit. Signed-off-by: Masashi Honma <masashi.honma@gmail.com> --- src/l2_packet/l2_packet_linux.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-)