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

[Jouni Malinen]

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?

-- 
Jouni Malinen                                            PGP id EFC895FA