[PATCH v3 2/6] ctrl_iface: MLO: introduce MLD level socket

[Aditya Kumar Singh]

On 8/5/24 22:55, Jouni Malinen wrote:
> On Thu, Aug 01, 2024 at 10:21:39PM +0530, Aditya Kumar Singh wrote:
>> With MLO, each link have socket created with "<ifname>_link<link id>" under
>> the control interface directory.
>>
>> Introduce a MLD level socket - "<ifname>" as well under the same control
>> interface directory. This socket can be used to pass the command to its
>> partner links directly instead of using the link level socket. Link ID
>> needs to be passed with the command.
>>
>> The structure of the command is -
>>   "<COMMAND APPLICABALE FOR THE LINK> LINKID <link id>"
> 
> That feels quite problematic since "LINKID" could occur in a valid
> command, e.g., when setting a string parameter. This "LINKID <link id>"
> part would likely need to be a prefix instead of postfix for the command
> to make this more robust.
> 

Sure will move to prefix. Thanks for the suggestion.


>> diff --git a/hostapd/ctrl_iface.c b/hostapd/ctrl_iface.c
> 
>> +static int hostapd_mld_ctrl_iface_receive_process(struct hostapd_mld *mld,
> 
>> +	/* Check if link id is provided in the command or not */
>> +	link_cmd = os_strstr(buf, "LINKID");
>> +	if (link_cmd) {
>> +		/* Trim the link id part now */
>> +		*(link_cmd - 1) = '\0';
> 
> That needs bounds checking.. The received command could start with
> "LINKID" and that -1 would make this write before the start of the
> buffer. Though, my comment above will likely make this not applicable,
> but anyway, all commands received from the control interface needs to be
> fully verified to be valid to avoid security issues.

Sure got it.

> 
>> +	} else if (os_strcmp(buf, "ATTACH") == 0) {
>> +		if (hostapd_mld_ctrl_iface_attach(mld, from, fromlen, NULL))
>> +			reply_len = -1;
>> +	} else if (os_strncmp(buf, "ATTACH ", 7) == 0) {
>> +		if (hostapd_mld_ctrl_iface_attach(mld, from, fromlen, buf + 7))
>> +			reply_len = -1;
>> +	} else if (os_strcmp(buf, "DETACH") == 0) {
>> +		if (hostapd_mld_ctrl_iface_detach(mld, from, fromlen))
>> +			reply_len = -1;
> 
> Is there a plan to use those hostapd_mld_ctrl_iface_{attach,detach}()
> functions for something else than for wrapping a call to
> ctrl_iface_{attach,detach}()? I would simply call the existing functions
> directly instead of going through that minimal wrapper function.
> 

I don't have any plans as of now. Sure, I will directly call the 
functions instead of the wrapper.


>> +static void hostapd_mld_ctrl_iface_receive(int sock, void *eloop_ctx,
> 
>> +	os_snprintf(buf, len, "%s/%s", mld->ctrl_interface, mld->name);
>> +	buf[len - 1] = '\0';
> 
> Instead of hardcoding \0 termination, it would be better to explicitly
> check the os_snprintf() return value with os_snprintf_error() to catch
> all truncation cases.

Sure will do.

-- 
Aditya