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

[Jouni Malinen]

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.

> 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.

> +	} 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.

> +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.

-- 
Jouni Malinen                                            PGP id EFC895FA