[PATCH net v2 0/2] net: dsa: mxl862xx: bound FDB dump walk and validate BRIDGE_ALLOC ids

[PATCH net v2 0/2] net: dsa: mxl862xx: bound FDB dump walk and validate BRIDGE_ALLOC ids

[memoriaru]

From: Zhang Hewei <memoriaru@foxmail.com>

Here is a v2 of the two mxl862xx robustness fixes, reworked after
the sashiko AI review.

Changes since v1 (https://lore.kernel.org/netdev/tencent_A56873DA4A296F5D794B6A5989A180A21C05@qq.com/):

- the walk limit is now checked after the "last" test: the walk
  needs one read more than the number of entries it reports, so
  checking before it rejected a correctly ended walk of a full table
- macro name, error message and comments say "conservative read
  limit" instead of "table capacity", which the exact value is not
- the "growing kernel memory without bound" wording is gone:
  entries whose port id does not match never reach the callback,
  which is exactly why the walk needs its own bound (the netlink
  dump skb never fills)
- BRIDGE_ALLOC validation now also rejects 0xffff (INVALID_HANDLE is
  documented but never defined; the MaxLinear GSW API uses 0xffff)
  and ids already in use, closing the duplicate-FID collapse
- Fixes: tags added (both issues come from the bridge offloading
  commit), self Reported-by trailers dropped, and the comment hunk
  that v1 carried in patch 2 is folded into patch 1

Base-commit: f47d223627f80cb7a1eb14c585fe6cb35de5e984

Zhang Hewei (2):
  net: dsa: mxl862xx: bound FDB dump walk with a read limit
  net: dsa: mxl862xx: validate the bridge id returned by BRIDGE_ALLOC

 drivers/net/dsa/mxl862xx/mxl862xx-api.h | 1584 +++++++++++++++++++++++
 drivers/net/dsa/mxl862xx/mxl862xx.c     |   57 +-
 drivers/net/dsa/mxl862xx/mxl862xx.h     |  343 +++++
 3 files changed, 1983 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/dsa/mxl862xx/mxl862xx-api.h
 create mode 100644 drivers/net/dsa/mxl862xx/mxl862xx.h

-- 
2.39.5 (Apple Git-154)

Re: [PATCH net v2 0/2] net: dsa: mxl862xx: bound FDB dump walk and validate BRIDGE_ALLOC ids

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net v2 0/2] net: dsa: mxl862xx: bound FDB dump walk and validate BRIDGE_ALLOC ids

[memoriaru]

From: Zhang Hewei <memoriaru@foxmail.com>

Hi,

Same context as the v1 thread, replying here for the v2 series:

Discovery: manual code inspection, while auditing this driver's
handling of device-controlled responses (the MAC table walk and the
BRIDGE_ALLOC reply).

Trigger status: the unbounded-walk mechanism was reproduced in-kernel
-- a self-contained module carrying the verbatim loop shape of
mxl862xx_port_fdb_dump() (device transport stubbed) built against the
official Ubuntu 6.8.0-71 headers and run under QEMU (TCG, unmodified
official vmlinuz) never terminated on its own (100M iterations logged;
a real table holds 4-16k entries). The reproduction is automated in
Docker and needs no special hardware. I do not have access to mxl862xx
hardware, so this exercises the driver-side loop logic, not a real
device.

The BRIDGE_ALLOC validation is logic-level: the driver consumed the
firmware-supplied id with only a transport status check, so the
reserved id 0 (which doubles as the "not allocated" sentinel and the
CPU-port bridge) was accepted verbatim. Whether compliant firmware can
be made to return such values is unknown without hardware; the checks
guard against firmware that breaks the API.

The v2 diffs themselves are small and mechanical and have not been
compile-tested against a full kernel tree (no hardware here); the
reproduction module compiles and runs on the official kernel.

Thanks,
Zhang Hewei

Re: [PATCH net v2 0/2] net: dsa: mxl862xx: bound FDB dump walk and validate BRIDGE_ALLOC ids

[Jakub Kicinski]

On Sat, 24 Oct 2026 05:03:22 +0800 memoriaru@foxmail.com wrote:
> Same context as the v1 thread, replying here for the v2 series:

Please add the relevant stuff to the cover letter when reposting.
The v2 did not apply so respin will be needed.
Also - please make sure you add appropriate people to CC on the cover
letter as well.
-- 
pw-bot: cr

[PATCH net v3 0/2] net: dsa: mxl862xx: bound FDB dump walk and validate BRIDGE_ALLOC ids

[memoriaru]

From: Zhang Hewei <memoriaru@foxmail.com>

Hello,

v3 of the two mxl862xx robustness fixes, rebased onto the current
net tree as requested.

The v2 series did not apply because it still carried full copies of
mxl862xx-api.h and mxl862xx.h (the driver pre-dating the upstream
restructure into mxl862xx-host.c / mxl862xx-phylink.c). Both fixes
are now re-ported onto the restructured driver; no driver files are
created or renamed by this series anymore, the two patches only
touch drivers/net/dsa/mxl862xx/mxl862xx.c.

The bugs themselves are unchanged in the restructured code:
mxl862xx_port_fdb_dump() still walks the MAC table in a while(true)
loop whose only exit is the device-controlled "last" flag, and
mxl862xx_allocate_bridge() still returns the firmware-supplied
bridge id without validating it.

v1: https://lore.kernel.org/netdev/tencent_5A7AEC56CDB23A83120519D0C074B592C706@qq.com/
v2: https://lore.kernel.org/netdev/tencent_6030CC520C02054D1B2925AEEF6FA92DC30A@qq.com/

Changes since v2:
- rebase onto net (fa503935c2d6): re-ported the FDB dump read limit
  to the restructured mxl862xx_port_fdb_dump() (the while(true) +
  "last" exit is unchanged, the limit check and its comment moved
  verbatim); re-ported the BRIDGE_ALLOC validation to the
  restructured mxl862xx_allocate_bridge() (unchanged logic)
- dropped the mxl862xx-api.h / mxl862xx.h file creations that v2
  carried: both files exist upstream since the restructure, so the
  series is now two pure fixes to drivers/net/dsa/mxl862xx/mxl862xx.c

Changes since v1:
- the walk limit is now checked after the "last" test: the walk
  needs one read more than the number of entries it reports, so
  checking before it rejected a correctly ended walk of a full table
- macro name, error message and comments say "conservative read
  limit" instead of "table capacity", which the exact value is not
- the "growing kernel memory without bound" wording is gone:
  entries whose port id does not match never reach the callback,
  which is exactly why the walk needs its own bound (the netlink
  dump skb never fills)
- BRIDGE_ALLOC validation now also rejects 0xffff (INVALID_HANDLE is
  documented but never defined; the MaxLinear GSW API uses 0xffff)
  and ids already in use, closing the duplicate-FID collapse
- Fixes: tags added (both issues come from the bridge offloading
  commit), self Reported-by trailers dropped

Base-commit: fa503935c2d6603856dafff0a0852631ab6e9503

Cc: Daniel Golle <daniel@makrotopia.org> (driver author)
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Johan Hovold <johan@kernel.org>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: netdev@vger.kernel.org

Zhang Hewei (2):
  net: dsa: mxl862xx: bound FDB dump walk with a read limit
  net: dsa: mxl862xx: validate the bridge id returned by BRIDGE_ALLOC

 drivers/net/dsa/mxl862xx/mxl862xx.c | 59 ++++++++++++++++++++++++++---
 1 file changed, 56 insertions(+), 3 deletions(-)

-- 
2.39.5 (Apple Git-154)

[PATCH net v3 1/2] net: dsa: mxl862xx: bound FDB dump walk with a read limit

[memoriaru]

From: Zhang Hewei <memoriaru@foxmail.com>

mxl862xx_port_fdb_dump() walks the MAC table in a while (true) loop
whose only exit is the device-controlled "last" flag. Entries whose
port id does not match the dumped port are skipped without invoking
the DSA callback, so the netlink dump buffer never fills either: a
device (or its firmware) that keeps answering with entries for other
ports and never sets last keeps the driver walking forever with RTNL
held.

The exact MAC table capacity is not part of the public driver API.
Use a conservative read limit (16k, well above any documented table
size for this family) and return -EIO when it is exceeded. The limit
is checked after the "last" test so that a correctly ended walk of a
full table is not rejected: the walk needs one read more than the
number of entries it reports.

Fixes: 340bdf984613c4a9 ("net: dsa: mxl862xx: implement bridge offloading")
Signed-off-by: Zhang Hewei <memoriaru@foxmail.com>
---
 drivers/net/dsa/mxl862xx/mxl862xx.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/net/dsa/mxl862xx/mxl862xx.c b/drivers/net/dsa/mxl862xx/mxl862xx.c
index e05ad52cd..37e8c18f1 100644
--- a/drivers/net/dsa/mxl862xx/mxl862xx.c
+++ b/drivers/net/dsa/mxl862xx/mxl862xx.c
@@ -30,6 +30,11 @@
  */
 #define MXL862XX_STATS_POLL_INTERVAL	(2 * HZ)
 
+/* Conservative bound for the MAC table read loop: the exact capacity is
+ * not part of the public driver API (see mxl862xx_port_fdb_dump()).
+ */
+#define MXL862XX_MAC_TABLE_WALK_LIMIT	16384
+
 struct mxl862xx_mib_desc {
 	unsigned int size;
 	unsigned int offset;
@@ -1503,6 +1508,7 @@ static int mxl862xx_port_fdb_dump(struct dsa_switch *ds, int port,
 	struct mxl862xx_mac_table_read param = { .initial = 1 };
 	struct mxl862xx_priv *priv = ds->priv;
 	u32 entry_port_id;
+	int count = 0;
 	int ret;
 
 	while (true) {
@@ -1513,6 +1519,22 @@ static int mxl862xx_port_fdb_dump(struct dsa_switch *ds, int port,
 		if (param.last)
 			break;
 
+		/* The exact MAC table capacity is not part of the public
+		 * driver API. Use a conservative read limit: 16k is well
+		 * above any documented table size for this family, while a
+		 * device that never sets last (or keeps answering with
+		 * entries for other ports, which never reach the callback)
+		 * would otherwise keep us walking forever with RTNL held.
+		 * Checked after the "last" test so that a correctly ended
+		 * walk of a full table is not rejected.
+		 */
+		if (count++ >= MXL862XX_MAC_TABLE_WALK_LIMIT) {
+			dev_err(ds->dev,
+				"FDB dump walk exceeded %d reads, aborting\n",
+				MXL862XX_MAC_TABLE_WALK_LIMIT);
+			return -EIO;
+		}
+
 		entry_port_id = le32_to_cpu(param.port_id);
 
 		if (entry_port_id == port) {
-- 
2.39.5 (Apple Git-154)

[PATCH net v3 2/2] net: dsa: mxl862xx: validate the bridge id returned by BRIDGE_ALLOC

[memoriaru]

From: Zhang Hewei <memoriaru@foxmail.com>

mxl862xx_allocate_bridge() only checks the transport status and
returns the firmware-supplied bridge id verbatim. Three protocol
violations slip through:

- id 0 is reserved ("not used for historic reasons") and doubles as
  the "not allocated" value in priv->bridges[] and priv->ports[].fid,
  so a port or bridge configured with it looks unallocated: later
  joins allocate again and FDB/MDB lookups fail with -ENOENT. It also
  collides with MXL862XX_DEFAULT_BRIDGE (the CPU port), merging the
  port's filtering domain with the CPU port's.
- the API documents INVALID_HANDLE for failed allocations without
  defining its value; the MaxLinear GSW API uses 0xffff, so reject
  that value as well.
- duplicate ids collapse the FID isolation of two domains: FDB and
  MDB entries are matched by FID, VID and MAC, so deleting an entry
  in one domain can remove an entry belonging to the other.

Firmware that follows the API never returns these values; the checks
only guard against firmware that breaks it.

Fixes: 340bdf984613c4a9 ("net: dsa: mxl862xx: implement bridge offloading")
Signed-off-by: Zhang Hewei <memoriaru@foxmail.com>
---
 drivers/net/dsa/mxl862xx/mxl862xx.c | 37 ++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mxl862xx/mxl862xx.c b/drivers/net/dsa/mxl862xx/mxl862xx.c
index 37e8c18f1..d8d58f280 100644
--- a/drivers/net/dsa/mxl862xx/mxl862xx.c
+++ b/drivers/net/dsa/mxl862xx/mxl862xx.c
@@ -589,16 +589,51 @@ static int mxl862xx_vf_alloc(struct mxl862xx_priv *priv,
 	return mxl862xx_vf_entry_discard(priv, vf->block_id, 0);
 }
 
+/* Firmware bridge ids are stored in priv->bridges[] (bridged ports)
+ * and priv->ports[].fid (standalone ports, which also allocate one),
+ * so a fresh allocation must not collide with either.
+ */
+static bool mxl862xx_bridge_id_in_use(struct mxl862xx_priv *priv, u16 id)
+{
+	int i;
+
+	for (i = 0; i <= MXL862XX_MAX_BRIDGES; i++)
+		if (priv->bridges[i] == id)
+			return true;
+
+	for (i = 0; i < MXL862XX_MAX_PORTS; i++)
+		if (priv->ports[i].fid == id)
+			return true;
+
+	return false;
+}
+
 static int mxl862xx_allocate_bridge(struct mxl862xx_priv *priv)
 {
 	struct mxl862xx_bridge_alloc br_alloc = {};
+	u16 id;
 	int ret;
 
 	ret = MXL862XX_API_READ(priv, MXL862XX_BRIDGE_ALLOC, br_alloc);
 	if (ret)
 		return ret;
 
-	return le16_to_cpu(br_alloc.bridge_id);
+	id = le16_to_cpu(br_alloc.bridge_id);
+
+	/* The API reserves id 0 ("not used for historic reasons"), which
+	 * also doubles as the "not allocated" value in priv->bridges[]
+	 * and priv->ports[].fid, and collides with
+	 * MXL862XX_DEFAULT_BRIDGE (the CPU port). It documents
+	 * INVALID_HANDLE for failed allocations without defining its
+	 * value; the MaxLinear GSW API uses 0xffff, so reject that too.
+	 * A duplicate id would collapse the FID isolation of two domains
+	 * (FDB and MDB entries are matched by FID, VID and MAC).
+	 * Firmware that follows the API never returns these values.
+	 */
+	if (!id || id == 0xffff || mxl862xx_bridge_id_in_use(priv, id))
+		return -EIO;
+
+	return id;
 }
 
 static void mxl862xx_free_bridge(struct dsa_switch *ds,
-- 
2.39.5 (Apple Git-154)

Re: [PATCH net v3 1/2] net: dsa: mxl862xx: bound FDB dump walk with a read limit

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.