diff mbox series

[v3,1/6] ctrl_iface: create link based hapd control sockets

Message ID 20240801165143.3212598-2-quic_adisi@quicinc.com
State Changes Requested
Headers show
Series MLO control socket changes | expand

Commit Message

Aditya Kumar Singh Aug. 1, 2024, 4:51 p.m. UTC 
From: Karthikeyan Kathirvel <quic_kathirve@quicinc.com>

Create link based control sockets to access the link based commands
through hostapd_cli. This will create the link interfaces in the name of
wlan<X>_link<X>

Example:
To fetch link 0 status from wlan0, below command can be used -
    $ hostapd_cli -i wlan0 -l 0 status

On failure of link/interface selection, below error will be observed
    $ hostapd_cli -i wlan0 -l 2 status
    Failed to connect to hostapd - wpa_ctrl_open: No such file or directory

Signed-off-by: Karthikeyan Kathirvel <quic_kathirve@quicinc.com>
Co-developed-by: Aditya Kumar Singh <quic_adisi@quicinc.com>
Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com>
---
 hostapd/ctrl_iface.c  | 16 ++++++++++++++--
 hostapd/hostapd_cli.c | 30 ++++++++++++++++++++++++++++--
 src/ap/hostapd.c      | 28 ++++++++++++++++++++++++++++
 src/ap/hostapd.h      |  1 +
 src/common/wpa_ctrl.h |  4 ++++
 5 files changed, 75 insertions(+), 4 deletions(-)

Comments

Jouni Malinen Aug. 5, 2024, 5:31 p.m. UTC | #1 
On Thu, Aug 01, 2024 at 10:21:38PM +0530, Aditya Kumar Singh wrote:

> diff --git a/hostapd/hostapd_cli.c b/hostapd/hostapd_cli.c
> @@ -54,7 +54,11 @@ static void usage(void)
> +#ifdef CONFIG_IEEE80211BE
> +		"usage: hostapd_cli [-p<path>] [-i<ifname>] [-l<link_id>] [-hvBr] "
> +#else
>  		"usage: hostapd_cli [-p<path>] [-i<ifname>] [-hvBr] "
> +#endif /* CONFIG_IEEE80211BE */

Please avoid duplicated versions by splitting that into

		"usage: hostapd_cli [-p<path>] [-i<ifname>] "
#ifdef CONFIG_IEEE80211BE
		"[-l<link_id>] "
#endif /* CONFIG_IEEE80211BE */
		"[-hvBr] "


> +#ifdef CONFIG_IEEE80211BE
> +		"   -l<link_id>  Link ID of the interface in case of Multi-Link\n"
> +		"                Operation\n"

That "Operation" fits fine into the end of the previous printed line..

> @@ -2205,19 +2214,26 @@ static void hostapd_cli_action(struct wpa_ctrl *ctrl)
>  	eloop_unregister_read_sock(fd);
>  }
>  
> -
>  int main(int argc, char *argv[])

Please no unrelated whitespace cleanup (especially when it is actually
incorrect for the coding style used in hostap.git).

> +#ifdef CONFIG_IEEE80211BE
> +		c = getopt(argc, argv, "a:BhG:i:l:p:P:rs:v");
> +#else
>  		c = getopt(argc, argv, "a:BhG:i:p:P:rs:v");
> +#endif /* CONFIG_IEEE80211BE */

Please avoid duplicated things. I would go with that CONFIG_IEEE80211BE
case for both since the default case in the switch will handle that fine
as-is.

> @@ -2252,6 +2268,16 @@ int main(int argc, char *argv[])
> +#ifdef CONFIG_IEEE80211BE
> +		case 'l':
> +			link_id = atoi(optarg);
> +			os_memset(buf, '\0', sizeof(buf));
> +			os_snprintf(buf, sizeof(buf), "%s_%s%d",
> +				    ctrl_ifname, WPA_CTRL_IFACE_LINK_NAME, link_id);

No os_memset() is needed before os_snprintf(), but use of
os_snprintf_error() would be recommended.

> diff --git a/src/ap/hostapd.c b/src/ap/hostapd.c
> +static void hostapd_set_ctrl_sock_iface(struct hostapd_data *hapd)
> +{
> +#ifdef CONFIG_IEEE80211BE
> +	os_memset(hapd->ctrl_sock_iface, '\0',
> +		  sizeof(hapd->ctrl_sock_iface));
> +	os_strlcpy(hapd->ctrl_sock_iface, hapd->conf->iface,
> +		   sizeof(hapd->ctrl_sock_iface));

No os_memset() before os_strlcpy()..

> +	if (hapd->conf->mld_ap) {
> +		char buf[128];
> +
> +		os_memset(buf, '\0', sizeof(buf));
> +		os_snprintf(buf, sizeof(buf), "%s_%s%d",
> +			    hapd->conf->iface, WPA_CTRL_IFACE_LINK_NAME,
> +			    hapd->mld_link_id);
> +		os_memset(hapd->ctrl_sock_iface, '\0',
> +			  sizeof(hapd->ctrl_sock_iface));
> +		os_strlcpy(hapd->ctrl_sock_iface, buf, sizeof(buf));

No os_memset() before os_snprintf()/os_strlcpy().

That last sizeof(buf) is very wrong for the os_strlcpy().. It is
supposed to be the size of the target buffer. This could result in
buffer overflow..

Why is that buf[] stack buffer used here? Couldn't this simply
os_snprintf() to hapd->ctrl_sock_iface?

Instead of first writing the non-mld_ap value into hapd->ctrl_sock_iface
and then overwriting it, this would be cleaner by having if-else..

> diff --git a/src/ap/hostapd.h b/src/ap/hostapd.h
> @@ -476,6 +476,7 @@ struct hostapd_data {
>  	struct hostapd_mld *mld;
>  	struct dl_list link;
>  	u8 mld_link_id;
> +	char ctrl_sock_iface[IFNAMSIZ + 1];

Is that large enough to include the "_link" + ID part?
Aditya Kumar Singh Aug. 6, 2024, 4:37 a.m. UTC | #2 
On 8/5/24 23:01, Jouni Malinen wrote:
> On Thu, Aug 01, 2024 at 10:21:38PM +0530, Aditya Kumar Singh wrote:
> 
>> diff --git a/hostapd/hostapd_cli.c b/hostapd/hostapd_cli.c
>> @@ -54,7 +54,11 @@ static void usage(void)
>> +#ifdef CONFIG_IEEE80211BE
>> +		"usage: hostapd_cli [-p<path>] [-i<ifname>] [-l<link_id>] [-hvBr] "
>> +#else
>>   		"usage: hostapd_cli [-p<path>] [-i<ifname>] [-hvBr] "
>> +#endif /* CONFIG_IEEE80211BE */
> 
> Please avoid duplicated versions by splitting that into
> 
> 		"usage: hostapd_cli [-p<path>] [-i<ifname>] "
> #ifdef CONFIG_IEEE80211BE
> 		"[-l<link_id>] "
> #endif /* CONFIG_IEEE80211BE */
> 		"[-hvBr] "
> 

Sure will do.

> 
>> +#ifdef CONFIG_IEEE80211BE
>> +		"   -l<link_id>  Link ID of the interface in case of Multi-Link\n"
>> +		"                Operation\n"
> 
> That "Operation" fits fine into the end of the previous printed line..

Okay.

> 
>> @@ -2205,19 +2214,26 @@ static void hostapd_cli_action(struct wpa_ctrl *ctrl)
>>   	eloop_unregister_read_sock(fd);
>>   }
>>   
>> -
>>   int main(int argc, char *argv[])
> 
> Please no unrelated whitespace cleanup (especially when it is actually
> incorrect for the coding style used in hostap.git).

Got it. This came accidentally. Thanks for pointing it out.

> 
>> +#ifdef CONFIG_IEEE80211BE
>> +		c = getopt(argc, argv, "a:BhG:i:l:p:P:rs:v");
>> +#else
>>   		c = getopt(argc, argv, "a:BhG:i:p:P:rs:v");
>> +#endif /* CONFIG_IEEE80211BE */
> 
> Please avoid duplicated things. I would go with that CONFIG_IEEE80211BE
> case for both since the default case in the switch will handle that fine
> as-is.
> 

Sure, got it.

>> @@ -2252,6 +2268,16 @@ int main(int argc, char *argv[])
>> +#ifdef CONFIG_IEEE80211BE
>> +		case 'l':
>> +			link_id = atoi(optarg);
>> +			os_memset(buf, '\0', sizeof(buf));
>> +			os_snprintf(buf, sizeof(buf), "%s_%s%d",
>> +				    ctrl_ifname, WPA_CTRL_IFACE_LINK_NAME, link_id);
> 
> No os_memset() is needed before os_snprintf(), but use of
> os_snprintf_error() would be recommended.

Okay.

> 
>> diff --git a/src/ap/hostapd.c b/src/ap/hostapd.c
>> +static void hostapd_set_ctrl_sock_iface(struct hostapd_data *hapd)
>> +{
>> +#ifdef CONFIG_IEEE80211BE
>> +	os_memset(hapd->ctrl_sock_iface, '\0',
>> +		  sizeof(hapd->ctrl_sock_iface));
>> +	os_strlcpy(hapd->ctrl_sock_iface, hapd->conf->iface,
>> +		   sizeof(hapd->ctrl_sock_iface));
> 
> No os_memset() before os_strlcpy()..

Got it.

> 
>> +	if (hapd->conf->mld_ap) {
>> +		char buf[128];
>> +
>> +		os_memset(buf, '\0', sizeof(buf));
>> +		os_snprintf(buf, sizeof(buf), "%s_%s%d",
>> +			    hapd->conf->iface, WPA_CTRL_IFACE_LINK_NAME,
>> +			    hapd->mld_link_id);
>> +		os_memset(hapd->ctrl_sock_iface, '\0',
>> +			  sizeof(hapd->ctrl_sock_iface));
>> +		os_strlcpy(hapd->ctrl_sock_iface, buf, sizeof(buf));
> 
> No os_memset() before os_snprintf()/os_strlcpy().
> 
> That last sizeof(buf) is very wrong for the os_strlcpy().. It is
> supposed to be the size of the target buffer. This could result in
> buffer overflow..
> 
> Why is that buf[] stack buffer used here? Couldn't this simply
> os_snprintf() to hapd->ctrl_sock_iface?
> 
> Instead of first writing the non-mld_ap value into hapd->ctrl_sock_iface
> and then overwriting it, this would be cleaner by having if-else..

Sure will move to to if-else. Thanks for the suggestion.

> 
>> diff --git a/src/ap/hostapd.h b/src/ap/hostapd.h
>> @@ -476,6 +476,7 @@ struct hostapd_data {
>>   	struct hostapd_mld *mld;
>>   	struct dl_list link;
>>   	u8 mld_link_id;
>> +	char ctrl_sock_iface[IFNAMSIZ + 1];
> 
> Is that large enough to include the "_link" + ID part?

Oops! Nope, typically tested with interface names not going beyond 6-7 
chars. But ideally this should be be "IFNAMSIZ + 7+ 1". Thanks for 
pointing this out.


Thanks for your review Jouni. I will address these and send next version 
for review.
diff mbox series

Patch

diff --git a/hostapd/ctrl_iface.c b/hostapd/ctrl_iface.c
index 39b9ef59dc57..3fa33be7a894 100644
--- a/hostapd/ctrl_iface.c
+++ b/hostapd/ctrl_iface.c
@@ -4687,18 +4687,26 @@  static char * hostapd_ctrl_iface_path(struct hostapd_data *hapd)
 {
 	char *buf;
 	size_t len;
+	char *ctrl_sock_iface;
+
+#ifdef CONFIG_IEEE80211BE
+	ctrl_sock_iface = hapd->ctrl_sock_iface;
+#else
+	ctrl_sock_iface = hapd->conf->iface;
+#endif /* CONFIG_IEEE80211BE */
 
 	if (hapd->conf->ctrl_interface == NULL)
 		return NULL;
 
 	len = os_strlen(hapd->conf->ctrl_interface) +
-		os_strlen(hapd->conf->iface) + 2;
+		os_strlen(ctrl_sock_iface) + 2;
+
 	buf = os_malloc(len);
 	if (buf == NULL)
 		return NULL;
 
 	os_snprintf(buf, len, "%s/%s",
-		    hapd->conf->ctrl_interface, hapd->conf->iface);
+		    hapd->conf->ctrl_interface, ctrl_sock_iface);
 	buf[len - 1] = '\0';
 	return buf;
 }
@@ -4869,7 +4877,11 @@  fail:
 #endif /* ANDROID */
 
 	if (os_strlen(hapd->conf->ctrl_interface) + 1 +
+#ifdef CONFIG_IEEE80211BE
+	    os_strlen(hapd->ctrl_sock_iface) >= sizeof(addr.sun_path))
+#else
 	    os_strlen(hapd->conf->iface) >= sizeof(addr.sun_path))
+#endif /* CONFIG_IEEE80211BE */
 		goto fail;
 
 	s = socket(PF_UNIX, SOCK_DGRAM, 0);
diff --git a/hostapd/hostapd_cli.c b/hostapd/hostapd_cli.c
index eb8a38350bd1..f05a734fea90 100644
--- a/hostapd/hostapd_cli.c
+++ b/hostapd/hostapd_cli.c
@@ -54,7 +54,11 @@  static void usage(void)
 	fprintf(stderr, "%s\n", hostapd_cli_version);
 	fprintf(stderr,
 		"\n"
+#ifdef CONFIG_IEEE80211BE
+		"usage: hostapd_cli [-p<path>] [-i<ifname>] [-l<link_id>] [-hvBr] "
+#else
 		"usage: hostapd_cli [-p<path>] [-i<ifname>] [-hvBr] "
+#endif /* CONFIG_IEEE80211BE */
 		"[-a<path>] \\\n"
 		"                   [-P<pid file>] [-G<ping interval>] [command..]\n"
 		"\n"
@@ -74,7 +78,12 @@  static void usage(void)
 		"   -B           run a daemon in the background\n"
 		"   -i<ifname>   Interface to listen on (default: first "
 		"interface found in the\n"
-		"                socket path)\n\n");
+		"                socket path)\n"
+#ifdef CONFIG_IEEE80211BE
+		"   -l<link_id>  Link ID of the interface in case of Multi-Link\n"
+		"                Operation\n"
+#endif /* CONFIG_IEEE80211BE */
+		"\n");
 	print_help(stderr, NULL);
 }
 
@@ -2205,19 +2214,26 @@  static void hostapd_cli_action(struct wpa_ctrl *ctrl)
 	eloop_unregister_read_sock(fd);
 }
 
-
 int main(int argc, char *argv[])
 {
 	int warning_displayed = 0;
 	int c;
 	int daemonize = 0;
 	int reconnect = 0;
+#ifdef CONFIG_IEEE80211BE
+	int link_id = -1;
+	char buf[300];
+#endif /* CONFIG_IEEE80211BE */
 
 	if (os_program_init())
 		return -1;
 
 	for (;;) {
+#ifdef CONFIG_IEEE80211BE
+		c = getopt(argc, argv, "a:BhG:i:l:p:P:rs:v");
+#else
 		c = getopt(argc, argv, "a:BhG:i:p:P:rs:v");
+#endif /* CONFIG_IEEE80211BE */
 		if (c < 0)
 			break;
 		switch (c) {
@@ -2252,6 +2268,16 @@  int main(int argc, char *argv[])
 		case 's':
 			client_socket_dir = optarg;
 			break;
+#ifdef CONFIG_IEEE80211BE
+		case 'l':
+			link_id = atoi(optarg);
+			os_memset(buf, '\0', sizeof(buf));
+			os_snprintf(buf, sizeof(buf), "%s_%s%d",
+				    ctrl_ifname, WPA_CTRL_IFACE_LINK_NAME, link_id);
+			os_free(ctrl_ifname);
+			ctrl_ifname = os_strdup(buf);
+			break;
+#endif /* CONFIG_IEEE80211BE */
 		default:
 			usage();
 			return -1;
diff --git a/src/ap/hostapd.c b/src/ap/hostapd.c
index a0ac3a857823..49c9d0ddefd7 100644
--- a/src/ap/hostapd.c
+++ b/src/ap/hostapd.c
@@ -1810,12 +1810,37 @@  int hostapd_set_acl(struct hostapd_data *hapd)
 }
 
 
+static void hostapd_set_ctrl_sock_iface(struct hostapd_data *hapd)
+{
+#ifdef CONFIG_IEEE80211BE
+	os_memset(hapd->ctrl_sock_iface, '\0',
+		  sizeof(hapd->ctrl_sock_iface));
+	os_strlcpy(hapd->ctrl_sock_iface, hapd->conf->iface,
+		   sizeof(hapd->ctrl_sock_iface));
+
+	if (hapd->conf->mld_ap) {
+		char buf[128];
+
+		os_memset(buf, '\0', sizeof(buf));
+		os_snprintf(buf, sizeof(buf), "%s_%s%d",
+			    hapd->conf->iface, WPA_CTRL_IFACE_LINK_NAME,
+			    hapd->mld_link_id);
+		os_memset(hapd->ctrl_sock_iface, '\0',
+			  sizeof(hapd->ctrl_sock_iface));
+		os_strlcpy(hapd->ctrl_sock_iface, buf, sizeof(buf));
+	}
+#endif /* CONFIG_IEEE80211BE */
+}
+
+
 static int start_ctrl_iface_bss(struct hostapd_data *hapd)
 {
 	if (!hapd->iface->interfaces ||
 	    !hapd->iface->interfaces->ctrl_iface_init)
 		return 0;
 
+	hostapd_set_ctrl_sock_iface(hapd);
+
 	if (hapd->iface->interfaces->ctrl_iface_init(hapd)) {
 		wpa_printf(MSG_ERROR,
 			   "Failed to setup control interface for %s",
@@ -1836,6 +1861,9 @@  static int start_ctrl_iface(struct hostapd_iface *iface)
 
 	for (i = 0; i < iface->num_bss; i++) {
 		struct hostapd_data *hapd = iface->bss[i];
+
+		hostapd_set_ctrl_sock_iface(hapd);
+
 		if (iface->interfaces->ctrl_iface_init(hapd)) {
 			wpa_printf(MSG_ERROR,
 				   "Failed to setup control interface for %s",
diff --git a/src/ap/hostapd.h b/src/ap/hostapd.h
index dcf395ca5ee8..34a665562d35 100644
--- a/src/ap/hostapd.h
+++ b/src/ap/hostapd.h
@@ -476,6 +476,7 @@  struct hostapd_data {
 	struct hostapd_mld *mld;
 	struct dl_list link;
 	u8 mld_link_id;
+	char ctrl_sock_iface[IFNAMSIZ + 1];
 #ifdef CONFIG_TESTING_OPTIONS
 	u8 eht_mld_link_removal_count;
 #endif /* CONFIG_TESTING_OPTIONS */
diff --git a/src/common/wpa_ctrl.h b/src/common/wpa_ctrl.h
index f6142501e440..865ac6d91052 100644
--- a/src/common/wpa_ctrl.h
+++ b/src/common/wpa_ctrl.h
@@ -674,4 +674,8 @@  char * wpa_ctrl_get_remote_ifname(struct wpa_ctrl *ctrl);
 }
 #endif
 
+#ifdef CONFIG_IEEE80211BE
+#define WPA_CTRL_IFACE_LINK_NAME	"link"
+#endif /* CONFIG_IEEE80211BE */
+
 #endif /* WPA_CTRL_H */