[PATCH 1/5] ap: check that the last band_freq_attrs was set

[PATCH 1/5] ap: check that the last band_freq_attrs was set

[James Prestwood]

Static analysis complains that 'last' could be NULL which is true.
This really could only happen if every frequency was disabled which
likely is impossible but in any case, check before dereferencing
the pointer.
---
 src/ap.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index bce389d3..ee3c4dca 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -1247,8 +1247,10 @@ static size_t ap_build_country_ie(struct ap_state *ap, uint8_t *out_buf,
 	}
 
 	/* finish final group */
-	*pos++ = nchans;
-	*pos++ = last->tx_power;
+	if (last) {
+		*pos++ = nchans;
+		*pos++ = last->tx_power;
+	}
 
 	len = pos - out_buf - 2;
 
-- 
2.34.1

[PATCH 2/5] ap: allow va_end to get called in ap_handshake_event

[James Prestwood]

Caught by static analysis, va_end was never being called since the
REKEY_COMPLETE event was returning early.
---
 src/ap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/ap.c b/src/ap.c
index ee3c4dca..25d1b8a3 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -1484,7 +1484,7 @@ static void ap_handshake_event(struct handshake_state *hs,
 	}
 	case HANDSHAKE_EVENT_REKEY_COMPLETE:
 		ap_set_sta_rekey_timer(ap, sta);
-		return;
+		break;
 	default:
 		break;
 	}
-- 
2.34.1

[PATCH 3/5] nl80211util: check l_genl_attr_recurse return in extract_nested

[James Prestwood]

Caught by static analysis, the recurse operation return was not being
checked.
---
 src/nl80211util.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/nl80211util.c b/src/nl80211util.c
index 0f45c905..3f9a43ac 100644
--- a/src/nl80211util.c
+++ b/src/nl80211util.c
@@ -136,9 +136,7 @@ static bool extract_nested(const void *data, uint16_t len, void *o)
 	const struct l_genl_attr *outer = data;
 	struct l_genl_attr *nested = o;
 
-	l_genl_attr_recurse(outer, nested);
-
-	return true;
+	return l_genl_attr_recurse(outer, nested);
 }
 
 static bool extract_u8(const void *data, uint16_t len, void *o)
-- 
2.34.1

[PATCH 4/5] ap: verify ATTR_MAC exists in NEW_STATION

[James Prestwood]

Caught by static analysis, if ATTR_MAC was not in the message there
would be a memcpy with uninitialized bytes. In addition there is no
reason to memcpy twice. Instead 'mac' can be a const pointer which
both verifies it exists and removes the need for a second memcpy.
---
 src/ap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index 25d1b8a3..a6f8f306 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -2963,7 +2963,7 @@ static void ap_handle_new_station(struct ap_state *ap, struct l_genl_msg *msg)
 	uint16_t type;
 	uint16_t len;
 	const void *data;
-	uint8_t mac[6];
+	const uint8_t *mac = NULL;
 	uint8_t *assoc_rsne = NULL;
 
 	if (!l_genl_attr_init(&attr, msg))
@@ -2983,12 +2983,12 @@ static void ap_handle_new_station(struct ap_state *ap, struct l_genl_msg *msg)
 			if (len != 6)
 				goto cleanup;
 
-			memcpy(mac, data, 6);
+			mac = data;
 			break;
 		}
 	}
 
-	if (!assoc_rsne)
+	if (!assoc_rsne || !mac)
 		goto cleanup;
 
 	/*
-- 
2.34.1

[PATCH 5/5] ap: bail in ap_del_station if AP is going down

[James Prestwood]

Caught by static analysis, if this condition is met the AP is going
down so we cannot continue further accessing the ap object.
---
 src/ap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/ap.c b/src/ap.c
index a6f8f306..b4e7593e 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -455,7 +455,8 @@ static void ap_del_station(struct sta_state *sta, uint16_t reason,
 		sta->ip_alloc_lease = NULL;
 		l_dhcp_server_expire_by_mac(ap->netconfig_dhcp, sta->addr);
 
-		ap_event_done(ap, prev);
+		if (ap_event_done(ap, prev))
+			return;
 	}
 
 	/*
-- 
2.34.1

Re: [PATCH 1/5] ap: check that the last band_freq_attrs was set

[Denis Kenzior]

Hi James,

On 2/29/24 12:12, James Prestwood wrote:
> Static analysis complains that 'last' could be NULL which is true.
> This really could only happen if every frequency was disabled which
> likely is impossible but in any case, check before dereferencing
> the pointer.
> ---
>   src/ap.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 

All applied, thanks.

Regards,
-Denis