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

[Aditya Kumar Singh]

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.


-- 
Aditya