qtnfmac: cleanup alignment in firmware communication protocol

Make sure that all elements in QLINK protocol message are aligned to
4 bytes. For this purpose add necessary amount of padding bytes to
each message. Besides, add padding for non-aligned variable length
fields, e.g. SSID, so that the first byte of the next variable length
element is aligned. to 4 bytes. Finally, introduce TLV parsing helpers
to reduce boilerplate TLV parsing code.
Signed-off-by: Igor Mitsyanko <igor.mitsyanko.os@quantenna.com>
Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

drivers/net/wireless/quantenna/qtnfmac/event.c

@@ -25,7 +25,6 @@ qtnf_event_handle_sta_assoc(struct qtnf_wmac *mac, struct qtnf_vif *vif,
 	size_t payload_len;
 	u16 tlv_type;
 	u16 tlv_value_len;
-	size_t tlv_full_len;
 	const struct qlink_tlv_hdr *tlv;
 	int ret = 0;
 
@@ -58,23 +57,17 @@ qtnf_event_handle_sta_assoc(struct qtnf_wmac *mac, struct qtnf_vif *vif,
 	sinfo->generation = vif->generation;
 
 	payload_len = len - sizeof(*sta_assoc);
-	tlv = (const struct qlink_tlv_hdr *)sta_assoc->ies;
 
-	while (payload_len >= sizeof(*tlv)) {
+	qlink_for_each_tlv(tlv, sta_assoc->ies, payload_len) {
 		tlv_type = le16_to_cpu(tlv->type);
 		tlv_value_len = le16_to_cpu(tlv->len);
-		tlv_full_len = tlv_value_len + sizeof(struct qlink_tlv_hdr);
-
-		if (tlv_full_len > payload_len) {
-			ret = -EINVAL;
-			goto out;
-		}
 
 		if (tlv_type == QTN_TLV_ID_IE_SET) {
 			const struct qlink_tlv_ie_set *ie_set;
 			unsigned int ie_len;
 
-			if (payload_len < sizeof(*ie_set)) {
+			if (tlv_value_len <
+			    (sizeof(*ie_set) - sizeof(ie_set->hdr))) {
 				ret = -EINVAL;
 				goto out;
 			}
@@ -88,12 +81,10 @@ qtnf_event_handle_sta_assoc(struct qtnf_wmac *mac, struct qtnf_vif *vif,
 				sinfo->assoc_req_ies_len = ie_len;
 			}
 		}
-
-		payload_len -= tlv_full_len;
-		tlv = (struct qlink_tlv_hdr *)(tlv->val + tlv_value_len);
 	}
 
-	if (payload_len) {
+	if (!qlink_tlv_parsing_ok(tlv, sta_assoc->ies, payload_len)) {
+		pr_err("Malformed TLV buffer\n");
 		ret = -EINVAL;
 		goto out;
 	}
@@ -153,7 +144,6 @@ qtnf_event_handle_bss_join(struct qtnf_vif *vif,
 	size_t payload_len;
 	u16 tlv_type;
 	u16 tlv_value_len;
-	size_t tlv_full_len;
 	const struct qlink_tlv_hdr *tlv;
 	const u8 *rsp_ies = NULL;
 	size_t rsp_ies_len = 0;
@@ -235,24 +225,17 @@ qtnf_event_handle_bss_join(struct qtnf_vif *vif,
 	}
 
 	payload_len = len - sizeof(*join_info);
-	tlv = (struct qlink_tlv_hdr *)join_info->ies;
 
-	while (payload_len >= sizeof(struct qlink_tlv_hdr)) {
+	qlink_for_each_tlv(tlv, join_info->ies, payload_len) {
 		tlv_type = le16_to_cpu(tlv->type);
 		tlv_value_len = le16_to_cpu(tlv->len);
-		tlv_full_len = tlv_value_len + sizeof(struct qlink_tlv_hdr);
-
-		if (payload_len < tlv_full_len) {
-			pr_warn("invalid %u TLV\n", tlv_type);
-			status = WLAN_STATUS_UNSPECIFIED_FAILURE;
-			goto done;
-		}
 
 		if (tlv_type == QTN_TLV_ID_IE_SET) {
 			const struct qlink_tlv_ie_set *ie_set;
 			unsigned int ie_len;
 
-			if (payload_len < sizeof(*ie_set)) {
+			if (tlv_value_len <
+			    (sizeof(*ie_set) - sizeof(ie_set->hdr))) {
 				pr_warn("invalid IE_SET TLV\n");
 				status = WLAN_STATUS_UNSPECIFIED_FAILURE;
 				goto done;
@@ -275,15 +258,10 @@ qtnf_event_handle_bss_join(struct qtnf_vif *vif,
 				break;
 			}
 		}
-
-		payload_len -= tlv_full_len;
-		tlv = (struct qlink_tlv_hdr *)(tlv->val + tlv_value_len);
 	}
 
-	if (payload_len)
-		pr_warn("VIF%u.%u: unexpected remaining payload: %zu\n",
-			vif->mac->macid, vif->vifid, payload_len);
-
+	if (!qlink_tlv_parsing_ok(tlv, join_info->ies, payload_len))
+		pr_warn("Malformed TLV buffer\n");
 done:
 	cfg80211_connect_result(vif->netdev, join_info->bssid, NULL, 0, rsp_ies,
 				rsp_ies_len, status, GFP_KERNEL);
@@ -368,7 +346,6 @@ qtnf_event_handle_scan_results(struct qtnf_vif *vif,
 	size_t payload_len;
 	u16 tlv_type;
 	u16 tlv_value_len;
-	size_t tlv_full_len;
 	const struct qlink_tlv_hdr *tlv;
 	const u8 *ies = NULL;
 	size_t ies_len = 0;
@@ -387,21 +364,17 @@ qtnf_event_handle_scan_results(struct qtnf_vif *vif,
 	}
 
 	payload_len = len - sizeof(*sr);
-	tlv = (struct qlink_tlv_hdr *)sr->payload;
 
-	while (payload_len >= sizeof(struct qlink_tlv_hdr)) {
+	qlink_for_each_tlv(tlv, sr->payload, payload_len) {
 		tlv_type = le16_to_cpu(tlv->type);
 		tlv_value_len = le16_to_cpu(tlv->len);
-		tlv_full_len = tlv_value_len + sizeof(struct qlink_tlv_hdr);
-
-		if (tlv_full_len > payload_len)
-			return -EINVAL;
 
 		if (tlv_type == QTN_TLV_ID_IE_SET) {
 			const struct qlink_tlv_ie_set *ie_set;
 			unsigned int ie_len;
 
-			if (payload_len < sizeof(*ie_set))
+			if (tlv_value_len <
+			    (sizeof(*ie_set) - sizeof(ie_set->hdr)))
 				return -EINVAL;
 
 			ie_set = (const struct qlink_tlv_ie_set *)tlv;
@@ -424,12 +397,9 @@ qtnf_event_handle_scan_results(struct qtnf_vif *vif,
 				ies_len = ie_len;
 			}
 		}
-
-		payload_len -= tlv_full_len;
-		tlv = (struct qlink_tlv_hdr *)(tlv->val + tlv_value_len);
 	}
 
-	if (payload_len)
+	if (!qlink_tlv_parsing_ok(tlv, sr->payload, payload_len))
 		return -EINVAL;
 
 	bss = cfg80211_inform_bss(wiphy, channel, frame_type,

drivers/net/wireless/quantenna/qtnfmac/qlink.h

@@ -19,6 +19,8 @@
 #define QLINK_PROTO_VER		\
 	QLINK_VER(QLINK_PROTO_VER_MAJOR, QLINK_PROTO_VER_MINOR)
 
+#define QLINK_ALIGN	4
+
 #define QLINK_MACID_RSVD		0xFF
 #define QLINK_VIFID_RSVD		0xFF
 
@@ -184,7 +186,7 @@ struct qlink_chandef {
 	__le16 center_freq1;
 	__le16 center_freq2;
 	u8 width;
-	u8 rsvd;
+	u8 rsvd[3];
 } __packed;
 
 #define QLINK_MAX_NR_CIPHER_SUITES            5
@@ -340,9 +342,9 @@ struct qlink_cmd {
 	struct qlink_msg_header mhdr;
 	__le16 cmd_id;
 	__le16 seq_num;
-	u8 rsvd[2];
 	u8 macid;
 	u8 vifid;
+	u8 rsvd[2];
 } __packed;
 
 /**
@@ -404,6 +406,7 @@ struct qlink_cmd_mgmt_frame_register {
 	struct qlink_cmd chdr;
 	__le16 frame_type;
 	u8 do_register;
+	u8 rsvd[1];
 } __packed;
 
 /**
@@ -441,6 +444,7 @@ struct qlink_cmd_frame_tx {
 struct qlink_cmd_get_sta_info {
 	struct qlink_cmd chdr;
 	u8 sta_addr[ETH_ALEN];
+	u8 rsvd[2];
 } __packed;
 
 /**
@@ -460,6 +464,7 @@ struct qlink_cmd_add_key {
 	u8 addr[ETH_ALEN];
 	__le32 cipher;
 	__le16 vlanid;
+	u8 rsvd[2];
 	u8 key_data[0];
 } __packed;
 
@@ -489,6 +494,7 @@ struct qlink_cmd_set_def_key {
 	u8 key_index;
 	u8 unicast;
 	u8 multicast;
+	u8 rsvd[1];
 } __packed;
 
 /**
@@ -499,6 +505,7 @@ struct qlink_cmd_set_def_key {
 struct qlink_cmd_set_def_mgmt_key {
 	struct qlink_cmd chdr;
 	u8 key_index;
+	u8 rsvd[3];
 } __packed;
 
 /**
@@ -515,6 +522,7 @@ struct qlink_cmd_change_sta {
 	__le16 if_type;
 	__le16 vlanid;
 	u8 sta_addr[ETH_ALEN];
+	u8 rsvd[2];
 } __packed;
 
 /**
@@ -525,8 +533,9 @@ struct qlink_cmd_change_sta {
 struct qlink_cmd_del_sta {
 	struct qlink_cmd chdr;
 	__le16 reason_code;
-	u8 subtype;
 	u8 sta_addr[ETH_ALEN];
+	u8 subtype;
+	u8 rsvd[3];
 } __packed;
 
 enum qlink_sta_connect_flags {
@@ -593,6 +602,7 @@ struct qlink_cmd_external_auth {
 struct qlink_cmd_disconnect {
 	struct qlink_cmd chdr;
 	__le16 reason;
+	u8 rsvd[2];
 } __packed;
 
 /**
@@ -604,6 +614,7 @@ struct qlink_cmd_disconnect {
 struct qlink_cmd_updown {
 	struct qlink_cmd chdr;
 	u8 if_up;
+	u8 rsvd[3];
 } __packed;
 
 /**
@@ -627,6 +638,7 @@ enum qlink_band {
 struct qlink_cmd_band_info_get {
 	struct qlink_cmd chdr;
 	u8 band;
+	u8 rsvd[3];
 } __packed;
 
 /**
@@ -702,6 +714,7 @@ struct qlink_cmd_chan_switch {
 	u8 radar_required;
 	u8 block_tx;
 	u8 beacon_count;
+	u8 rsvd[3];
 } __packed;
 
 /**
@@ -805,6 +818,7 @@ struct qlink_cmd_pm_set {
 	struct qlink_cmd chdr;
 	__le32 pm_standby_timer;
 	u8 pm_mode;
+	u8 rsvd[3];
 } __packed;
 
 /**
@@ -1225,6 +1239,7 @@ struct qlink_event_bss_join {
 struct qlink_event_bss_leave {
 	struct qlink_event ehdr;
 	__le16 reason;
+	u8 rsvd[2];
 } __packed;
 
 /**
@@ -1341,10 +1356,10 @@ struct qlink_event_radar {
  */
 struct qlink_event_external_auth {
 	struct qlink_event ehdr;
+	__le32 akm_suite;
 	u8 ssid[IEEE80211_MAX_SSID_LEN];
-	u8 ssid_len;
 	u8 bssid[ETH_ALEN];
-	__le32 akm_suite;
+	u8 ssid_len;
 	u8 action;
 } __packed;
 
@@ -1560,6 +1575,7 @@ struct qlink_tlv_ie_set {
 	struct qlink_tlv_hdr hdr;
 	u8 type;
 	u8 flags;
+	u8 rsvd[2];
 	u8 ie_data[0];
 } __packed;
 
@@ -1572,6 +1588,7 @@ struct qlink_tlv_ie_set {
 struct qlink_tlv_ext_ie {
 	struct qlink_tlv_hdr hdr;
 	u8 eid_ext;
+	u8 rsvd[3];
 	u8 ie_data[0];
 } __packed;
 

drivers/net/wireless/quantenna/qtnfmac/qlink_util.h

@@ -20,8 +20,9 @@ static inline void qtnf_cmd_skb_put_tlv_arr(struct sk_buff *skb,
 					    u16 tlv_id, const u8 arr[],
 					    size_t arr_len)
 {
-	struct qlink_tlv_hdr *hdr = skb_put(skb, sizeof(*hdr) + arr_len);
+	struct qlink_tlv_hdr *hdr;
 
+	hdr = skb_put(skb, sizeof(*hdr) + round_up(arr_len, QLINK_ALIGN));
 	hdr->type = cpu_to_le16(tlv_id);
 	hdr->len = cpu_to_le16(arr_len);
 	memcpy(hdr->val, arr, arr_len);
@@ -35,27 +36,6 @@ static inline void qtnf_cmd_skb_put_tlv_tag(struct sk_buff *skb, u16 tlv_id)
 	hdr->len = cpu_to_le16(0);
 }
 
-static inline void qtnf_cmd_skb_put_tlv_u8(struct sk_buff *skb, u16 tlv_id,
-					   u8 value)
-{
-	struct qlink_tlv_hdr *hdr = skb_put(skb, sizeof(*hdr) + sizeof(value));
-
-	hdr->type = cpu_to_le16(tlv_id);
-	hdr->len = cpu_to_le16(sizeof(value));
-	*hdr->val = value;
-}
-
-static inline void qtnf_cmd_skb_put_tlv_u16(struct sk_buff *skb,
-					    u16 tlv_id, u16 value)
-{
-	struct qlink_tlv_hdr *hdr = skb_put(skb, sizeof(*hdr) + sizeof(value));
-	__le16 tmp = cpu_to_le16(value);
-
-	hdr->type = cpu_to_le16(tlv_id);
-	hdr->len = cpu_to_le16(sizeof(value));
-	memcpy(hdr->val, &tmp, sizeof(tmp));
-}
-
 static inline void qtnf_cmd_skb_put_tlv_u32(struct sk_buff *skb,
 					    u16 tlv_id, u32 value)
 {
@@ -85,4 +65,17 @@ u32 qlink_utils_chflags_cfg2q(u32 cfgflags);
 void qlink_utils_regrule_q2nl(struct ieee80211_reg_rule *rule,
 			      const struct qlink_tlv_reg_rule *tlv_rule);
 
+#define qlink_for_each_tlv(_tlv, _start, _datalen)			\
+	for (_tlv = (const struct qlink_tlv_hdr *)(_start);		\
+	     (const u8 *)(_start) + (_datalen) - (const u8 *)_tlv >=	\
+		(int)sizeof(*_tlv) &&					\
+	     (const u8 *)(_start) + (_datalen) - (const u8 *)_tlv >=	\
+		(int)sizeof(*_tlv) + le16_to_cpu(_tlv->len);		\
+	     _tlv = (const struct qlink_tlv_hdr *)(_tlv->val +		\
+		round_up(le16_to_cpu(_tlv->len), QLINK_ALIGN)))
+
+#define qlink_tlv_parsing_ok(_tlv_last, _start, _datalen)	\
+	((const u8 *)(_tlv_last) == \
+		(const u8 *)(_start) + round_up(_datalen, QLINK_ALIGN))
+
 #endif /* _QTN_FMAC_QLINK_UTIL_H_ */