Skip to contentSkip to conversation

ath10k: add some sanity checks

PATCH Superseded Dan Carpenter 3 participants 5 messages Apr 11, 2016 – Apr 19, 2016

Subsystems (2)
Kernel janitors Qualcomm Atheros ATH10K wireless driver
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 ath10k: add some sanity checks

Smatch complains that since "ev->peer_id" comes from skb->data that means we can't trust it and have to do a bounds check on it to prevent an array overflow.

Fixes 6942726f7f7b ('ath10k: add fast peer_map lookup')
Signed-off-by Dan Carpenter <[email protected]>
drivers/net/wireless/ath/ath10k/txrx.c +6 -0
index 9369411..fd6ba69 100644
@@ -190,6 +190,9 @@ void ath10k_peer_map_event(struct ath10k_htt *htt,
 	struct ath10k *ar = htt->ar;
 	struct ath10k_peer *peer;
 
+	if (ev->peer_id >= ATH10K_MAX_NUM_PEER_IDS)
+		return;
+
 	spin_lock_bh(&ar->data_lock);
 	peer = ath10k_peer_find(ar, ev->vdev_id, ev->addr);
 	if (!peer) {

@@ -218,6 +221,9 @@ void ath10k_peer_unmap_event(struct ath10k_htt *htt,
 	struct ath10k *ar = htt->ar;
 	struct ath10k_peer *peer;
 
+	if (ev->peer_id >= ATH10K_MAX_NUM_PEER_IDS)
+		return;
+
 	spin_lock_bh(&ar->data_lock);
 	peer = ath10k_peer_find_by_id(ar, ev->peer_id);
 	if (!peer) {
#1 lore raw
Quoted 12 lines · Dan Carpenter

Smatch complains that since "ev->peer_id" comes from skb->data that means we can't trust it and have to do a bounds check on it to prevent an array overflow.

Fixes 6942726f7f7b ('ath10k: add fast peer_map lookup')
Signed-off-by Dan Carpenter <[email protected]>
drivers/net/wireless/ath/ath10k/txrx.c +0 -0
index 9369411..fd6ba69 100644

drivers/net/wireless/ath/ath10k/txrx.c void ath10k_peer_map_event(struct ath10k_htt *htt, · line 195

Quoted 6 lines
drivers/net/wireless/ath/ath10k/txrx.c +3 -0
@@ -190,6 +190,9 @@ void ath10k_peer_map_event(struct ath10k_htt *htt,
        struct ath10k *ar = htt->ar;
        struct ath10k_peer *peer;
 
+       if (ev->peer_id >= ATH10K_MAX_NUM_PEER_IDS)
+               return;
+

This will lead to a timeout in the other part of the code so printing a warning here will make it easier to understand the failure, e.g.

ath10k_warn(ar, "received htt peer map event with idx out of bounds:
%hu\n", ev->peer_id);

drivers/net/wireless/ath/ath10k/txrx.c void ath10k_peer_unmap_event(struct ath10k_htt *htt, · line 225

Quoted 9 lines
drivers/net/wireless/ath/ath10k/txrx.c +2 -0
        spin_lock_bh(&ar->data_lock);
        peer = ath10k_peer_find(ar, ev->vdev_id, ev->addr);
        if (!peer) {

@@ -218,6 +221,9 @@ void ath10k_peer_unmap_event(struct ath10k_htt *htt,
        struct ath10k *ar = htt->ar;
        struct ath10k_peer *peer;
 
+       if (ev->peer_id >= ATH10K_MAX_NUM_PEER_IDS)
+               return;

Ditto but s/map/unmap.

Anyway, good catch, thanks!

Michał
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#2 lore raw

PATCH v2 1/1 ath10k: add some sanity checks

Smatch complains that since "ev->peer_id" comes from skb->data that means we can't trust it and have to do a bounds check on it to prevent an array overflow.

Fixes 6942726f7f7b ('ath10k: add fast peer_map lookup')
Signed-off-by Dan Carpenter <[email protected]>

---
v2: Add a warning message. There is a checkpatch.pl warning that the alignment should match the open parenthesis but I ignored it because we're going off the end of the 80 character limit and this way is fine.

drivers/net/wireless/ath/ath10k/txrx.c +14 -0
index 9369411..0885fce 100644
@@ -190,6 +190,13 @@ void ath10k_peer_map_event(struct ath10k_htt *htt,
 	struct ath10k *ar = htt->ar;
 	struct ath10k_peer *peer;
 
+	if (ev->peer_id >= ATH10K_MAX_NUM_PEER_IDS) {
+		ath10k_warn(ar,
+			"received htt peer map event with idx out of bounds: %hu\n",
+			ev->peer_id);
+		return;
+	}
+
 	spin_lock_bh(&ar->data_lock);
 	peer = ath10k_peer_find(ar, ev->vdev_id, ev->addr);
 	if (!peer) {

@@ -218,6 +225,13 @@ void ath10k_peer_unmap_event(struct ath10k_htt *htt,
 	struct ath10k *ar = htt->ar;
 	struct ath10k_peer *peer;
 
+	if (ev->peer_id >= ATH10K_MAX_NUM_PEER_IDS) {
+		ath10k_warn(ar,
+			"received htt peer unmap event with idx out of bounds: %hu\n",
+			ev->peer_id);
+		return;
+	}
+
 	spin_lock_bh(&ar->data_lock);
 	peer = ath10k_peer_find_by_id(ar, ev->peer_id);
 	if (!peer) {
#3 lore raw
Quoted 6 lines · Dan Carpenter

Smatch complains that since "ev->peer_id" comes from skb->data that means we can't trust it and have to do a bounds check on it to prevent an array overflow.

Fixes 6942726f7f7b ('ath10k: add fast peer_map lookup')
Signed-off-by Dan Carpenter <[email protected]>

In ath.git pending branch I modified the title to be a bit more unique:

ath10k: add some sanity checks to peer_map_event() functions

Quoted 4 lines

---
v2: Add a warning message. There is a checkpatch.pl warning that the alignment should match the open parenthesis but I ignored it because we're going off the end of the 80 character limit and this way is fine.

In ath10k we have used 90 char limit so I fixed this in the pending branch. And actually I don't think checkpatch complains about the message format string length of a logging function like ath10k_warn().

Signature · 1 line
Kalle Valo
Applied
#4 lore raw
In reply to Dan Carpenter
Quoted 6 lines · Dan Carpenter

Smatch complains that since "ev->peer_id" comes from skb->data that means we can't trust it and have to do a bounds check on it to prevent an array overflow.

Fixes 6942726f7f7b ('ath10k: add fast peer_map lookup')
Signed-off-by Dan Carpenter <[email protected]>

Applied, thanks.

Signature · 1 line
Kalle Valo