On Thu, Jul 28, 2022 at 07:15:31PM +0530, Veerendranath Jakkam wrote:
> Parse NL80211_ATTR_MLO_LINKS in NL80211_CMD_CONNECT event and cache the
> MLO connection information. Set the legacy connection fields such as
> assoc_freq and bssid to the values the MLO link on which association
> happened.
> diff --git a/src/drivers/driver.h b/src/drivers/driver.h
> @@ -2623,6 +2623,16 @@ struct weighted_pcl {
> +struct driver_sta_mlo_info {
> + u16 valid_links;
It would be good to document this as something like "bitmap of valid
Link IDs".
> diff --git a/src/drivers/driver_nl80211_event.c b/src/drivers/driver_nl80211_event.c
> +static void nl80211_parse_mlo_info(struct wpa_driver_nl80211_data *drv,
> + struct nlattr *addr,
> + struct nlattr *mlo_links,
> + struct nlattr *resp_ie)
> +{
> + struct nlattr *link;
> + int rem_links;
> + const u8 *ml_ie;
> + struct driver_sta_mlo_info *mlo = &drv->sta_mlo_info;
> +
> + if (!addr || !mlo_links || !resp_ie)
> + return;
> +
> + ml_ie = get_ie_ext(nla_data(resp_ie), nla_len(resp_ie),
> + WLAN_EID_EXT_MULTI_LINK);
> +
> +#define ML_IE_SELF_LINK_ID_OFFSET \
> + (3 + /* IE header */ \
> + 2 + /* Control field */ \
> + 1 + /* Common info length field */ \
> + 6) /* MLD mac address */
> + if (!ml_ie || nla_len(resp_ie) <= ML_IE_SELF_LINK_ID_OFFSET)
> + return;
> +
> + drv->mlo_assoc_link_id = ml_ie[ML_IE_SELF_LINK_ID_OFFSET];
There are a number of different types of Multi-Link element. This seems
to be hardcoded to assume this is the Basic Multi-Link element and that
the Link ID Info field is present. While that might be the case in
general when receiving this element in (Re)Association Response frame,
this does not look robust enough, so I'd prefer to see explicit checks
added for the Type subfield of the Multi-Link Control field (i.e.,
ignore this if Type != 0 (Basic)) and on the Link ID Info Present
subfield (needs to 1 for the Link ID Info field to be present).
Furthermore, the Link ID Info field uses only four bits (B0..B3) while
the rest of the octet are reserved, so this assignment of
drv->mlo_assoc_link_id needs to mask out the reserved bits that could be
used for something else in the future.
> + link_id = nla_get_u8(tb[NL80211_ATTR_MLO_LINK_ID]);
> + mlo->valid_links |= BIT(link_id);
> + os_memcpy(mlo->links[link_id].addr,
> + nla_data(tb[NL80211_ATTR_MAC]), ETH_ALEN);
There should be bounds checking on the link_id value, i.e., if it is >=
MAX_NUM_MLD_LINKS, the links[] array use needs to prevented.
> + if (drv->mlo_assoc_link_id >= MAX_NUM_MLD_LINKS ||
> + !(mlo->valid_links & BIT(drv->mlo_assoc_link_id))) {
> + wpa_printf(MSG_ERROR, "Invalid MLO assoc link ID %d",
> + drv->mlo_assoc_link_id);
> + mlo->valid_links = 0;
> + return;
> + }
So there is some bounds checking here, but that is too late and separate
from the parsing of information for each individual link above.
--
Jouni Malinen PGP id EFC895FA
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
