From 6254f4400110d1bf1708ddb505f0cd37eb36d9b5 Mon Sep 17 00:00:00 2001 From: d00f Date: Sat, 8 Aug 2026 17:13:08 +0200 Subject: [PATCH 1/7] cmd: reject out-of-range numeric arguments instead of wrapping atoi_short() accumulated into a uint16_t without checking, so "vlan 65540" wrapped to 4 and edited VLAN 4 instead of failing. atoi_byte() had the same hole with "300" landing on 44. Both now refuse the digit that would push the value past its type, before it lands. The partial result is deliberately left alone rather than zeroed. Zeroing would give the function one tidy rule, every failure leaves 0, but a caller that ignores the return would then write that 0, and 0 is not a harmless number everywhere. Set as a port MTU it stops the port taking frames: I put 0 on a live 2.5G LAG member and its LACPDU receives moved by 4 in fifteen seconds against 21 on the sibling port, with the partner going expired. Putting the size back recovered both. A wrong number does less damage than that, and the real fix belongs in the callers that ignore the return anyway. The test sits inside the loop rather than after it, so no wider accumulator is needed and the parser stays off the internal RAM budget. 056a30a on the branch in #303 fixes atoi_byte a different way, by widening the accumulator. Whichever lands first, the other hunk should go. --- cmd_parser.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/cmd_parser.c b/cmd_parser.c index 4e8107e..88e39f2 100644 --- a/cmd_parser.c +++ b/cmd_parser.c @@ -191,8 +191,11 @@ uint8_t atoi_byte(__xdata uint8_t *out, uint8_t idx) uint8_t num = 0; while (isnumber(cmd_buffer[idx])) { + uint8_t val = cmd_buffer[idx] - '0'; err = 0; - num = (num * 10) + cmd_buffer[idx] - '0'; + if (num > 25 || (num == 25 && val > 5)) + return 1; + num = (num * 10) + val; idx++; } @@ -209,6 +212,8 @@ uint8_t atoi_short(__xdata uint16_t *vlan, uint8_t idx) while (isnumber(cmd_buffer[idx])) { err = 0; uint8_t val = cmd_buffer[idx] - '0'; + if (*vlan > 6553 || (*vlan == 6553 && val > 5)) + return 1; *vlan = (*vlan * 10) + val; idx++; } From 10d472d8a660e33d63a04d001f683c4463268675 Mon Sep 17 00:00:00 2001 From: d00f Date: Sat, 8 Aug 2026 17:13:28 +0200 Subject: [PATCH 2/7] vlan: reject VLAN IDs the table cannot hold vlan_create() and vlan_delete() wrote the ID straight into the table index register. vlan_get() has refused anything >= 0xfff for a while, so reads were guarded and writes were not: "vlan 4095 1 2" built an entry that no read path can see, and IDs above that either miss the table or alias onto another VLAN. Both writers now enforce the range vlan_get() already did, and parse_vlan() rejects the same values with the usage message so the CLI says why. The check sits after the "vlan 0 mgmt" branch, which legitimately takes 0 to switch the management VLAN off. Costs nothing in RAM: rtl837x_port.rel stays at DSEG 0, OSEG 5, and the image still reports 10207 bytes of XDATA in use. --- cmd_parser.c | 2 ++ rtl837x_port.c | 8 ++++++++ 2 files changed, 10 insertions(+) diff --git a/cmd_parser.c b/cmd_parser.c index 88e39f2..bc6a8bd 100644 --- a/cmd_parser.c +++ b/cmd_parser.c @@ -354,6 +354,8 @@ void parse_vlan(void) print_string("Management VLAN set to "); print_short(management_vlan); write_char('\n'); return; } + if (!vlan_settings.vlan || vlan_settings.vlan > 4094) + goto err; uint8_t w = 2; if (cmd_words_len > w && isletter(cmd_buffer[cmd_words_b[w]])) { register uint8_t i = 0; diff --git a/rtl837x_port.c b/rtl837x_port.c index c540ac3..d203e12 100644 --- a/rtl837x_port.c +++ b/rtl837x_port.c @@ -115,6 +115,9 @@ uint16_t port_pvid_get(uint8_t port) __banked void vlan_delete(uint16_t vlan) __banked { + if (!vlan || vlan >= 0xfff) + return; + print_string("\nvlan_delete called \n"); print_short(vlan); vlan_name_remove(vlan); REG_WRITE(RTL837x_TBL_DATA_IN_A, 0, 0, 0, 0); @@ -197,6 +200,11 @@ __xdata uint16_t vlan_name(register uint16_t vlan) __banked */ void vlan_create(void) __banked { + if (!vlan_settings.vlan || vlan_settings.vlan >= 0xfff) { + print_string("\nInvalid VLAN: "); print_short(vlan_settings.vlan); write_char('\n'); + return; + } + // For now, the CPU-port is always a tagged member: vlan_settings.members |= 0x0200; // Set 10th bit vlan_settings.tagged |= 0x0200; From 99b0fc4b329fc92eb6b94c4152cef4d61be69bac Mon Sep 17 00:00:00 2001 From: d00f Date: Sat, 8 Aug 2026 17:30:25 +0200 Subject: [PATCH 3/7] cmd: give mtu a lower bound as well as an upper one "mtu 1 0" was accepted. The chip takes it verbatim, the port reports 0x0000 back, and it stops passing frames: on a live 2.5G LAG member the LACPDU receives moved by 4 in fifteen seconds against 21 on the sibling port, and the partner went expired. Restoring the size brought both back. Nothing shorter than a minimum Ethernet frame is a usable maximum, so the range is now 64 to 16383. The upper end is unchanged and still comes from the width of the field the value is written into. This also covers most of #312 by accident: "mtu 1 abc" leaves the parse result at 0 and now gets rejected on the bound rather than reaching the register. It does not cover all of it. The handler still ignores what atoi_short() returns, so "mtu 1 99999" stops on a partial 9999 and goes through as a number nobody typed. The GUI is not affected either way, it offers a fixed list of sizes. --- cmd_parser.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd_parser.c b/cmd_parser.c index bc6a8bd..dbc7848 100644 --- a/cmd_parser.c +++ b/cmd_parser.c @@ -737,8 +737,8 @@ void parse_mtu(void) return; } atoi_short(&mtu, cmd_words_b[2]); - if (mtu > 0x3fff) { - print_string("Maximum MTU is 16383\n"); + if (mtu < 64 || mtu > 0x3fff) { + print_string("MTU must be 64..16383\n"); return; } REG_WRITE(RTL8373_REG_MAC_L2_PORT_MAX_LEN + ((uint16_t) p << 8), (mtu >> 10) & 0xf, (mtu >> 2) & 0xff, From 1098e733374f6a7c6c909f0a9c900156f01b68e2 Mon Sep 17 00:00:00 2001 From: d00f Date: Sat, 8 Aug 2026 17:38:10 +0200 Subject: [PATCH 4/7] cmd: stop "mtu" from acting on a value it failed to parse The handler threw away atoi_short()'s return and leant on the range test alone. The range test cannot tell a failed parse from a small number, so "mtu 1 99999" stopped at the partial 9999 and went through as a number nobody typed. With the parse result checked, a failure is rejected with the same message as an out-of-range value. --- cmd_parser.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cmd_parser.c b/cmd_parser.c index dbc7848..e10d8c7 100644 --- a/cmd_parser.c +++ b/cmd_parser.c @@ -736,8 +736,7 @@ void parse_mtu(void) print_string("mtu [port] [size]\n"); return; } - atoi_short(&mtu, cmd_words_b[2]); - if (mtu < 64 || mtu > 0x3fff) { + if (atoi_short(&mtu, cmd_words_b[2]) || mtu < 64 || mtu > 0x3fff) { print_string("MTU must be 64..16383\n"); return; } From 1e19a9abe28c3127c239a2f589ce1cad69320500 Mon Sep 17 00:00:00 2001 From: d00f Date: Sat, 8 Aug 2026 17:38:10 +0200 Subject: [PATCH 5/7] cmd: bound the PVID and say something when it is refused "pvid 1 5000" packed 5000 into the 12-bit PVID field and truncated on the way, so the port ended up with a PVID nobody chose. 0 and anything above 4094 are refused now, matching what the VLAN table can hold. A failed parse used to be dropped without a word; both cases print the usage line. --- cmd_parser.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cmd_parser.c b/cmd_parser.c index e10d8c7..a7d1d0d 100644 --- a/cmd_parser.c +++ b/cmd_parser.c @@ -1568,8 +1568,10 @@ void cmd_parser(void) __banked uint8_t port; port = cmd_buffer[cmd_words_b[1]] - '1'; port = machine.phys_to_log_port[port]; - if (!atoi_short(&pvid, cmd_words_b[2])) + if (!atoi_short(&pvid, cmd_words_b[2]) && pvid && pvid <= 4094) port_pvid_set(port, pvid); + else + print_string("Error: pvid <1-4094>\n"); } else if (cmd_compare(0, "vlan")) { parse_vlan(); } else if (cmd_compare(0, "isolate")) { From 1701d4dc53c0060b840434969dc288b47b1064e9 Mon Sep 17 00:00:00 2001 From: d00f Date: Sat, 8 Aug 2026 17:38:10 +0200 Subject: [PATCH 6/7] cmd: refuse a management VLAN that cannot exist "vlan 5000 mgmt" parked the management interface on a VLAN the table cannot hold, which quietly cuts management off. IDs above 4094 now fall through to the usage message. 0 still switches the management VLAN off, which is the documented way to disable it. --- cmd_parser.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cmd_parser.c b/cmd_parser.c index a7d1d0d..f8bb887 100644 --- a/cmd_parser.c +++ b/cmd_parser.c @@ -347,6 +347,8 @@ void parse_vlan(void) return; } if (cmd_compare(2, "mgmt")) { + if (vlan_settings.vlan > 4094) + goto err; management_vlan = vlan_settings.vlan; if (!vlan_settings.vlan) print_string("Management VLAN disabled\n"); From 930a08ec52168f218b00ee59682caa8099e2a174 Mon Sep 17 00:00:00 2001 From: d00f Date: Sat, 8 Aug 2026 17:46:24 +0200 Subject: [PATCH 7/7] cmd: validate the port argument of mtu and pvid Both handlers turned the first character of the port word into an index with no check at all. "mtu 0 100" computes '0' - '1' = 255, reads far past the end of phys_to_log_port[9], and writes the size to whatever register 0x1250 plus that garbage points at. "pvid 0 2" walks the same path into port_pvid_set(). lag and vlan already validate their port arguments; these two just did not. The port now has to be a single digit 1 to 9, which is exactly the range the mapping table holds. A second digit or a stray letter falls to the usage message. "mtu show" also gained the return it was missing: after printing the table it fell through, derived a port from the word "show" and printed the garbage byte before the length check stopped it. --- cmd_parser.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/cmd_parser.c b/cmd_parser.c index f8bb887..22a4c78 100644 --- a/cmd_parser.c +++ b/cmd_parser.c @@ -730,14 +730,16 @@ void parse_mtu(void) print_string("Port "); print_byte(machine.log_to_phys_port[p]); write_char(' '); print_short(mtu); write_char('\n'); } + return; } - p = cmd_buffer[cmd_words_b[1]] - '1'; - p = machine.phys_to_log_port[p]; - print_byte(p); - if (cmd_words_len != 3) { + if (cmd_words_len != 3 || cmd_buffer[cmd_words_b[1]] < '1' + || cmd_buffer[cmd_words_b[1]] > '9' + || cmd_buffer[cmd_words_b[1] + 1] > ' ') { print_string("mtu [port] [size]\n"); return; } + p = machine.phys_to_log_port[cmd_buffer[cmd_words_b[1]] - '1']; + print_byte(p); if (atoi_short(&mtu, cmd_words_b[2]) || mtu < 64 || mtu > 0x3fff) { print_string("MTU must be 64..16383\n"); return; @@ -1567,11 +1569,11 @@ void cmd_parser(void) __banked } } else if (cmd_compare(0, "pvid") && cmd_words_len == 3) { __xdata uint16_t pvid; - uint8_t port; - port = cmd_buffer[cmd_words_b[1]] - '1'; - port = machine.phys_to_log_port[port]; - if (!atoi_short(&pvid, cmd_words_b[2]) && pvid && pvid <= 4094) - port_pvid_set(port, pvid); + if (cmd_buffer[cmd_words_b[1]] >= '1' + && cmd_buffer[cmd_words_b[1]] <= '9' + && cmd_buffer[cmd_words_b[1] + 1] <= ' ' + && !atoi_short(&pvid, cmd_words_b[2]) && pvid && pvid <= 4094) + port_pvid_set(machine.phys_to_log_port[cmd_buffer[cmd_words_b[1]] - '1'], pvid); else print_string("Error: pvid <1-4094>\n"); } else if (cmd_compare(0, "vlan")) {