Make rx_bytes & tx_bytes 64-bit rather than 32-bit and populate using NL80211_STA_INFO_RX_BYTES64 and NL80211_STA_INFO_TX_BYTES64 where support for these are available. This resolves the race vulnerable 32-bit value wrap/overflow. Rework RADIUS accounting to use these for Acct-Input-Octets, Acct-Input-Gigawords, Acct-Output-Octets and Acct-Output-Gigawords, these values are often used for billing purposes. Signed-off-by: Nick Lowe <nick.lowe@xxxxxxxxxxxx> --- src/ap/accounting.c | 42 ++++++++++++++---------------------------- src/ap/ctrl_iface_ap.c | 2 +- src/ap/sta_info.h | 6 ++---- src/drivers/driver.h | 3 ++- src/drivers/driver_hostap.c | 10 ++++------ src/drivers/driver_nl80211.c | 36 ++++++++++++++++++++++++++++++++---- 6 files changed, 55 insertions(+), 44 deletions(-) diff --git a/src/ap/accounting.c b/src/ap/accounting.c index f3ce121..2702356 100644 --- a/src/ap/accounting.c +++ b/src/ap/accounting.c @@ -167,19 +167,17 @@ static int accounting_sta_update_stats(struct hostapd_data *hapd, if (hostapd_drv_read_sta_data(hapd, data, sta->addr)) return -1; - if (sta->last_rx_bytes > data->rx_bytes) - sta->acct_input_gigawords++; - if (sta->last_tx_bytes > data->tx_bytes) - sta->acct_output_gigawords++; sta->last_rx_bytes = data->rx_bytes; sta->last_tx_bytes = data->tx_bytes; hostapd_logger(hapd, sta->addr, HOSTAPD_MODULE_RADIUS, HOSTAPD_LEVEL_DEBUG, "updated TX/RX stats: " - "Acct-Input-Octets=%lu Acct-Input-Gigawords=%u " - "Acct-Output-Octets=%lu Acct-Output-Gigawords=%u", - sta->last_rx_bytes, sta->acct_input_gigawords, - sta->last_tx_bytes, sta->acct_output_gigawords); + "Acct-Input-Octets=%lu Acct-Input-Gigawords=%lu " + "Acct-Output-Octets=%lu Acct-Output-Gigawords=%lu", + (unsigned long)sta->last_rx_bytes, + (unsigned long)(sta->last_rx_bytes >> 32), + (unsigned long)sta->last_tx_bytes, + (unsigned long)(sta->last_tx_bytes >> 32)); return 0; } @@ -225,7 +223,6 @@ void accounting_sta_start(struct hostapd_data *hapd, struct sta_info *sta) os_get_reltime(&sta->acct_session_start); sta->last_rx_bytes = sta->last_tx_bytes = 0; - sta->acct_input_gigawords = sta->acct_output_gigawords = 0; hostapd_drv_sta_clear_stats(hapd, sta->addr); if (!hapd->conf->radius->acct_server) @@ -254,7 +251,6 @@ static void accounting_sta_report(struct hostapd_data *hapd, int cause = sta->acct_terminate_cause; struct hostap_sta_driver_data data; struct os_reltime now_r, diff; - u32 gigawords; if (!hapd->conf->radius->acct_server) return; @@ -290,35 +286,25 @@ static void accounting_sta_report(struct hostapd_data *hapd, } if (!radius_msg_add_attr_int32(msg, RADIUS_ATTR_ACCT_INPUT_OCTETS, - data.rx_bytes)) { + (u32)data.rx_bytes)) { wpa_printf(MSG_INFO, "Could not add Acct-Input-Octets"); goto fail; } - gigawords = sta->acct_input_gigawords; -#if __WORDSIZE == 64 - gigawords += data.rx_bytes >> 32; -#endif - if (gigawords && - !radius_msg_add_attr_int32( - msg, RADIUS_ATTR_ACCT_INPUT_GIGAWORDS, - gigawords)) { + if (!radius_msg_add_attr_int32(msg, + RADIUS_ATTR_ACCT_INPUT_GIGAWORDS, + (u32)(data.rx_bytes >> 32))) { wpa_printf(MSG_INFO, "Could not add Acct-Input-Gigawords"); goto fail; } if (!radius_msg_add_attr_int32(msg, RADIUS_ATTR_ACCT_OUTPUT_OCTETS, - data.tx_bytes)) { + (u32)data.tx_bytes)) { wpa_printf(MSG_INFO, "Could not add Acct-Output-Octets"); goto fail; } - gigawords = sta->acct_output_gigawords; -#if __WORDSIZE == 64 - gigawords += data.tx_bytes >> 32; -#endif - if (gigawords && - !radius_msg_add_attr_int32( - msg, RADIUS_ATTR_ACCT_OUTPUT_GIGAWORDS, - gigawords)) { + if (!radius_msg_add_attr_int32(msg, + RADIUS_ATTR_ACCT_OUTPUT_GIGAWORDS, + (u32)(data.tx_bytes >> 32))) { wpa_printf(MSG_INFO, "Could not add Acct-Output-Gigawords"); goto fail; } diff --git a/src/ap/ctrl_iface_ap.c b/src/ap/ctrl_iface_ap.c index c98978f..4f93ac2 100644 --- a/src/ap/ctrl_iface_ap.c +++ b/src/ap/ctrl_iface_ap.c @@ -35,7 +35,7 @@ static int hostapd_get_sta_tx_rx(struct hostapd_data *hapd, return 0; ret = os_snprintf(buf, buflen, "rx_packets=%lu\ntx_packets=%lu\n" - "rx_bytes=%lu\ntx_bytes=%lu\n", + "rx_bytes=%llu\ntx_bytes=%llu\n", data.rx_packets, data.tx_packets, data.rx_bytes, data.tx_bytes); if (os_snprintf_error(buflen, ret)) diff --git a/src/ap/sta_info.h b/src/ap/sta_info.h index 359f480..114ba65 100644 --- a/src/ap/sta_info.h +++ b/src/ap/sta_info.h @@ -107,10 +107,8 @@ struct sta_info { int acct_terminate_cause; /* Acct-Terminate-Cause */ int acct_interim_interval; /* Acct-Interim-Interval */ - unsigned long last_rx_bytes; - unsigned long last_tx_bytes; - u32 acct_input_gigawords; /* Acct-Input-Gigawords */ - u32 acct_output_gigawords; /* Acct-Output-Gigawords */ + unsigned long long last_rx_bytes; + unsigned long long last_tx_bytes; u8 *challenge; /* IEEE 802.11 Shared Key Authentication Challenge */ diff --git a/src/drivers/driver.h b/src/drivers/driver.h index a4f6704..6734a99 100644 --- a/src/drivers/driver.h +++ b/src/drivers/driver.h @@ -1368,7 +1368,8 @@ struct wpa_driver_capa { struct hostapd_data; struct hostap_sta_driver_data { - unsigned long rx_packets, tx_packets, rx_bytes, tx_bytes; + unsigned long rx_packets, tx_packets; + unsigned long long rx_bytes, tx_bytes; unsigned long current_tx_rate; unsigned long inactive_msec; unsigned long flags; diff --git a/src/drivers/driver_hostap.c b/src/drivers/driver_hostap.c index 517a3bb..6e43cef 100644 --- a/src/drivers/driver_hostap.c +++ b/src/drivers/driver_hostap.c @@ -579,7 +579,6 @@ static int hostap_read_sta_data(void *priv, struct hostap_driver_data *drv = priv; char buf[1024], line[128], *pos; FILE *f; - unsigned long val; memset(data, 0, sizeof(*data)); snprintf(buf, sizeof(buf), "/proc/net/hostap/%s/" MACSTR, @@ -597,15 +596,14 @@ static int hostap_read_sta_data(void *priv, if (!pos) continue; *pos++ = '\0'; - val = strtoul(pos, NULL, 10); if (strcmp(line, "rx_packets") == 0) - data->rx_packets = val; + data->rx_packets = strtoul(pos, NULL, 10); else if (strcmp(line, "tx_packets") == 0) - data->tx_packets = val; + data->tx_packets = strtoul(pos, NULL, 10); else if (strcmp(line, "rx_bytes") == 0) - data->rx_bytes = val; + data->rx_bytes = strtoull(pos, NULL, 20); else if (strcmp(line, "tx_bytes") == 0) - data->tx_bytes = val; + data->tx_bytes = strtoull(pos, NULL, 20); } fclose(f); diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c index 0f312a0..4554006 100644 --- a/src/drivers/driver_nl80211.c +++ b/src/drivers/driver_nl80211.c @@ -5327,7 +5327,13 @@ static int get_sta_handler(struct nl_msg *msg, void *arg) [NL80211_STA_INFO_RX_PACKETS] = { .type = NLA_U32 }, [NL80211_STA_INFO_TX_PACKETS] = { .type = NLA_U32 }, [NL80211_STA_INFO_TX_FAILED] = { .type = NLA_U32 }, +#if defined(NL80211_STA_INFO_RX_BYTES64) && defined(NL80211_STA_INFO_TX_BYTES64) + [NL80211_STA_INFO_RX_BYTES64] = { .type = NLA_U64 }, + [NL80211_STA_INFO_TX_BYTES64] = { .type = NLA_U64 }, +#endif }; + u32 bytes_lo; + u32 bytes_hi; nla_parse(tb, NL80211_ATTR_MAX, genlmsg_attrdata(gnlh, 0), genlmsg_attrlen(gnlh, 0), NULL); @@ -5352,10 +5358,32 @@ static int get_sta_handler(struct nl_msg *msg, void *arg) if (stats[NL80211_STA_INFO_INACTIVE_TIME]) data->inactive_msec = nla_get_u32(stats[NL80211_STA_INFO_INACTIVE_TIME]); - if (stats[NL80211_STA_INFO_RX_BYTES]) - data->rx_bytes = nla_get_u32(stats[NL80211_STA_INFO_RX_BYTES]); - if (stats[NL80211_STA_INFO_TX_BYTES]) - data->tx_bytes = nla_get_u32(stats[NL80211_STA_INFO_TX_BYTES]); +#if defined(NL80211_STA_INFO_RX_BYTES64) && defined(NL80211_STA_INFO_TX_BYTES64) + if (stats[NL80211_STA_INFO_RX_BYTES64]) + data->rx_bytes = nla_get_u64(stats[NL80211_STA_INFO_RX_BYTES64]); + else if (stats[NL80211_STA_INFO_RX_BYTES]) { +#else + if (stats[NL80211_STA_INFO_RX_BYTES]) { +#endif + bytes_lo = nla_get_u32(stats[NL80211_STA_INFO_RX_BYTES]); + bytes_hi = (u32)(data->rx_bytes >> 32); + if (bytes_lo < (u32)data->rx_bytes) + bytes_hi++; + data->rx_bytes = (u64)bytes_hi << 32 | bytes_lo; + } +#if defined(NL80211_STA_INFO_RX_BYTES64) && defined(NL80211_STA_INFO_TX_BYTES64) + if (stats[NL80211_STA_INFO_TX_BYTES64]) + data->tx_bytes = nla_get_u64(stats[NL80211_STA_INFO_TX_BYTES64]); + else if (stats[NL80211_STA_INFO_TX_BYTES]) { +#else + if (stats[NL80211_STA_INFO_TX_BYTES]) { +#endif + bytes_lo = nla_get_u32(stats[NL80211_STA_INFO_TX_BYTES]); + bytes_hi = (u32)(data->tx_bytes >> 32); + if (bytes_lo < (u32)data->tx_bytes) + bytes_hi++; + data->tx_bytes = (u64)bytes_hi << 32 | bytes_lo; + } if (stats[NL80211_STA_INFO_RX_PACKETS]) data->rx_packets = nla_get_u32(stats[NL80211_STA_INFO_RX_PACKETS]); -- 2.7.1
From 3d2766ddc4ead1a2dbcb22a81e47b5c5101f3275 Mon Sep 17 00:00:00 2001 From: Nick Lowe <nick.lowe@xxxxxxxxxxxx> Date: Fri, 19 Feb 2016 10:22:32 +0000 Subject: [PATCH] Make rx_bytes & tx_bytes 64-bit rather than 32-bit and populate using NL80211_STA_INFO_RX_BYTES64 and NL80211_STA_INFO_TX_BYTES64 where support for these are available. This resolves the race vulnerable 32-bit value wrap/overflow. Rework RADIUS accounting to use these for Acct-Input-Octets, Acct-Input-Gigawords, Acct-Output-Octets and Acct-Output-Gigawords, these values are often used for billing purposes. Signed-off-by: Nick Lowe <nick.lowe@xxxxxxxxxxxx> --- src/ap/accounting.c | 42 ++++++++++++++---------------------------- src/ap/ctrl_iface_ap.c | 2 +- src/ap/sta_info.h | 6 ++---- src/drivers/driver.h | 3 ++- src/drivers/driver_hostap.c | 10 ++++------ src/drivers/driver_nl80211.c | 36 ++++++++++++++++++++++++++++++++---- 6 files changed, 55 insertions(+), 44 deletions(-) diff --git a/src/ap/accounting.c b/src/ap/accounting.c index f3ce121..2702356 100644 --- a/src/ap/accounting.c +++ b/src/ap/accounting.c @@ -167,19 +167,17 @@ static int accounting_sta_update_stats(struct hostapd_data *hapd, if (hostapd_drv_read_sta_data(hapd, data, sta->addr)) return -1; - if (sta->last_rx_bytes > data->rx_bytes) - sta->acct_input_gigawords++; - if (sta->last_tx_bytes > data->tx_bytes) - sta->acct_output_gigawords++; sta->last_rx_bytes = data->rx_bytes; sta->last_tx_bytes = data->tx_bytes; hostapd_logger(hapd, sta->addr, HOSTAPD_MODULE_RADIUS, HOSTAPD_LEVEL_DEBUG, "updated TX/RX stats: " - "Acct-Input-Octets=%lu Acct-Input-Gigawords=%u " - "Acct-Output-Octets=%lu Acct-Output-Gigawords=%u", - sta->last_rx_bytes, sta->acct_input_gigawords, - sta->last_tx_bytes, sta->acct_output_gigawords); + "Acct-Input-Octets=%lu Acct-Input-Gigawords=%lu " + "Acct-Output-Octets=%lu Acct-Output-Gigawords=%lu", + (unsigned long)sta->last_rx_bytes, + (unsigned long)(sta->last_rx_bytes >> 32), + (unsigned long)sta->last_tx_bytes, + (unsigned long)(sta->last_tx_bytes >> 32)); return 0; } @@ -225,7 +223,6 @@ void accounting_sta_start(struct hostapd_data *hapd, struct sta_info *sta) os_get_reltime(&sta->acct_session_start); sta->last_rx_bytes = sta->last_tx_bytes = 0; - sta->acct_input_gigawords = sta->acct_output_gigawords = 0; hostapd_drv_sta_clear_stats(hapd, sta->addr); if (!hapd->conf->radius->acct_server) @@ -254,7 +251,6 @@ static void accounting_sta_report(struct hostapd_data *hapd, int cause = sta->acct_terminate_cause; struct hostap_sta_driver_data data; struct os_reltime now_r, diff; - u32 gigawords; if (!hapd->conf->radius->acct_server) return; @@ -290,35 +286,25 @@ static void accounting_sta_report(struct hostapd_data *hapd, } if (!radius_msg_add_attr_int32(msg, RADIUS_ATTR_ACCT_INPUT_OCTETS, - data.rx_bytes)) { + (u32)data.rx_bytes)) { wpa_printf(MSG_INFO, "Could not add Acct-Input-Octets"); goto fail; } - gigawords = sta->acct_input_gigawords; -#if __WORDSIZE == 64 - gigawords += data.rx_bytes >> 32; -#endif - if (gigawords && - !radius_msg_add_attr_int32( - msg, RADIUS_ATTR_ACCT_INPUT_GIGAWORDS, - gigawords)) { + if (!radius_msg_add_attr_int32(msg, + RADIUS_ATTR_ACCT_INPUT_GIGAWORDS, + (u32)(data.rx_bytes >> 32))) { wpa_printf(MSG_INFO, "Could not add Acct-Input-Gigawords"); goto fail; } if (!radius_msg_add_attr_int32(msg, RADIUS_ATTR_ACCT_OUTPUT_OCTETS, - data.tx_bytes)) { + (u32)data.tx_bytes)) { wpa_printf(MSG_INFO, "Could not add Acct-Output-Octets"); goto fail; } - gigawords = sta->acct_output_gigawords; -#if __WORDSIZE == 64 - gigawords += data.tx_bytes >> 32; -#endif - if (gigawords && - !radius_msg_add_attr_int32( - msg, RADIUS_ATTR_ACCT_OUTPUT_GIGAWORDS, - gigawords)) { + if (!radius_msg_add_attr_int32(msg, + RADIUS_ATTR_ACCT_OUTPUT_GIGAWORDS, + (u32)(data.tx_bytes >> 32))) { wpa_printf(MSG_INFO, "Could not add Acct-Output-Gigawords"); goto fail; } diff --git a/src/ap/ctrl_iface_ap.c b/src/ap/ctrl_iface_ap.c index c98978f..4f93ac2 100644 --- a/src/ap/ctrl_iface_ap.c +++ b/src/ap/ctrl_iface_ap.c @@ -35,7 +35,7 @@ static int hostapd_get_sta_tx_rx(struct hostapd_data *hapd, return 0; ret = os_snprintf(buf, buflen, "rx_packets=%lu\ntx_packets=%lu\n" - "rx_bytes=%lu\ntx_bytes=%lu\n", + "rx_bytes=%llu\ntx_bytes=%llu\n", data.rx_packets, data.tx_packets, data.rx_bytes, data.tx_bytes); if (os_snprintf_error(buflen, ret)) diff --git a/src/ap/sta_info.h b/src/ap/sta_info.h index 359f480..114ba65 100644 --- a/src/ap/sta_info.h +++ b/src/ap/sta_info.h @@ -107,10 +107,8 @@ struct sta_info { int acct_terminate_cause; /* Acct-Terminate-Cause */ int acct_interim_interval; /* Acct-Interim-Interval */ - unsigned long last_rx_bytes; - unsigned long last_tx_bytes; - u32 acct_input_gigawords; /* Acct-Input-Gigawords */ - u32 acct_output_gigawords; /* Acct-Output-Gigawords */ + unsigned long long last_rx_bytes; + unsigned long long last_tx_bytes; u8 *challenge; /* IEEE 802.11 Shared Key Authentication Challenge */ diff --git a/src/drivers/driver.h b/src/drivers/driver.h index a4f6704..6734a99 100644 --- a/src/drivers/driver.h +++ b/src/drivers/driver.h @@ -1368,7 +1368,8 @@ struct wpa_driver_capa { struct hostapd_data; struct hostap_sta_driver_data { - unsigned long rx_packets, tx_packets, rx_bytes, tx_bytes; + unsigned long rx_packets, tx_packets; + unsigned long long rx_bytes, tx_bytes; unsigned long current_tx_rate; unsigned long inactive_msec; unsigned long flags; diff --git a/src/drivers/driver_hostap.c b/src/drivers/driver_hostap.c index 517a3bb..6e43cef 100644 --- a/src/drivers/driver_hostap.c +++ b/src/drivers/driver_hostap.c @@ -579,7 +579,6 @@ static int hostap_read_sta_data(void *priv, struct hostap_driver_data *drv = priv; char buf[1024], line[128], *pos; FILE *f; - unsigned long val; memset(data, 0, sizeof(*data)); snprintf(buf, sizeof(buf), "/proc/net/hostap/%s/" MACSTR, @@ -597,15 +596,14 @@ static int hostap_read_sta_data(void *priv, if (!pos) continue; *pos++ = '\0'; - val = strtoul(pos, NULL, 10); if (strcmp(line, "rx_packets") == 0) - data->rx_packets = val; + data->rx_packets = strtoul(pos, NULL, 10); else if (strcmp(line, "tx_packets") == 0) - data->tx_packets = val; + data->tx_packets = strtoul(pos, NULL, 10); else if (strcmp(line, "rx_bytes") == 0) - data->rx_bytes = val; + data->rx_bytes = strtoull(pos, NULL, 20); else if (strcmp(line, "tx_bytes") == 0) - data->tx_bytes = val; + data->tx_bytes = strtoull(pos, NULL, 20); } fclose(f); diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c index 0f312a0..4554006 100644 --- a/src/drivers/driver_nl80211.c +++ b/src/drivers/driver_nl80211.c @@ -5327,7 +5327,13 @@ static int get_sta_handler(struct nl_msg *msg, void *arg) [NL80211_STA_INFO_RX_PACKETS] = { .type = NLA_U32 }, [NL80211_STA_INFO_TX_PACKETS] = { .type = NLA_U32 }, [NL80211_STA_INFO_TX_FAILED] = { .type = NLA_U32 }, +#if defined(NL80211_STA_INFO_RX_BYTES64) && defined(NL80211_STA_INFO_TX_BYTES64) + [NL80211_STA_INFO_RX_BYTES64] = { .type = NLA_U64 }, + [NL80211_STA_INFO_TX_BYTES64] = { .type = NLA_U64 }, +#endif }; + u32 bytes_lo; + u32 bytes_hi; nla_parse(tb, NL80211_ATTR_MAX, genlmsg_attrdata(gnlh, 0), genlmsg_attrlen(gnlh, 0), NULL); @@ -5352,10 +5358,32 @@ static int get_sta_handler(struct nl_msg *msg, void *arg) if (stats[NL80211_STA_INFO_INACTIVE_TIME]) data->inactive_msec = nla_get_u32(stats[NL80211_STA_INFO_INACTIVE_TIME]); - if (stats[NL80211_STA_INFO_RX_BYTES]) - data->rx_bytes = nla_get_u32(stats[NL80211_STA_INFO_RX_BYTES]); - if (stats[NL80211_STA_INFO_TX_BYTES]) - data->tx_bytes = nla_get_u32(stats[NL80211_STA_INFO_TX_BYTES]); +#if defined(NL80211_STA_INFO_RX_BYTES64) && defined(NL80211_STA_INFO_TX_BYTES64) + if (stats[NL80211_STA_INFO_RX_BYTES64]) + data->rx_bytes = nla_get_u64(stats[NL80211_STA_INFO_RX_BYTES64]); + else if (stats[NL80211_STA_INFO_RX_BYTES]) { +#else + if (stats[NL80211_STA_INFO_RX_BYTES]) { +#endif + bytes_lo = nla_get_u32(stats[NL80211_STA_INFO_RX_BYTES]); + bytes_hi = (u32)(data->rx_bytes >> 32); + if (bytes_lo < (u32)data->rx_bytes) + bytes_hi++; + data->rx_bytes = (u64)bytes_hi << 32 | bytes_lo; + } +#if defined(NL80211_STA_INFO_RX_BYTES64) && defined(NL80211_STA_INFO_TX_BYTES64) + if (stats[NL80211_STA_INFO_TX_BYTES64]) + data->tx_bytes = nla_get_u64(stats[NL80211_STA_INFO_TX_BYTES64]); + else if (stats[NL80211_STA_INFO_TX_BYTES]) { +#else + if (stats[NL80211_STA_INFO_TX_BYTES]) { +#endif + bytes_lo = nla_get_u32(stats[NL80211_STA_INFO_TX_BYTES]); + bytes_hi = (u32)(data->tx_bytes >> 32); + if (bytes_lo < (u32)data->tx_bytes) + bytes_hi++; + data->tx_bytes = (u64)bytes_hi << 32 | bytes_lo; + } if (stats[NL80211_STA_INFO_RX_PACKETS]) data->rx_packets = nla_get_u32(stats[NL80211_STA_INFO_RX_PACKETS]); -- 2.7.1
_______________________________________________ Hostap mailing list Hostap@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/hostap