* [PATCH] mt76: mt7915: potential array overflow in mt7915_mcu_tx_rate_report()
@ 2020-07-09 10:47 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2020-07-09 10:47 UTC (permalink / raw)
To: Felix Fietkau, Ryder Lee
Cc: Lorenzo Bianconi, Kalle Valo, Jakub Kicinski, Matthias Brugger,
Yiwei Chung, YF Luo, Shayne Chen, Chih-Min Chen, linux-wireless,
linux-mediatek, kernel-janitors
Smatch complains that this comes from the "wcidx" value comes from the
network and thus cannot be trusted. In this case, it actually seems to
come from the firmware. If your wireless firmware is malicious then
probably no amount of carefulness can protect you.
On the other hand, these days we still try to check the firmware as much
as possible. Verifying that the index is within bounds will silence a
static checker warning. And it's harmless and a good exercise in kernel
hardening. So I suggest that we do add a bounds check.
Fixes: e57b7901469f ("mt76: add mac80211 driver for MT7915 PCIe-based chipsets")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Normally for networking patches, when we change the declaration block,
we must update the order to make sure it's in reverse Christmas tree
format. This code wasn't strictly in reverse Christmas tree order
originally because we needed to initialize "wcidx" before we could
initialize "wcid" etc. Re-ordering the initializers makes the diff
slightly larger than people might expect but it's a required part of
networking patches.
.../net/wireless/mediatek/mt76/mt7915/mcu.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c b/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
index c8c12c740c1a..8fb8255650a7 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
@@ -505,15 +505,22 @@ static void
mt7915_mcu_tx_rate_report(struct mt7915_dev *dev, struct sk_buff *skb)
{
struct mt7915_mcu_ra_info *ra = (struct mt7915_mcu_ra_info *)skb->data;
- u16 wcidx = le16_to_cpu(ra->wlan_idx);
- struct mt76_wcid *wcid = rcu_dereference(dev->mt76.wcid[wcidx]);
- struct mt7915_sta *msta = container_of(wcid, struct mt7915_sta, wcid);
- struct mt7915_sta_stats *stats = &msta->stats;
- struct mt76_phy *mphy = &dev->mphy;
struct rate_info rate = {}, prob_rate = {};
+ u16 probe = le16_to_cpu(ra->prob_up_rate);
u16 attempts = le16_to_cpu(ra->attempts);
u16 curr = le16_to_cpu(ra->curr_rate);
- u16 probe = le16_to_cpu(ra->prob_up_rate);
+ u16 wcidx = le16_to_cpu(ra->wlan_idx);
+ struct mt76_phy *mphy = &dev->mphy;
+ struct mt7915_sta_stats *stats;
+ struct mt7915_sta *msta;
+ struct mt76_wcid *wcid;
+
+ if (wcidx >= MT76_N_WCIDS)
+ return;
+
+ wcid = rcu_dereference(dev->mt76.wcid[wcidx]);
+ msta = container_of(wcid, struct mt7915_sta, wcid);
+ stats = &msta->stats;
if (msta->wcid.ext_phy && dev->mt76.phy2)
mphy = dev->mt76.phy2;
--
2.27.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] mt76: mt7915: potential array overflow in mt7915_mcu_tx_rate_report()
@ 2020-07-09 10:47 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2020-07-09 10:47 UTC (permalink / raw)
To: Felix Fietkau, Ryder Lee
Cc: kernel-janitors, linux-wireless, YF Luo, Chih-Min Chen,
Matthias Brugger, Yiwei Chung, linux-mediatek, Lorenzo Bianconi,
Jakub Kicinski, Kalle Valo, Shayne Chen
Smatch complains that this comes from the "wcidx" value comes from the
network and thus cannot be trusted. In this case, it actually seems to
come from the firmware. If your wireless firmware is malicious then
probably no amount of carefulness can protect you.
On the other hand, these days we still try to check the firmware as much
as possible. Verifying that the index is within bounds will silence a
static checker warning. And it's harmless and a good exercise in kernel
hardening. So I suggest that we do add a bounds check.
Fixes: e57b7901469f ("mt76: add mac80211 driver for MT7915 PCIe-based chipsets")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Normally for networking patches, when we change the declaration block,
we must update the order to make sure it's in reverse Christmas tree
format. This code wasn't strictly in reverse Christmas tree order
originally because we needed to initialize "wcidx" before we could
initialize "wcid" etc. Re-ordering the initializers makes the diff
slightly larger than people might expect but it's a required part of
networking patches.
.../net/wireless/mediatek/mt76/mt7915/mcu.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c b/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
index c8c12c740c1a..8fb8255650a7 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
@@ -505,15 +505,22 @@ static void
mt7915_mcu_tx_rate_report(struct mt7915_dev *dev, struct sk_buff *skb)
{
struct mt7915_mcu_ra_info *ra = (struct mt7915_mcu_ra_info *)skb->data;
- u16 wcidx = le16_to_cpu(ra->wlan_idx);
- struct mt76_wcid *wcid = rcu_dereference(dev->mt76.wcid[wcidx]);
- struct mt7915_sta *msta = container_of(wcid, struct mt7915_sta, wcid);
- struct mt7915_sta_stats *stats = &msta->stats;
- struct mt76_phy *mphy = &dev->mphy;
struct rate_info rate = {}, prob_rate = {};
+ u16 probe = le16_to_cpu(ra->prob_up_rate);
u16 attempts = le16_to_cpu(ra->attempts);
u16 curr = le16_to_cpu(ra->curr_rate);
- u16 probe = le16_to_cpu(ra->prob_up_rate);
+ u16 wcidx = le16_to_cpu(ra->wlan_idx);
+ struct mt76_phy *mphy = &dev->mphy;
+ struct mt7915_sta_stats *stats;
+ struct mt7915_sta *msta;
+ struct mt76_wcid *wcid;
+
+ if (wcidx >= MT76_N_WCIDS)
+ return;
+
+ wcid = rcu_dereference(dev->mt76.wcid[wcidx]);
+ msta = container_of(wcid, struct mt7915_sta, wcid);
+ stats = &msta->stats;
if (msta->wcid.ext_phy && dev->mt76.phy2)
mphy = dev->mt76.phy2;
--
2.27.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] mt76: mt7915: potential array overflow in mt7915_mcu_tx_rate_report()
@ 2020-07-09 10:47 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2020-07-09 10:47 UTC (permalink / raw)
To: Felix Fietkau, Ryder Lee
Cc: kernel-janitors, linux-wireless, YF Luo, Chih-Min Chen,
Matthias Brugger, Yiwei Chung, linux-mediatek, Lorenzo Bianconi,
Jakub Kicinski, Kalle Valo, Shayne Chen
Smatch complains that this comes from the "wcidx" value comes from the
network and thus cannot be trusted. In this case, it actually seems to
come from the firmware. If your wireless firmware is malicious then
probably no amount of carefulness can protect you.
On the other hand, these days we still try to check the firmware as much
as possible. Verifying that the index is within bounds will silence a
static checker warning. And it's harmless and a good exercise in kernel
hardening. So I suggest that we do add a bounds check.
Fixes: e57b7901469f ("mt76: add mac80211 driver for MT7915 PCIe-based chipsets")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Normally for networking patches, when we change the declaration block,
we must update the order to make sure it's in reverse Christmas tree
format. This code wasn't strictly in reverse Christmas tree order
originally because we needed to initialize "wcidx" before we could
initialize "wcid" etc. Re-ordering the initializers makes the diff
slightly larger than people might expect but it's a required part of
networking patches.
.../net/wireless/mediatek/mt76/mt7915/mcu.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c b/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
index c8c12c740c1a..8fb8255650a7 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
@@ -505,15 +505,22 @@ static void
mt7915_mcu_tx_rate_report(struct mt7915_dev *dev, struct sk_buff *skb)
{
struct mt7915_mcu_ra_info *ra = (struct mt7915_mcu_ra_info *)skb->data;
- u16 wcidx = le16_to_cpu(ra->wlan_idx);
- struct mt76_wcid *wcid = rcu_dereference(dev->mt76.wcid[wcidx]);
- struct mt7915_sta *msta = container_of(wcid, struct mt7915_sta, wcid);
- struct mt7915_sta_stats *stats = &msta->stats;
- struct mt76_phy *mphy = &dev->mphy;
struct rate_info rate = {}, prob_rate = {};
+ u16 probe = le16_to_cpu(ra->prob_up_rate);
u16 attempts = le16_to_cpu(ra->attempts);
u16 curr = le16_to_cpu(ra->curr_rate);
- u16 probe = le16_to_cpu(ra->prob_up_rate);
+ u16 wcidx = le16_to_cpu(ra->wlan_idx);
+ struct mt76_phy *mphy = &dev->mphy;
+ struct mt7915_sta_stats *stats;
+ struct mt7915_sta *msta;
+ struct mt76_wcid *wcid;
+
+ if (wcidx >= MT76_N_WCIDS)
+ return;
+
+ wcid = rcu_dereference(dev->mt76.wcid[wcidx]);
+ msta = container_of(wcid, struct mt7915_sta, wcid);
+ stats = &msta->stats;
if (msta->wcid.ext_phy && dev->mt76.phy2)
mphy = dev->mt76.phy2;
--
2.27.0
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2] mt76: mt7915: potential array overflow in mt7915_mcu_tx_rate_report()
2020-07-09 10:47 ` Dan Carpenter
(?)
@ 2020-07-09 11:04 ` Dan Carpenter
-1 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2020-07-09 11:04 UTC (permalink / raw)
To: Felix Fietkau, Ryder Lee
Cc: Lorenzo Bianconi, Kalle Valo, Jakub Kicinski, Matthias Brugger,
Yiwei Chung, YF Luo, Shayne Chen, Chih-Min Chen, linux-wireless,
linux-mediatek, kernel-janitors
Smatch complains that "wcidx" value comes from the network and thus
cannot be trusted. In this case, it actually seems to come from the
firmware. If your wireless firmware is malicious then probably no
amount of carefulness can protect you.
On the other hand, these days we still try to check the firmware as much
as possible. Verifying that the index is within bounds will silence a
static checker warning. And it's harmless and a good exercise in kernel
hardening. So I suggest that we do add a bounds check.
Fixes: e57b7901469f ("mt76: add mac80211 driver for MT7915 PCIe-based chipsets")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Fix a typos in commit message.
Normally for networking patches, when we change the declaration block,
we must update the order to make sure it's in reverse Christmas tree
format. This code wasn't strictly in reverse Christmas tree order
originally because we needed to initialize "wcidx" before we could
initialize "wcid" etc. Re-ordering the initializers makes the diff
slightly larger than people might expect but it's a required part of
networking patches.
.../net/wireless/mediatek/mt76/mt7915/mcu.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c b/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
index c8c12c740c1a..8fb8255650a7 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
@@ -505,15 +505,22 @@ static void
mt7915_mcu_tx_rate_report(struct mt7915_dev *dev, struct sk_buff *skb)
{
struct mt7915_mcu_ra_info *ra = (struct mt7915_mcu_ra_info *)skb->data;
- u16 wcidx = le16_to_cpu(ra->wlan_idx);
- struct mt76_wcid *wcid = rcu_dereference(dev->mt76.wcid[wcidx]);
- struct mt7915_sta *msta = container_of(wcid, struct mt7915_sta, wcid);
- struct mt7915_sta_stats *stats = &msta->stats;
- struct mt76_phy *mphy = &dev->mphy;
struct rate_info rate = {}, prob_rate = {};
+ u16 probe = le16_to_cpu(ra->prob_up_rate);
u16 attempts = le16_to_cpu(ra->attempts);
u16 curr = le16_to_cpu(ra->curr_rate);
- u16 probe = le16_to_cpu(ra->prob_up_rate);
+ u16 wcidx = le16_to_cpu(ra->wlan_idx);
+ struct mt76_phy *mphy = &dev->mphy;
+ struct mt7915_sta_stats *stats;
+ struct mt7915_sta *msta;
+ struct mt76_wcid *wcid;
+
+ if (wcidx >= MT76_N_WCIDS)
+ return;
+
+ wcid = rcu_dereference(dev->mt76.wcid[wcidx]);
+ msta = container_of(wcid, struct mt7915_sta, wcid);
+ stats = &msta->stats;
if (msta->wcid.ext_phy && dev->mt76.phy2)
mphy = dev->mt76.phy2;
--
2.27.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2] mt76: mt7915: potential array overflow in mt7915_mcu_tx_rate_report()
@ 2020-07-09 11:04 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2020-07-09 11:04 UTC (permalink / raw)
To: Felix Fietkau, Ryder Lee
Cc: kernel-janitors, linux-wireless, YF Luo, Chih-Min Chen,
Matthias Brugger, Yiwei Chung, linux-mediatek, Lorenzo Bianconi,
Jakub Kicinski, Kalle Valo, Shayne Chen
Smatch complains that "wcidx" value comes from the network and thus
cannot be trusted. In this case, it actually seems to come from the
firmware. If your wireless firmware is malicious then probably no
amount of carefulness can protect you.
On the other hand, these days we still try to check the firmware as much
as possible. Verifying that the index is within bounds will silence a
static checker warning. And it's harmless and a good exercise in kernel
hardening. So I suggest that we do add a bounds check.
Fixes: e57b7901469f ("mt76: add mac80211 driver for MT7915 PCIe-based chipsets")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Fix a typos in commit message.
Normally for networking patches, when we change the declaration block,
we must update the order to make sure it's in reverse Christmas tree
format. This code wasn't strictly in reverse Christmas tree order
originally because we needed to initialize "wcidx" before we could
initialize "wcid" etc. Re-ordering the initializers makes the diff
slightly larger than people might expect but it's a required part of
networking patches.
.../net/wireless/mediatek/mt76/mt7915/mcu.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c b/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
index c8c12c740c1a..8fb8255650a7 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
@@ -505,15 +505,22 @@ static void
mt7915_mcu_tx_rate_report(struct mt7915_dev *dev, struct sk_buff *skb)
{
struct mt7915_mcu_ra_info *ra = (struct mt7915_mcu_ra_info *)skb->data;
- u16 wcidx = le16_to_cpu(ra->wlan_idx);
- struct mt76_wcid *wcid = rcu_dereference(dev->mt76.wcid[wcidx]);
- struct mt7915_sta *msta = container_of(wcid, struct mt7915_sta, wcid);
- struct mt7915_sta_stats *stats = &msta->stats;
- struct mt76_phy *mphy = &dev->mphy;
struct rate_info rate = {}, prob_rate = {};
+ u16 probe = le16_to_cpu(ra->prob_up_rate);
u16 attempts = le16_to_cpu(ra->attempts);
u16 curr = le16_to_cpu(ra->curr_rate);
- u16 probe = le16_to_cpu(ra->prob_up_rate);
+ u16 wcidx = le16_to_cpu(ra->wlan_idx);
+ struct mt76_phy *mphy = &dev->mphy;
+ struct mt7915_sta_stats *stats;
+ struct mt7915_sta *msta;
+ struct mt76_wcid *wcid;
+
+ if (wcidx >= MT76_N_WCIDS)
+ return;
+
+ wcid = rcu_dereference(dev->mt76.wcid[wcidx]);
+ msta = container_of(wcid, struct mt7915_sta, wcid);
+ stats = &msta->stats;
if (msta->wcid.ext_phy && dev->mt76.phy2)
mphy = dev->mt76.phy2;
--
2.27.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2] mt76: mt7915: potential array overflow in mt7915_mcu_tx_rate_report()
@ 2020-07-09 11:04 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2020-07-09 11:04 UTC (permalink / raw)
To: Felix Fietkau, Ryder Lee
Cc: kernel-janitors, linux-wireless, YF Luo, Chih-Min Chen,
Matthias Brugger, Yiwei Chung, linux-mediatek, Lorenzo Bianconi,
Jakub Kicinski, Kalle Valo, Shayne Chen
Smatch complains that "wcidx" value comes from the network and thus
cannot be trusted. In this case, it actually seems to come from the
firmware. If your wireless firmware is malicious then probably no
amount of carefulness can protect you.
On the other hand, these days we still try to check the firmware as much
as possible. Verifying that the index is within bounds will silence a
static checker warning. And it's harmless and a good exercise in kernel
hardening. So I suggest that we do add a bounds check.
Fixes: e57b7901469f ("mt76: add mac80211 driver for MT7915 PCIe-based chipsets")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Fix a typos in commit message.
Normally for networking patches, when we change the declaration block,
we must update the order to make sure it's in reverse Christmas tree
format. This code wasn't strictly in reverse Christmas tree order
originally because we needed to initialize "wcidx" before we could
initialize "wcid" etc. Re-ordering the initializers makes the diff
slightly larger than people might expect but it's a required part of
networking patches.
.../net/wireless/mediatek/mt76/mt7915/mcu.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c b/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
index c8c12c740c1a..8fb8255650a7 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
@@ -505,15 +505,22 @@ static void
mt7915_mcu_tx_rate_report(struct mt7915_dev *dev, struct sk_buff *skb)
{
struct mt7915_mcu_ra_info *ra = (struct mt7915_mcu_ra_info *)skb->data;
- u16 wcidx = le16_to_cpu(ra->wlan_idx);
- struct mt76_wcid *wcid = rcu_dereference(dev->mt76.wcid[wcidx]);
- struct mt7915_sta *msta = container_of(wcid, struct mt7915_sta, wcid);
- struct mt7915_sta_stats *stats = &msta->stats;
- struct mt76_phy *mphy = &dev->mphy;
struct rate_info rate = {}, prob_rate = {};
+ u16 probe = le16_to_cpu(ra->prob_up_rate);
u16 attempts = le16_to_cpu(ra->attempts);
u16 curr = le16_to_cpu(ra->curr_rate);
- u16 probe = le16_to_cpu(ra->prob_up_rate);
+ u16 wcidx = le16_to_cpu(ra->wlan_idx);
+ struct mt76_phy *mphy = &dev->mphy;
+ struct mt7915_sta_stats *stats;
+ struct mt7915_sta *msta;
+ struct mt76_wcid *wcid;
+
+ if (wcidx >= MT76_N_WCIDS)
+ return;
+
+ wcid = rcu_dereference(dev->mt76.wcid[wcidx]);
+ msta = container_of(wcid, struct mt7915_sta, wcid);
+ stats = &msta->stats;
if (msta->wcid.ext_phy && dev->mt76.phy2)
mphy = dev->mt76.phy2;
--
2.27.0
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-07-09 11:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-09 10:47 [PATCH] mt76: mt7915: potential array overflow in mt7915_mcu_tx_rate_report() Dan Carpenter
2020-07-09 10:47 ` Dan Carpenter
2020-07-09 10:47 ` Dan Carpenter
2020-07-09 11:04 ` [PATCH v2] " Dan Carpenter
2020-07-09 11:04 ` Dan Carpenter
2020-07-09 11:04 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.