[PATCH net 1/2] net: dsa: mxl862xx: bound FDB dump walk by table capacity

[PATCH net 1/2] net: dsa: mxl862xx: bound FDB dump walk by table capacity

[memoriaru]

From: Zhang Hewei <memoriaru@foxmail.com>

The switch MAC table walk in mxl862xx_port_fdb_dump() only exits on the
device-controlled "last" flag. A device (or its firmware) that never
sets last=1 keeps the driver looping forever with RTNL held, and every
matching entry invokes the DSA callback, growing kernel memory without
bound.

The exact MAC table capacity is not part of the public driver API; bound
the walk by a conservative upper bound (16k entries, well above any
documented table size for this family) and return -EIO when exceeded.

Reported-by: memoriaru <memoriaru@foxmail.com>
Signed-off-by: Zhang Hewei <memoriaru@foxmail.com>
---
 drivers/net/dsa/mxl862xx/mxl862xx.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/dsa/mxl862xx/mxl862xx.c b/drivers/net/dsa/mxl862xx/mxl862xx.c
index e05ad52..12cd407 100644
--- a/drivers/net/dsa/mxl862xx/mxl862xx.c
+++ b/drivers/net/dsa/mxl862xx/mxl862xx.c
@@ -1497,12 +1497,15 @@ static int mxl862xx_port_fdb_del(struct dsa_switch *ds, int port,
 	return ret;
 }
 
+#define MXL862XX_MAC_TABLE_FDB_SIZE 16384
+
 static int mxl862xx_port_fdb_dump(struct dsa_switch *ds, int port,
 				  dsa_fdb_dump_cb_t *cb, void *data)
 {
 	struct mxl862xx_mac_table_read param = { .initial = 1 };
 	struct mxl862xx_priv *priv = ds->priv;
 	u32 entry_port_id;
+	unsigned long count = 0;
 	int ret;
 
 	while (true) {
@@ -1510,6 +1513,16 @@ static int mxl862xx_port_fdb_dump(struct dsa_switch *ds, int port,
 		if (ret)
 			return ret;
 
+		/* The MAC table capacity is fixed by the device. A walk longer
+		 * than that means the device (or its firmware) is not
+		 * responding correctly; bail out instead of looping forever
+		 * with RTNL held.
+		 */
+		if (count++ >= MXL862XX_MAC_TABLE_FDB_SIZE) {
+			dev_err(ds->dev, "FDB dump exceeded table capacity\n");
+			return -EIO;
+		}
+
 		if (param.last)
 			break;
 
-- 
2.39.5 (Apple Git-154)

Re: [PATCH net 1/2] net: dsa: mxl862xx: bound FDB dump walk by table capacity

[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 1/2] net: dsa: mxl862xx: bound FDB dump walk by table capacity

[memoriaru]

From: Zhang Hewei <memoriaru@foxmail.com>

Hi,

Replying with the missing context requested by the bot:

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

Trigger status:

Patch 1 (unbounded FDB dump walk): the loop mechanism was reproduced
in-kernel. A self-contained module carrying the verbatim loop shape of
mxl862xx_port_fdb_dump() (device transport stubbed) was built against
the official Ubuntu 6.8.0-71 kernel headers and loaded under QEMU (TCG,
unmodified official vmlinuz): the walk never terminated on its own
(100M iterations logged; a real table holds 4-16k entries). The
reproduction is fully 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 the real device.

Patch 2 (bridge_id == 0 accepted): reproduced at the logic level in
the same module: the driver consumes the firmware-supplied bridge_id
with only a ret < 0 check, so id 0 -- which the API header declares
unused (mxl862xx-api.h:243-244) and which equals
MXL862XX_DEFAULT_BRIDGE (the CPU port) -- is accepted verbatim as the
port FID, merging the port's filtering domain with the CPU port's.
Whether a device can be made to actually return 0 is unknown without
hardware; per the API spec such a reply is already protocol-invalid.

Build status: the reproduction module compiles and runs on the
official kernel; the two patch hunks themselves are small and
mechanical and have not been compile-tested against a full kernel
tree (no mxl862xx hardware available here). Reproduction code can be
shared on request.

Thanks,
Zhang Hewei

Re: [PATCH net 1/2] net: dsa: mxl862xx: bound FDB dump walk by table capacity

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] net: dsa: mxl862xx: off-by-one in mxl862xx_port_fdb_dump()
  capacity check
- [Low] The commit message for "net: dsa: mxl862xx: bound FDB dump walk
  by table capacity" says that with firmware that never sets last=1,…
- [Low] The patch is presented as a fix for an infinite loop with RTNL
  held, but it has no Fixes: tag.

Pre-existing issues:
- [Medium] The kernel-doc for struct mxl862xx_mac_table_read
  (drivers/net/dsa/mxl862xx/mxl862xx-api.h) says "@last: Indicates that…

--

net: dsa: mxl862xx: bound FDB dump walk by table capacity

This commit caps the MAC table walk in mxl862xx_port_fdb_dump() at 16384
entries. If the firmware has not set the last flag by then, the walk
stops with -EIO and logs an error. The goal is to avoid an endless loop
with RTNL held when the firmware never reports the end of the table.

> A device (or its firmware) that never
> sets last=1 keeps the driver looping forever with RTNL held, and every
> matching entry invokes the DSA callback, growing kernel memory without
> bound.

[Severity: Low]
Is the "growing kernel memory without bound" part accurate?

The only callback passed to port_fdb_dump is dsa_user_port_fdb_do_dump(),
reached via dsa_user_fdb_dump()->dsa_port_fdb_dump(). It does not allocate
anything per entry. It writes into the fixed-size netlink dump skb and
returns -EMSGSIZE once the skb is full:

net/dsa/user.c:dsa_user_port_fdb_do_dump() {
    ...
	if (dump->idx < ctx->fdb_idx)
		goto skip;

	nlh = nlmsg_put(dump->skb, portid, seq, RTM_NEWNEIGH,
			sizeof(*ndm), NLM_F_MULTI);
	if (!nlh)
		return -EMSGSIZE;
    ...
nla_put_failure:
	nlmsg_cancel(dump->skb, nlh);
	return -EMSGSIZE;
}

mxl862xx_port_fdb_dump() already returns on any non-zero cb() result, so
the walk ends when the skb fills up. Skipped entries use no space.

Before this patch, the unbounded cost was CPU and RTNL hold time spent
looping over entries that don't match the port, or that are skipped. It
was not memory growth. Could the commit message describe it that way?

> Reported-by: memoriaru <memoriaru@foxmail.com>
> Signed-off-by: Zhang Hewei <memoriaru@foxmail.com>

[Severity: Low]
Should this have a Fixes: tag? The unbounded while (true) walk in
mxl862xx_port_fdb_dump() came in with:

Fixes: 340bdf984613 ("net: dsa: mxl862xx: implement bridge offloading")

The Reported-by: uses the same address as the Signed-off-by:. There is
also no Closes: or Link: tag, which checkpatch warns about. Was this seen
on real hardware?

The trigger also looks narrower than described. Firmware that has
stopped responding already fails in mxl862xx_busy_wait():

	return readx_poll_timeout(mxl862xx_ctrl_read, priv, val,
				  !(val & CTRL_BUSY_MASK), 15, 500000);

Garbled replies are rejected with -EIO by the CRC-6 check in
mxl862xx_issue_cmd() and the CRC-16 check on the read payload in
mxl862xx_api_wrap(). Both of these already leave the loop through the
existing "if (ret) return ret;".

So the only new case covered seems to be firmware that keeps sending
well-formed, CRC-valid replies but never sets last. Could the commit
message and the "not responding correctly" code comment say that more
precisely?

> diff --git a/drivers/net/dsa/mxl862xx/mxl862xx.c b/drivers/net/dsa/mxl862xx/mxl862xx.c
> index e05ad52cd297e..12cd40756a0c9 100644
> --- a/drivers/net/dsa/mxl862xx/mxl862xx.c
> +++ b/drivers/net/dsa/mxl862xx/mxl862xx.c

[ ... ]

> @@ -1510,6 +1513,16 @@ static int mxl862xx_port_fdb_dump(struct dsa_switch *ds, int port,
>  		if (ret)
>  			return ret;
>  
> +		/* The MAC table capacity is fixed by the device. A walk longer
> +		 * than that means the device (or its firmware) is not
> +		 * responding correctly; bail out instead of looping forever
> +		 * with RTNL held.
> +		 */
> +		if (count++ >= MXL862XX_MAC_TABLE_FDB_SIZE) {
> +			dev_err(ds->dev, "FDB dump exceeded table capacity\n");
> +			return -EIO;
> +		}
> +
>  		if (param.last)
>  			break;

[Severity: Medium]
Does this bound count the terminating read as if it were an entry?

The count check runs before the param.last test. Suppose the table
holds exactly MXL862XX_MAC_TABLE_FDB_SIZE entries:

- The 16384 reads that return entries with last == 0 take count from
  0 to 16384.
- The next read is the one with last == 1.
- At that point count++ >= 16384 is already true.

So mxl862xx_port_fdb_dump() logs "FDB dump exceeded table capacity"
and returns -EIO instead of finishing the walk.

A table that is exactly at the cap fails the whole dump, even though
the firmware ended the walk correctly. The error message is also
wrong in that case, because nothing was exceeded. 16384 is meant to
be well above any real table size, so this may not be reachable on
current parts. The check still treats the sentinel as an entry,
though.

Would it be cleaner to test param.last first and only count reads
that actually carry an entry? Moving the bound below the
"if (param.last) break;" keeps the same protection against firmware
that never sets last.

The kernel-doc for struct mxl862xx_mac_table_read suggests the
last == 1 reply may itself carry a valid entry. If so, the counting
here should line up with whatever is decided about handling that
final reply.

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch, but can
this break drop the final MAC table entry?

The kernel-doc for struct mxl862xx_mac_table_read in mxl862xx-api.h
says:

 * @last: Indicates that the read operation returned last entry

That reads as if the last=1 response still carries a valid entry. The
loop breaks before it looks at param.port_id, param.mac or param.tci, so
that response is never passed to cb().

If the firmware fills in the entry fields along with last=1, every FDB
dump would silently leave out the final entry. If it doesn't, the
kernel-doc is misleading. The tree doesn't show which firmware behaviour
is correct. This code dates back to commit 340bdf984613.

Taken together, the code change looks fine. All three points concern
the commit message, the trailers, or code that was already there.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/tencent_A56873DA4A296F5D794B6A5989A180A21C05%40qq.com

Re: [PATCH net 1/2] net: dsa: mxl862xx: bound FDB dump walk by table capacity

[memoriaru]

From: Zhang Hewei <memoriaru@foxmail.com>

Thanks -- reviewed both Medium findings against the tree. Planning a
v2; replies to each below.

[Medium] off-by-one in the capacity check: the fix is warranted, with
one nuance on the premise. The kernel-doc for @last says "Indicates
that the read operation returned last entry", which reads as last
being set on the response that carries the final entry -- under that
reading, a walk of exactly MXL862XX_MAC_TABLE_FDB_SIZE entries
completes before the check trips. If instead the terminal response
carries no entry (and still counts as a read), the current order
rejects a correctly ended full table. The public headers do not
settle which it is, so v2 moves the param.last test above the count
check and drops the protocol dependency entirely; the macro name and
the error message get reworded to "conservative upper bound" to match
the comment.

[Medium] is rejecting only bridge_id 0 enough: fair. INVALID_HANDLE
is referenced by the API docs but never defined or checked by the
driver, so if it is 0xffff (the MaxLinear GSW convention), a failed
allocation would currently be consumed as a FID. v2 rejects 0xffff as
well, and range-checks the id once the driver-side bridge limit is
confirmed. Duplicate-id detection: the cross-domain fdb_del
consequence you describe is real; I'll see how far validation can go
without inventing API facts that are absent from the public headers.

The Lows are accepted too: the comment hunk moves into patch 1, the
"growing kernel memory without bound" wording is dropped (reported
entries consume the fixed netlink dump skb and -EMSGSIZE aborts the
walk; the unbounded case is a device answering forever with entries
that are filtered out before the callback), Fixes: tags will be
added, and the self Reported-by trailer goes away.

pw-bot: cr

[PATCH net 2/2] net: dsa: mxl862xx: reject bridge_id 0 from BRIDGE_ALLOC

[memoriaru]

From: Zhang Hewei <memoriaru@foxmail.com>

The device controls bridge_id in the BRIDGE_ALLOC response.
mxl862xx_allocate_bridge() only checks the transport status (ret < 0)
and returns le16_to_cpu(bridge_id) verbatim.

The API header states that "ID 0 is not used for historic reasons"
(mxl862xx-api.h:243-244), and MXL862XX_DEFAULT_BRIDGE == 0 is used for
the CPU port. Accepting id 0 assigns fid = 0 to a standalone user port,
merging its filtering domain with the CPU port's; duplicate non-zero ids
returned for two ports collapse their per-port FID isolation.

Reject the protocol-invalid value 0 with -EIO.

Reported-by: memoriaru <memoriaru@foxmail.com>
Signed-off-by: Zhang Hewei <memoriaru@foxmail.com>
---
 drivers/net/dsa/mxl862xx/mxl862xx.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/mxl862xx/mxl862xx.c b/drivers/net/dsa/mxl862xx/mxl862xx.c
index 12cd407..dbe2001 100644
--- a/drivers/net/dsa/mxl862xx/mxl862xx.c
+++ b/drivers/net/dsa/mxl862xx/mxl862xx.c
@@ -593,7 +593,16 @@ static int mxl862xx_allocate_bridge(struct mxl862xx_priv *priv)
 	if (ret)
 		return ret;
 
-	return le16_to_cpu(br_alloc.bridge_id);
+	/* Per the API spec, id 0 is not used for historic reasons. Accepting
+	 * it verbatim collides with MXL862XX_DEFAULT_BRIDGE (the CPU port),
+	 * silently merging the port's FID with the CPU port's filtering
+	 * domain.
+	 */
+	ret = le16_to_cpu(br_alloc.bridge_id);
+	if (ret == 0)
+		return -EIO;
+
+	return ret;
 }
 
 static void mxl862xx_free_bridge(struct dsa_switch *ds,
@@ -1513,9 +1522,11 @@ static int mxl862xx_port_fdb_dump(struct dsa_switch *ds, int port,
 		if (ret)
 			return ret;
 
-		/* The MAC table capacity is fixed by the device. A walk longer
-		 * than that means the device (or its firmware) is not
-		 * responding correctly; bail out instead of looping forever
+		/* The exact MAC table capacity is not part of the public
+		 * driver API. Use a conservative upper bound: 16k entries is
+		 * well above any documented table size for this family, while
+		 * a device that stopped responding correctly (e.g. firmware
+		 * crash mid-walk) would otherwise keep us looping forever
 		 * with RTNL held.
 		 */
 		if (count++ >= MXL862XX_MAC_TABLE_FDB_SIZE) {
-- 
2.39.5 (Apple Git-154)

Re: [PATCH net 2/2] net: dsa: mxl862xx: reject bridge_id 0 from BRIDGE_ALLOC

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] mxl862xx_allocate_bridge()
  (drivers/net/dsa/mxl862xx/mxl862xx.c:587-606) still checks the device-…
- [Medium] net: dsa: mxl862xx: off-by-one check order in
  mxl862xx_port_fdb_dump()
- [Low] The commit message does not mention one of the two hunks, and
  that hunk has nothing to do with bridge_id.
- [Low] The commit message has several gaps.

--

net: dsa: mxl862xx: reject bridge_id 0 from BRIDGE_ALLOC

This patch makes mxl862xx_allocate_bridge() return -EIO when the
BRIDGE_ALLOC response carries bridge_id 0. That value is reserved by the
API and collides with MXL862XX_DEFAULT_BRIDGE, which the CPU port uses.
The patch also rewrites the comment on the iteration bound in
mxl862xx_port_fdb_dump().

> mxl862xx_allocate_bridge() only checks the transport status (ret < 0)
> and returns le16_to_cpu(bridge_id) verbatim.

[Severity: Low]
I have a few small questions about the commit message.

The code being replaced checks "if (ret) return ret;", not ret < 0.
Should the description match the code?

Should this have a Fixes: tag? git blame shows all of
mxl862xx_allocate_bridge() came from:

Fixes: 340bdf984613 ("net: dsa: mxl862xx: implement bridge offloading")

The message doesn't mention that 0 is also the "not allocated" value
for priv->bridges[]:

mxl862xx_port_bridge_join() {
    ...
	if (!priv->bridges[bridge.num]) {
		ret = mxl862xx_allocate_bridge(priv);
    ...
}

mxl862xx_get_fid() {
    ...
	case DSA_DB_BRIDGE:
		if (!priv->bridges[db.bridge.num])
			return -ENOENT;
    ...
}

So with an id of 0, the bridge also looked unallocated. Each later join
allocated it again, and bridge FDB and MDB operations failed with -ENOENT.
Would it be worth describing this as well?

Firmware that follows the API never returns 0, so this change guards
against firmware that breaks the API. Could the message say so? It would
help with net vs net-next and stable decisions.

The Reported-by: trailer has the author's own address and there's no
Closes: link. Is that intended?

> diff --git a/drivers/net/dsa/mxl862xx/mxl862xx.c b/drivers/net/dsa/mxl862xx/mxl862xx.c
> index 12cd40756a0c9..dbe20014d090e 100644
> --- a/drivers/net/dsa/mxl862xx/mxl862xx.c
> +++ b/drivers/net/dsa/mxl862xx/mxl862xx.c
> @@ -593,7 +593,16 @@ static int mxl862xx_allocate_bridge(struct mxl862xx_priv *priv)
>  	if (ret)
>  		return ret;
>  
> -	return le16_to_cpu(br_alloc.bridge_id);
> +	/* Per the API spec, id 0 is not used for historic reasons. Accepting
> +	 * it verbatim collides with MXL862XX_DEFAULT_BRIDGE (the CPU port),
> +	 * silently merging the port's FID with the CPU port's filtering
> +	 * domain.
> +	 */
> +	ret = le16_to_cpu(br_alloc.bridge_id);
> +	if (ret == 0)
> +		return -EIO;
> +
> +	return ret;
>  }

[Severity: Medium]
Is rejecting only 0 enough to cover what the commit message describes?
Every other u16 value is still returned as a valid FID.

The kernel-doc for struct mxl862xx_bridge_alloc in mxl862xx-api.h says:

 * @bridge_id: If the bridge allocation is successful, a valid ID will be
 *             returned in this field. Otherwise, INVALID_HANDLE is
 *             returned.

The driver doesn't define or check INVALID_HANDLE. If it is ~0, as in
the MaxLinear GSW API, 65535 would be accepted as a FID.

The commit message also says "duplicate non-zero ids returned for two
ports collapse their per-port FID isolation". However, nothing compares
the returned id with priv->ports[*].fid or priv->bridges[*], so
duplicates still get through.

There's also no upper bound check against the hardware FID range.
Commit 340bdf984613 says the switch supports at most 63 bridges.

Both callers only check for ret < 0 and then use the value:

mxl862xx_port_setup()
    priv->ports[port].fid = ret;
    -> mxl862xx_bridge_config_fwd(), mxl862xx_set_bridge_port()

mxl862xx_port_bridge_join()
    priv->bridges[bridge.num] = ret;
    -> mxl862xx_sync_bridge_members(), mxl862xx_get_fid(), BRIDGE_FREE

mxl862xx_port_fdb_del() matches an entry only by FID, VID and MAC. So if
two domains get the same FID, deleting an FDB entry in one domain can
remove an entry belonging to the other.

Should this check also reject INVALID_HANDLE, ids outside the hardware
range, and ids that are already in use?

[ ... ]

> @@ -1513,9 +1522,11 @@ static int mxl862xx_port_fdb_dump(struct dsa_switch *ds, int port,
>  		if (ret)
>  			return ret;
>  
> -		/* The MAC table capacity is fixed by the device. A walk longer
> -		 * than that means the device (or its firmware) is not
> -		 * responding correctly; bail out instead of looping forever
> +		/* The exact MAC table capacity is not part of the public
> +		 * driver API. Use a conservative upper bound: 16k entries is
> +		 * well above any documented table size for this family, while
> +		 * a device that stopped responding correctly (e.g. firmware
> +		 * crash mid-walk) would otherwise keep us looping forever
>  		 * with RTNL held.
>  		 */
>  		if (count++ >= MXL862XX_MAC_TABLE_FDB_SIZE) {

[Severity: Medium]
The previous patch introduced this, not this one, but this hunk
rewrites the comment above it anyway. Is the bound checked in the
right place?

In mxl862xx_port_fdb_dump() the count check runs before the
param.last test. The final MXL862XX_MAC_TABLEENTRYREAD response only
sets last and carries no entry, but it is still counted against
MXL862XX_MAC_TABLE_FDB_SIZE.

With N valid entries the loop does N + 1 reads. On the last read,
count is already N. If the table holds exactly
MXL862XX_MAC_TABLE_FDB_SIZE entries, the dump returns -EIO and logs
"FDB dump exceeded table capacity". The firmware ended the walk
correctly, yet bridge fdb show fails on a full table.

The macro name and the error message both call 16384 the table
capacity. A full table is exactly the case the bound is meant to
allow. Should the param.last test be moved above the count check?

[Severity: Low]
This isn't a bug, but this hunk has nothing to do with bridge_id, and the
commit message doesn't mention it. It rewrites the comment that the
previous patch in the series added ("net: dsa: mxl862xx: bound FDB dump
walk by table capacity").

Should it be squashed into that patch instead?

The new comment says 16384 is not the table capacity, but the code
around it still says it is:

#define MXL862XX_MAC_TABLE_FDB_SIZE 16384
...
			dev_err(ds->dev, "FDB dump exceeded table capacity\n");

The previous patch's subject also says "by table capacity". Should the
macro name and the error message be changed to match the new comment?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/tencent_A56873DA4A296F5D794B6A5989A180A21C05%40qq.com