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
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
