[PATCH wireless] wifi: mt76: mt792x: validate ACPI SAR table length before parsing

[PATCH wireless] wifi: mt76: mt792x: validate ACPI SAR table length before parsing

[Devin Wittmayer]

Both SAR readers derive the entry count from the ACPI buffer length.
Nothing checks that the length is at least the header size, that it
divides evenly into whole entries, or that the count the table declares
in nr_tbl matches.  A malformed table whose length happens to land the
count inside the accepted range is parsed with the wrong layout and its
limits are applied to the radio.

mt792x_acpi_read() allocates exactly package.count bytes and only rejects
counts below 4, so a four byte table is possible.  The MTDS path already
reads ->enable at offset 4 before any length check, past the end of such
a buffer.

Take the length checks first, then read the header fields, then compare
the derived count against nr_tbl.

Seen on an ASUS ROG Zephyrus G15 GA503RM whose MTGS and MTDS are both
malformed.  The parser accepted them and applied a limit that clamped
every txpower_sku entry to tmac 4, about 2 dBm.  With them rejected,
txpower goes from 3.00 dBm to 23.00 dBm and txpower_sku spreads back
across OFDM/HT20/HT40/VHT80/VHT160.

Fixes: f965333e491e ("mt76: mt7921: introduce ACPI SAR support")
Link: https://github.com/morrownr/mt76/issues/62
Reported-by: Sagun Kayastha <sgn.kayastha@gmail.com>
Tested-by: Sagun Kayastha <sgn.kayastha@gmail.com>
Signed-off-by: Devin Wittmayer <lucid_duck@justthetip.ca>
---
 .../wireless/mediatek/mt76/mt792x_acpi_sar.c  | 28 ++++++++++++++++---
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt792x_acpi_sar.c b/drivers/net/wireless/mediatek/mt76/mt792x_acpi_sar.c
index 946dd79..7e72c38 100644
--- a/drivers/net/wireless/mediatek/mt76/mt792x_acpi_sar.c
+++ b/drivers/net/wireless/mediatek/mt76/mt792x_acpi_sar.c
@@ -114,6 +114,7 @@ mt792x_asar_acpi_read_mtds(struct mt792x_dev *dev, u8 **table, u8 version)
 {
 	int len, ret, sarlen, prelen, tblcnt;
 	bool enable;
+	u8 nr_tbl;
 
 	ret = mt792x_acpi_read(dev, MT792x_ACPI_MTDS, table, &len);
 	if (ret)
@@ -122,12 +123,10 @@ mt792x_asar_acpi_read_mtds(struct mt792x_dev *dev, u8 **table, u8 version)
 	/* Table content validation */
 	switch (version) {
 	case 1:
-		enable = ((struct mt792x_asar_dyn *)*table)->enable;
 		sarlen = sizeof(struct mt792x_asar_dyn_limit);
 		prelen = sizeof(struct mt792x_asar_dyn);
 		break;
 	case 2:
-		enable = ((struct mt792x_asar_dyn_v2 *)*table)->enable;
 		sarlen = sizeof(struct mt792x_asar_dyn_limit_v2);
 		prelen = sizeof(struct mt792x_asar_dyn_v2);
 		break;
@@ -135,8 +134,19 @@ mt792x_asar_acpi_read_mtds(struct mt792x_dev *dev, u8 **table, u8 version)
 		return -EINVAL;
 	}
 
+	if (len < prelen || (len - prelen) % sarlen)
+		return -EINVAL;
+
+	if (version == 1) {
+		enable = ((struct mt792x_asar_dyn *)*table)->enable;
+		nr_tbl = ((struct mt792x_asar_dyn *)*table)->nr_tbl;
+	} else {
+		enable = ((struct mt792x_asar_dyn_v2 *)*table)->enable;
+		nr_tbl = ((struct mt792x_asar_dyn_v2 *)*table)->nr_tbl;
+	}
+
 	tblcnt = (len - prelen) / sarlen;
-	if (!enable ||
+	if (!enable || tblcnt != nr_tbl ||
 	    tblcnt > MT792x_ASAR_MAX_DYN || tblcnt < MT792x_ASAR_MIN_DYN)
 		return -EINVAL;
 
@@ -148,6 +158,7 @@ static int
 mt792x_asar_acpi_read_mtgs(struct mt792x_dev *dev, u8 **table, u8 version)
 {
 	int len, ret, sarlen, prelen, tblcnt;
+	u8 nr_tbl;
 
 	ret = mt792x_acpi_read(dev, MT792x_ACPI_MTGS, table, &len);
 	if (ret)
@@ -167,8 +178,17 @@ mt792x_asar_acpi_read_mtgs(struct mt792x_dev *dev, u8 **table, u8 version)
 		return -EINVAL;
 	}
 
+	if (len < prelen || (len - prelen) % sarlen)
+		return -EINVAL;
+
+	if (version == 1)
+		nr_tbl = ((struct mt792x_asar_geo *)*table)->nr_tbl;
+	else
+		nr_tbl = ((struct mt792x_asar_geo_v2 *)*table)->nr_tbl;
+
 	tblcnt = (len - prelen) / sarlen;
-	if (tblcnt > MT792x_ASAR_MAX_GEO || tblcnt < MT792x_ASAR_MIN_GEO)
+	if (tblcnt != nr_tbl ||
+	    tblcnt > MT792x_ASAR_MAX_GEO || tblcnt < MT792x_ASAR_MIN_GEO)
 		return -EINVAL;
 
 	return 0;
-- 
2.55.0

Re: [PATCH wireless] wifi: mt76: mt792x: validate ACPI SAR table length before parsing

[Devin Wittmayer]

I've been back through the GA503RM's bytes since sending this, and
calling those tables malformed isn't right. Both are version 2, and MTCL
is what declares version 1 for all three, so they get read as v1 and
every field shifts a byte:

  MTGS  28 bytes   as v1: (28-6) % 5 = 2, nr_tbl 0, count 4
                   as v2: (28-7) % 7 = 0, nr_tbl 3, count 3
  MTDS  19 bytes   as v1: (19-6) % 6 = 1, nr_tbl 0, count 2
                   as v2: (19-7) % 12 = 0, nr_tbl 1, count 1

Read as version 2 they're an ordinary SAR table:

  MTGS, FCC   2.4 GHz 14 dBm    5 GHz 14.5 dBm    6 GHz 8.5 dBm
  MTDS        8 to 14.5 dBm across the range

So this patch rejects two good tables and leaves the machine with no SAR
limit at all. The reporter measured 23 dBm afterwards, on a laptop whose
own tables ask for 14.5.

The layouts are 6 + 5n and 7 + 7n bytes with the count declared inside,
so they never collide at any accepted size and the right one falls out
of the shape. I have that written: it applies 14.5 dBm where this patch
applies nothing.

I think parsing them is the better answer, since it gives the machine
what it asks for. Rejecting is safer if you'd sooner not have the driver
guess at a layout.

Devin

[PATCH wireless v2] wifi: mt76: mt792x: pick the SAR table layout from the table itself

[Devin Wittmayer]

The SAR power tables are parsed with a version byte from a different
ACPI table, never their own.

Some firmware declares version 1 and ships version 2 tables. The
entries then start a byte early, and the 6 GHz limits the table
carries are dropped, because the older layout has no room for them.

  ASUS GA503RM, MTCL declares v1:
  MTGS  28 bytes   v1: (28-6) % 5 = 2, no fit   v2: 3 tables
  MTDS  19 bytes   v1: (19-6) % 6 = 1, no fit   v2: 1 table

The layouts accept disjoint lengths, so take whichever one fits and
keep it per table.

Reported-by: Sagun Kayastha <sgn.kayastha@gmail.com>
Closes: https://github.com/morrownr/mt76/issues/62
Fixes: f965333e491e ("mt76: mt7921: introduce ACPI SAR support")
Signed-off-by: Devin Wittmayer <lucid_duck@justthetip.ca>
---
The first version rejected these tables. Going back through the
bytes, they are version 2 tables that MTCL mislabels, so this reads
them instead. Rejecting left the machine with no SAR limit at all.

The layouts accept disjoint lengths over the whole legal range:

  MTDS  v1   12  18  24  30  36  42  48  54
        v2   19  31  43  55  67  79  91 103
  MTGS  v1   21  26  31  36  41  46
        v2   28  35  42  49  56  63

The declared count is checked against the length too.

Fed the reporter's three tables to an MT7922 through an ACPI
override. Patched, both come out v2 and every range is filled:

  stock    frp: 60 60 60 60 60 127 127 127 127 127 127
  patched  frp: 60 60 60 60 60  60  60  60  60  60  60

That adapter is world-roaming, so both arms read the table's WW row.
The FCC row is not reachable here. That is where the misread turns
0x1D into 0x00, and where the reporter measured a flat clamp.

 .../wireless/mediatek/mt76/mt792x_acpi_sar.c  | 110 +++++++++++-------
 .../wireless/mediatek/mt76/mt792x_acpi_sar.h  |   2 +
 2 files changed, 73 insertions(+), 39 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt792x_acpi_sar.c b/drivers/net/wireless/mediatek/mt76/mt792x_acpi_sar.c
index 946dd7956e4a..9f71cf7e2dfd 100644
--- a/drivers/net/wireless/mediatek/mt76/mt792x_acpi_sar.c
+++ b/drivers/net/wireless/mediatek/mt76/mt792x_acpi_sar.c
@@ -108,68 +108,98 @@ mt792x_asar_acpi_read_mtcl(struct mt792x_dev *dev, u8 **table, u8 *version)
 	return ret;
 }
 
+/* A table's layout is decided by the version byte in MTCL, but some firmware
+ * labels a version 2 table as version 1.  Accept whichever layout the table
+ * actually has: for every accepted entry count the two do not overlap, since
+ * one is 6 + 5n bytes and the other 7 + 7n.
+ */
+static bool
+mt792x_asar_fits(u8 *table, int len, int prelen, int sarlen, int nr_off,
+		 int min, int max)
+{
+	int tblcnt;
+
+	if (len < prelen || (len - prelen) % sarlen)
+		return false;
+
+	tblcnt = (len - prelen) / sarlen;
+	if (tblcnt < min || tblcnt > max)
+		return false;
+
+	return tblcnt == table[nr_off];
+}
+
 /* MTDS : Dynamic SAR Power Table */
 static int
-mt792x_asar_acpi_read_mtds(struct mt792x_dev *dev, u8 **table, u8 version)
+mt792x_asar_acpi_read_mtds(struct mt792x_dev *dev, u8 **table, u8 version,
+			   u8 *used)
 {
-	int len, ret, sarlen, prelen, tblcnt;
-	bool enable;
+	int len, ret;
+	u8 *t;
 
 	ret = mt792x_acpi_read(dev, MT792x_ACPI_MTDS, table, &len);
 	if (ret)
 		return ret;
 
+	t = *table;
+
 	/* Table content validation */
-	switch (version) {
-	case 1:
-		enable = ((struct mt792x_asar_dyn *)*table)->enable;
-		sarlen = sizeof(struct mt792x_asar_dyn_limit);
-		prelen = sizeof(struct mt792x_asar_dyn);
-		break;
-	case 2:
-		enable = ((struct mt792x_asar_dyn_v2 *)*table)->enable;
-		sarlen = sizeof(struct mt792x_asar_dyn_limit_v2);
-		prelen = sizeof(struct mt792x_asar_dyn_v2);
-		break;
-	default:
+	if (mt792x_asar_fits(t, len, sizeof(struct mt792x_asar_dyn),
+			     sizeof(struct mt792x_asar_dyn_limit),
+			     offsetof(struct mt792x_asar_dyn, nr_tbl),
+			     MT792x_ASAR_MIN_DYN, MT792x_ASAR_MAX_DYN))
+		*used = 1;
+	else if (mt792x_asar_fits(t, len, sizeof(struct mt792x_asar_dyn_v2),
+				  sizeof(struct mt792x_asar_dyn_limit_v2),
+				  offsetof(struct mt792x_asar_dyn_v2, nr_tbl),
+				  MT792x_ASAR_MIN_DYN, MT792x_ASAR_MAX_DYN))
+		*used = 2;
+	else
 		return -EINVAL;
-	}
 
-	tblcnt = (len - prelen) / sarlen;
-	if (!enable ||
-	    tblcnt > MT792x_ASAR_MAX_DYN || tblcnt < MT792x_ASAR_MIN_DYN)
+	if (!t[offsetof(struct mt792x_asar_dyn, enable)])
 		return -EINVAL;
 
+	if (version && *used != version)
+		dev_info(dev->mt76.dev,
+			 "MTDS is v%u, MTCL says v%u; using v%u\n",
+			 *used, version, *used);
+
 	return 0;
 }
 
 /* MTGS : Geo SAR Power Table */
 static int
-mt792x_asar_acpi_read_mtgs(struct mt792x_dev *dev, u8 **table, u8 version)
+mt792x_asar_acpi_read_mtgs(struct mt792x_dev *dev, u8 **table, u8 version,
+			   u8 *used)
 {
-	int len, ret, sarlen, prelen, tblcnt;
+	int len, ret;
+	u8 *t;
 
 	ret = mt792x_acpi_read(dev, MT792x_ACPI_MTGS, table, &len);
 	if (ret)
 		return ret;
 
+	t = *table;
+
 	/* Table content validation */
-	switch (version) {
-	case 1:
-		sarlen = sizeof(struct mt792x_asar_geo_limit);
-		prelen = sizeof(struct mt792x_asar_geo);
-		break;
-	case 2:
-		sarlen = sizeof(struct mt792x_asar_geo_limit_v2);
-		prelen = sizeof(struct mt792x_asar_geo_v2);
-		break;
-	default:
+	if (mt792x_asar_fits(t, len, sizeof(struct mt792x_asar_geo),
+			     sizeof(struct mt792x_asar_geo_limit),
+			     offsetof(struct mt792x_asar_geo, nr_tbl),
+			     MT792x_ASAR_MIN_GEO, MT792x_ASAR_MAX_GEO))
+		*used = 1;
+	else if (mt792x_asar_fits(t, len, sizeof(struct mt792x_asar_geo_v2),
+				  sizeof(struct mt792x_asar_geo_limit_v2),
+				  offsetof(struct mt792x_asar_geo_v2, nr_tbl),
+				  MT792x_ASAR_MIN_GEO, MT792x_ASAR_MAX_GEO))
+		*used = 2;
+	else
 		return -EINVAL;
-	}
 
-	tblcnt = (len - prelen) / sarlen;
-	if (tblcnt > MT792x_ASAR_MAX_GEO || tblcnt < MT792x_ASAR_MIN_GEO)
-		return -EINVAL;
+	if (version && *used != version)
+		dev_info(dev->mt76.dev,
+			 "MTGS is v%u, MTCL says v%u; using v%u\n",
+			 *used, version, *used);
 
 	return 0;
 }
@@ -205,14 +235,16 @@ int mt792x_init_acpi_sar(struct mt792x_dev *dev)
 		asar->countrylist = NULL;
 	}
 
-	ret = mt792x_asar_acpi_read_mtds(dev, (u8 **)&asar->dyn, asar->ver);
+	ret = mt792x_asar_acpi_read_mtds(dev, (u8 **)&asar->dyn, asar->ver,
+					 &asar->dyn_ver);
 	if (ret) {
 		devm_kfree(dev->mt76.dev, asar->dyn);
 		asar->dyn = NULL;
 	}
 
 	/* MTGS is optional */
-	ret = mt792x_asar_acpi_read_mtgs(dev, (u8 **)&asar->geo, asar->ver);
+	ret = mt792x_asar_acpi_read_mtgs(dev, (u8 **)&asar->geo, asar->ver,
+					 &asar->geo_ver);
 	if (ret) {
 		devm_kfree(dev->mt76.dev, asar->geo);
 		asar->geo = NULL;
@@ -254,7 +286,7 @@ mt792x_asar_get_geo_pwr(struct mt792x_phy *phy,
 		break;
 	}
 
-	if (asar->ver == 1) {
+	if (asar->geo_ver == 1) {
 		band_pwr = &asar->geo->tbl[idx].band[0];
 		max = ARRAY_SIZE(asar->geo->tbl[idx].band);
 	} else {
@@ -297,7 +329,7 @@ mt792x_asar_range_pwr(struct mt792x_phy *phy,
 	if (!capa)
 		return 127;
 
-	if (asar->ver == 1) {
+	if (asar->dyn_ver == 1) {
 		limit = &asar->dyn->tbl[0].frp[0];
 		max = ARRAY_SIZE(asar->dyn->tbl[0].frp);
 	} else {
diff --git a/drivers/net/wireless/mediatek/mt76/mt792x_acpi_sar.h b/drivers/net/wireless/mediatek/mt76/mt792x_acpi_sar.h
index 474033073831..1d42f24b05f6 100644
--- a/drivers/net/wireless/mediatek/mt76/mt792x_acpi_sar.h
+++ b/drivers/net/wireless/mediatek/mt76/mt792x_acpi_sar.h
@@ -105,6 +105,8 @@ struct mt792x_asar_fg {
 
 struct mt792x_acpi_sar {
 	u8 ver;
+	u8 dyn_ver;
+	u8 geo_ver;
 	union {
 		struct mt792x_asar_dyn *dyn;
 		struct mt792x_asar_dyn_v2 *dyn_v2;
-- 
2.55.0

Re: [PATCH wireless v2] wifi: mt76: mt792x: pick the SAR table layout from the table itself

[Devin Wittmayer]

Sagun tested v2 on the GA503RM, the machine these tables came from. Both parse
as v2 and the FCC row reads. Transmit power goes from a flat 2 dBm to the 14.5
the table holds. His numbers are on the issue:

https://github.com/morrownr/mt76/issues/62#issuecomment-5550016870

He also notes the 5 GHz CCK row rising, 36 to 56 against an EEPROM 32. It reads
36 here with and without the patch, so the excess is not new. CCK is not sent on
5 GHz and 2.4 GHz is unaffected.

Tested-by: Sagun Kayastha <sgn.kayastha@gmail.com>

Devin