[PATCH] bridge: tighten VLAN parsing

[PATCH] bridge: tighten VLAN parsing

[William Gonzalez]

---
 bridge/fdb.c    |  45 +++++++++++--------
 bridge/mdb.c    |  21 ++++++---
 bridge/vlan.c   | 113 ++++++++++++++++++------------------------------
 include/utils.h |   2 +
 lib/utils.c     |  51 ++++++++++++++++++++++
 5 files changed, 136 insertions(+), 96 deletions(-)

diff --git a/bridge/fdb.c b/bridge/fdb.c
index 05f093b5..fadd82ae 100644
--- a/bridge/fdb.c
+++ b/bridge/fdb.c
@@ -364,10 +364,14 @@ static int fdb_show(int argc, char **argv)
 			NEXT_ARG();
 			br = *argv;
 		} else if (strcmp(*argv, "vlan") == 0) {
+			__u16 vid;
+
 			NEXT_ARG();
 			if (filter_vlan)
 				duparg("vlan", *argv);
-			filter_vlan = atoi(*argv);
+			if (get_vlan(&vid, *argv))
+				invarg("invalid vlan", *argv);
+			filter_vlan = vid;
 		} else if (strcmp(*argv, "state") == 0) {
 			unsigned int state;
 
@@ -474,7 +478,8 @@ static int fdb_modify(int cmd, int flags, int argc, char **argv)
 	unsigned long src_vni = ~0;
 	unsigned int via = 0;
 	char *endptr;
-	short vid = -1;
+	__u16 vid = 0;
+	bool vid_set = false;
 	__u32 nhid = 0;
 
 	while (argc > 0) {
@@ -537,10 +542,12 @@ static int fdb_modify(int cmd, int flags, int argc, char **argv)
 			req.ndm.ndm_state |= NUD_REACHABLE;
 			req.ndm.ndm_state &= ~NUD_NOARP;
 		} else if (matches(*argv, "vlan") == 0) {
-			if (vid >= 0)
+			if (vid_set)
 				duparg2("vlan", *argv);
 			NEXT_ARG();
-			vid = atoi(*argv);
+			if (get_vlan(&vid, *argv))
+				invarg("invalid vlan", *argv);
+			vid_set = true;
 		} else if (matches(*argv, "use") == 0) {
 			req.ndm.ndm_flags |= NTF_USE;
 		} else if (matches(*argv, "extern_learn") == 0) {
@@ -595,7 +602,7 @@ static int fdb_modify(int cmd, int flags, int argc, char **argv)
 	if (dst_ok)
 		addattr_l(&req.n, sizeof(req), NDA_DST, &dst.data, dst.bytelen);
 
-	if (vid >= 0)
+	if (vid_set)
 		addattr16(&req.n, sizeof(req), NDA_VLAN, vid);
 	if (nhid > 0)
 		addattr32(&req.n, sizeof(req), NDA_NH_ID, nhid);
@@ -644,7 +651,8 @@ static int fdb_get(int argc, char **argv)
 	char abuf[ETH_ALEN];
 	int br_ifindex = 0;
 	char *addr = NULL;
-	short vlan = -1;
+	__u16 vlan = 0;
+	bool vlan_set = false;
 	char *endptr;
 	int ret;
 
@@ -669,10 +677,12 @@ static int fdb_get(int argc, char **argv)
 		} else if (matches(*argv, "master") == 0) {
 			req.ndm.ndm_flags |= NTF_MASTER;
 		} else if (matches(*argv, "vlan") == 0) {
-			if (vlan >= 0)
+			if (vlan_set)
 				duparg2("vlan", *argv);
 			NEXT_ARG();
-			vlan = atoi(*argv);
+			if (get_vlan(&vlan, *argv))
+				invarg("invalid vlan", *argv);
+			vlan_set = true;
 		} else if (matches(*argv, "dynamic") == 0) {
 			filter_dynamic = 1;
 		} else {
@@ -702,7 +712,7 @@ static int fdb_get(int argc, char **argv)
 
 	addattr_l(&req.n, sizeof(req), NDA_LLADDR, abuf, ETH_ALEN);
 
-	if (vlan >= 0)
+	if (vlan_set)
 		addattr16(&req.n, sizeof(req), NDA_VLAN, vlan);
 
 	if (vni != ~0)
@@ -758,7 +768,9 @@ static int fdb_flush(int argc, char **argv)
 	};
 	unsigned short ndm_state_mask = 0;
 	unsigned short ndm_flags_mask = 0;
-	short vid = -1, brport_ifidx = -1;
+	short brport_ifidx = -1;
+	__u16 vid = 0;
+	bool vid_set = false;
 	char *d = NULL, *brport = NULL;
 	unsigned short ndm_flags = 0;
 	unsigned short ndm_state = 0;
@@ -832,10 +844,12 @@ static int fdb_flush(int argc, char **argv)
 			NEXT_ARG();
 			brport = *argv;
 		} else if (strcmp(*argv, "vlan") == 0) {
-			if (vid >= 0)
+			if (vid_set)
 				duparg2("vlan", *argv);
 			NEXT_ARG();
-			vid = atoi(*argv);
+			if (get_vlan(&vid, *argv))
+				invarg("invalid vlan", *argv);
+			vid_set = true;
 		} else if (strcmp(*argv, "src_vni") == 0) {
 			NEXT_ARG();
 			src_vni = strtoul(*argv, &endptr, 0);
@@ -901,11 +915,6 @@ static int fdb_flush(int argc, char **argv)
 		}
 	}
 
-	if (vid >= 4096) {
-		fprintf(stderr, "Invalid VLAN ID \"%hu\"\n", vid);
-		return -1;
-	}
-
 	/* if self and master were not specified assume self */
 	if (!(ndm_flags & (NTF_SELF | NTF_MASTER)))
 		ndm_flags |= NTF_SELF;
@@ -914,7 +923,7 @@ static int fdb_flush(int argc, char **argv)
 	req.ndm.ndm_state = ndm_state;
 	if (brport_ifidx > -1)
 		addattr32(&req.n, sizeof(req), NDA_IFINDEX, brport_ifidx);
-	if (vid > -1)
+	if (vid_set)
 		addattr16(&req.n, sizeof(req), NDA_VLAN, vid);
 	if (src_vni != ~0)
 		addattr32(&req.n, sizeof(req), NDA_SRC_VNI, src_vni);
diff --git a/bridge/mdb.c b/bridge/mdb.c
index 112deeb9..b1d6ec28 100644
--- a/bridge/mdb.c
+++ b/bridge/mdb.c
@@ -452,10 +452,14 @@ static int mdb_show(int argc, char **argv)
 				duparg("dev", *argv);
 			filter_dev = *argv;
 		} else if (strcmp(*argv, "vid") == 0) {
+			__u16 vid;
+
 			NEXT_ARG();
 			if (filter_vlan)
 				duparg("vid", *argv);
-			filter_vlan = atoi(*argv);
+			if (get_vlan(&vid, *argv))
+				invarg("invalid vlan", *argv);
+			filter_vlan = vid;
 		}
 		argc--; argv++;
 	}
@@ -706,7 +710,7 @@ static int mdb_modify(int cmd, int flags, int argc, char **argv)
 	char *src_list = NULL, *proto = NULL, *dst = NULL;
 	struct br_mdb_entry entry = {};
 	bool set_attrs = false;
-	short vid = 0;
+	__u16 vid = 0;
 
 	while (argc > 0) {
 		if (strcmp(*argv, "dev") == 0) {
@@ -725,7 +729,8 @@ static int mdb_modify(int cmd, int flags, int argc, char **argv)
 			;/* nothing */
 		} else if (strcmp(*argv, "vid") == 0) {
 			NEXT_ARG();
-			vid = atoi(*argv);
+			if (get_vlan(&vid, *argv))
+				invarg("invalid vlan", *argv);
 		} else if (strcmp(*argv, "src") == 0) {
 			NEXT_ARG();
 			src = *argv;
@@ -868,7 +873,7 @@ static int mdb_get(int argc, char **argv)
 	struct br_mdb_entry entry = {};
 	struct nlmsghdr *answer;
 	bool get_attrs = false;
-	short vid = 0;
+	__u16 vid = 0;
 	int ret = 0;
 
 	while (argc > 0) {
@@ -880,7 +885,8 @@ static int mdb_get(int argc, char **argv)
 			grp = *argv;
 		} else if (strcmp(*argv, "vid") == 0) {
 			NEXT_ARG();
-			vid = atoi(*argv);
+			if (get_vlan(&vid, *argv))
+				invarg("invalid vlan", *argv);
 		} else if (strcmp(*argv, "src") == 0) {
 			NEXT_ARG();
 			src = *argv;
@@ -963,7 +969,7 @@ static int mdb_flush(int argc, char **argv)
 	struct br_mdb_entry entry = {};
 	unsigned short state_mask = 0;
 	bool set_attrs = false;
-	short vid = 0;
+	__u16 vid = 0;
 
 	while (argc > 0) {
 		if (strcmp(*argv, "dev") == 0) {
@@ -974,7 +980,8 @@ static int mdb_flush(int argc, char **argv)
 			p = *argv;
 		} else if (strcmp(*argv, "vid") == 0) {
 			NEXT_ARG();
-			vid = atoi(*argv);
+			if (get_vlan(&vid, *argv))
+				invarg("invalid vlan", *argv);
 		} else if (strcmp(*argv, "src_vni") == 0) {
 			NEXT_ARG();
 			src_vni = *argv;
diff --git a/bridge/vlan.c b/bridge/vlan.c
index 09c01153..0f0c8333 100644
--- a/bridge/vlan.c
+++ b/bridge/vlan.c
@@ -165,8 +165,10 @@ static int vlan_modify(int cmd, int argc, char **argv)
 		.ifm.ifi_family = PF_BRIDGE,
 	};
 	char *d = NULL;
-	short vid = -1;
-	short vid_end = -1;
+	__u16 vid = 0;
+	__u16 vid_end = 0;
+	bool vid_set = false;
+	bool vid_range = false;
 	struct rtattr *afspec;
 	struct bridge_vlan_info vinfo = {};
 	bool tunnel_info_set = false;
@@ -179,19 +181,15 @@ static int vlan_modify(int cmd, int argc, char **argv)
 			NEXT_ARG();
 			d = *argv;
 		} else if (strcmp(*argv, "vid") == 0) {
-			char *p;
-
 			NEXT_ARG();
-			p = strchr(*argv, '-');
-			if (p) {
-				*p = '\0';
-				p++;
-				vid = atoi(*argv);
-				vid_end = atoi(p);
+			if (vid_set)
+				duparg2("vid", *argv);
+			if (get_vlan_range(&vid, &vid_end, *argv))
+				invarg("invalid vlan", *argv);
+			vid_set = true;
+			vid_range = vid_end != vid;
+			if (vid_range)
 				vinfo.flags |= BRIDGE_VLAN_INFO_RANGE_BEGIN;
-			} else {
-				vid = atoi(*argv);
-			}
 		} else if (strcmp(*argv, "self") == 0) {
 			flags |= BRIDGE_FLAGS_SELF;
 		} else if (strcmp(*argv, "master") == 0) {
@@ -213,7 +211,7 @@ static int vlan_modify(int cmd, int argc, char **argv)
 		argc--; argv++;
 	}
 
-	if (d == NULL || vid == -1) {
+	if (d == NULL || !vid_set) {
 		fprintf(stderr, "Device and VLAN ID are required arguments.\n");
 		return -1;
 	}
@@ -224,13 +222,8 @@ static int vlan_modify(int cmd, int argc, char **argv)
 		return -1;
 	}
 
-	if (vid < 1 || vid > 4094) {
-		fprintf(stderr, "Invalid VLAN ID \"%d\"\n", vid);
-		return -1;
-	}
-
-	if (vinfo.flags & BRIDGE_VLAN_INFO_RANGE_BEGIN) {
-		if (vid_end == -1 || vid_end >= 4096 || vid >= vid_end) {
+	if (vid_range) {
+		if (vid_end <= vid) {
 			fprintf(stderr, "Invalid VLAN range \"%hu-%hu\"\n",
 				vid, vid_end);
 			return -1;
@@ -248,10 +241,12 @@ static int vlan_modify(int cmd, int argc, char **argv)
 		addattr16(&req.n, sizeof(req), IFLA_BRIDGE_FLAGS, flags);
 
 	if (tunnel_info_set)
-		add_tunnel_info_range(&req.n, sizeof(req), vid, vid_end,
+		add_tunnel_info_range(&req.n, sizeof(req), vid,
+				      vid_range ? vid_end : -1,
 				      tun_id_start, tun_id_end);
 	else
-		add_vlan_info_range(&req.n, sizeof(req), vid, vid_end,
+		add_vlan_info_range(&req.n, sizeof(req), vid,
+				    vid_range ? vid_end : -1,
 				    vinfo.flags);
 
 	addattr_nest_end(&req.n, afspec);
@@ -277,7 +272,7 @@ static int vlan_option_set(int argc, char **argv)
 	struct bridge_vlan_info vinfo = {};
 	struct rtattr *afspec;
 	char *d = NULL;
-	short vid = -1;
+	__u16 vid = 0;
 
 	afspec = addattr_nest(&req.n, sizeof(req), BRIDGE_VLANDB_ENTRY);
 	afspec->rta_type |= NLA_F_NESTED;
@@ -293,35 +288,19 @@ static int vlan_option_set(int argc, char **argv)
 				return -1;
 			}
 		} else if (strcmp(*argv, "vid") == 0) {
-			short vid_end = -1;
-			char *p;
+			__u16 vid_end = 0;
+			bool vid_range = false;
 
 			NEXT_ARG();
-			p = strchr(*argv, '-');
-			if (p) {
-				*p = '\0';
-				p++;
-				vid = atoi(*argv);
-				vid_end = atoi(p);
-				if (vid >= vid_end || vid_end >= 4096) {
-					fprintf(stderr, "Invalid VLAN range \"%hu-%hu\"\n",
-						vid, vid_end);
-					return -1;
-				}
-			} else {
-				vid = atoi(*argv);
-			}
-			if (vid >= 4096) {
-				fprintf(stderr, "Invalid VLAN ID \"%hu\"\n",
-					vid);
-				return -1;
-			}
+			if (get_vlan_range(&vid, &vid_end, *argv))
+				invarg("invalid vlan", *argv);
+			vid_range = vid_end != vid;
 
 			vinfo.flags = BRIDGE_VLAN_INFO_ONLY_OPTS;
 			vinfo.vid = vid;
 			addattr_l(&req.n, sizeof(req), BRIDGE_VLANDB_ENTRY_INFO,
 				  &vinfo, sizeof(vinfo));
-			if (vid_end != -1)
+			if (vid_range)
 				addattr16(&req.n, sizeof(req),
 					  BRIDGE_VLANDB_ENTRY_RANGE, vid_end);
 		} else if (strcmp(*argv, "state") == 0) {
@@ -400,9 +379,9 @@ static int vlan_global_option_set(int argc, char **argv)
 		.bvm.family = PF_BRIDGE,
 	};
 	struct rtattr *afspec;
-	short vid_end = -1;
+	__u16 vid_end = 0;
 	char *d = NULL;
-	short vid = -1;
+	__u16 vid = 0;
 	__u64 val64;
 	__u32 val32;
 	__u16 val16;
@@ -422,31 +401,15 @@ static int vlan_global_option_set(int argc, char **argv)
 				return -1;
 			}
 		} else if (strcmp(*argv, "vid") == 0) {
-			char *p;
+			bool vid_range = false;
 
 			NEXT_ARG();
-			p = strchr(*argv, '-');
-			if (p) {
-				*p = '\0';
-				p++;
-				vid = atoi(*argv);
-				vid_end = atoi(p);
-				if (vid >= vid_end || vid_end >= 4096) {
-					fprintf(stderr, "Invalid VLAN range \"%hu-%hu\"\n",
-						vid, vid_end);
-					return -1;
-				}
-			} else {
-				vid = atoi(*argv);
-			}
-			if (vid >= 4096) {
-				fprintf(stderr, "Invalid VLAN ID \"%hu\"\n",
-					vid);
-				return -1;
-			}
+			if (get_vlan_range(&vid, &vid_end, *argv))
+				invarg("invalid vlan", *argv);
+			vid_range = vid_end != vid;
 			addattr16(&req.n, sizeof(req), BRIDGE_VLANDB_GOPTS_ID,
 				  vid);
-			if (vid_end != -1)
+			if (vid_range)
 				addattr16(&req.n, sizeof(req),
 					  BRIDGE_VLANDB_GOPTS_RANGE, vid_end);
 		} else if (strcmp(*argv, "mcast_snooping") == 0) {
@@ -1102,10 +1065,14 @@ static int vlan_show(int argc, char **argv, int subject)
 				duparg("dev", *argv);
 			filter_dev = *argv;
 		} else if (strcmp(*argv, "vid") == 0) {
+			__u16 vid;
+
 			NEXT_ARG();
 			if (filter_vlan)
 				duparg("vid", *argv);
-			filter_vlan = atoi(*argv);
+			if (get_vlan(&vid, *argv))
+				invarg("invalid vlan", *argv);
+			filter_vlan = vid;
 		}
 		argc--; argv++;
 	}
@@ -1219,10 +1186,14 @@ static int vlan_global_show(int argc, char **argv)
 				duparg("dev", *argv);
 			filter_dev = *argv;
 		} else if (strcmp(*argv, "vid") == 0) {
+			__u16 vid;
+
 			NEXT_ARG();
 			if (filter_vlan)
 				duparg("vid", *argv);
-			filter_vlan = atoi(*argv);
+			if (get_vlan(&vid, *argv))
+				invarg("invalid vlan", *argv);
+			filter_vlan = vid;
 		}
 		argc--; argv++;
 	}
diff --git a/include/utils.h b/include/utils.h
index e4e318e2..06d5242f 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -151,6 +151,8 @@ int get_u32(__u32 *val, const char *arg, int base);
 int get_s32(__s32 *val, const char *arg, int base);
 int get_u16(__u16 *val, const char *arg, int base);
 int get_u8(__u8 *val, const char *arg, int base);
+int get_vlan(__u16 *val, const char *arg);
+int get_vlan_range(__u16 *start, __u16 *end, const char *arg);
 int get_be64(__be64 *val, const char *arg, int base);
 int get_be32(__be32 *val, const char *arg, int base);
 int get_be16(__be16 *val, const char *arg, int base);
diff --git a/lib/utils.c b/lib/utils.c
index 1215fe31..debba751 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -387,6 +387,57 @@ int get_u16(__u16 *val, const char *arg, int base)
 	return 0;
 }
 
+int get_vlan(__u16 *val, const char *arg)
+{
+	if (get_u16(val, arg, 10))
+		return -1;
+
+	if (*val < 1 || *val > 4094)
+		return -1;
+
+	return 0;
+}
+
+int get_vlan_range(__u16 *start, __u16 *end, const char *arg)
+{
+	char *start_arg;
+	const char *sep;
+	size_t len;
+	int ret;
+
+	if (!arg)
+		return -1;
+
+	sep = strchr(arg, '-');
+	if (!sep) {
+		ret = get_vlan(start, arg);
+		if (!ret)
+			*end = *start;
+		return ret;
+	}
+
+	len = sep - arg;
+	if (!len || !sep[1])
+		return -1;
+
+	start_arg = malloc(len + 1);
+	if (!start_arg)
+		return -1;
+
+	memcpy(start_arg, arg, len);
+	start_arg[len] = '\0';
+
+	ret = get_vlan(start, start_arg) || get_vlan(end, sep + 1);
+	free(start_arg);
+	if (ret)
+		return -1;
+
+	if (*start >= *end)
+		return -1;
+
+	return 0;
+}
+
 int get_u8(__u8 *val, const char *arg, int base)
 {
 	unsigned long res;
-- 
2.39.3 (Apple Git-145)

Re: [PATCH] bridge: tighten VLAN parsing

[Stephen Hemminger]

On Fri, 31 Jul 2026 22:40:51 -0400
William Gonzalez <gonzalez.williamalexander1@gmail.com> wrote:

> ---

It is good to see more input validation but this patch fails the basic
requirements of iproute2 submissions.

1. No Signed-off-by
2. No commit message
3. No explanation

Longer AI generated review.

Subject: Re: [PATCH] bridge: tighten VLAN parsing

On Fri, 31 Jul 2026 22:40:51 -0400, William Gonzalez wrote:

This patch cannot be accepted in its current form due to missing required elements.

## BLOCKING ISSUES

### 1. Missing Commit Message (REQUIRED)

The patch has no commit description at all. This is unacceptable for iproute2.
Every patch MUST have:
- A clear subject line
- A description of what problem is being solved
- An explanation of the approach taken
- Examples of before/after behavior if applicable
- Signed-off-by line (DCO compliance)

### 2. Missing DCO Sign-off

The patch lacks a Signed-off-by line, which is mandatory for DCO (Developer
Certificate of Origin) compliance. All patches must include:

```
Signed-off-by: William Gonzalez <gonzalez.williamalexander1@gmail.com>
```

This certifies that you have the right to submit the patch under the GPL-2.0+
license and that you agree to the Developer Certificate of Origin.

## Technical Review

The technical changes (replacing atoi() with validated parsing) are good, but
there are issues that need addressing:

### VLAN 0 Handling Issue

The get_vlan() function rejects VLAN ID 0:

```c
if (*val < 1 || *val > 4094)
	return -1;
```

VLAN ID 0 is valid for 802.1Q priority-tagged frames. The patch needs to
either:
1. Document why VLAN 0 is rejected (if intentional)
2. Support VLAN 0 where appropriate
3. Have different validators for different contexts

### Memory Allocation

Using malloc() for parsing 4-digit numbers is unnecessary:

```c
start_arg = malloc(len + 1);
```

Better alternatives:
1. Use `strdupa()` to avoid malloc/free handling:
```c
char *start_arg = strdupa(arg);
start_arg[len] = '\0';
```

2. Or use a fixed stack buffer:
```c
char start_arg[8];  /* max "4094" plus null */
```

The `strdupa()` approach is cleaner as it avoids both malloc failure handling
and fixed buffer size checks.

### Range Validation Change

The patch changes behavior for ranges like "100-100" (single VLAN as range).
This needs to be documented or fixed.

## Required for Resubmission

1. **Add proper commit message** explaining:
   - What problem this solves
   - Why atoi() is inadequate
   - What the new validation provides
   - Any behavior changes

2. **Add Signed-off-by line** for DCO compliance

3. **Address VLAN 0 handling** - either support it or document why not

4. **Remove unnecessary malloc()** - use stack buffer

5. **Clarify range validation** changes

Example of acceptable commit message:

```
bridge: tighten VLAN parsing

The current VLAN ID parsing uses atoi() which silently accepts
invalid input like "123abc" and treats it as 123. This can cause
user confusion and unexpected behavior.

Replace atoi() with get_vlan() helper that:
- Validates the entire string is numeric
- Ensures VLAN ID is in valid range (1-4094)
- Provides clear error messages for invalid input

Note: VLAN 0 is not accepted as [explain rationale].

Before: bridge fdb add ... vlan 123x  (silently uses 123)
After:  bridge fdb add ... vlan 123x  (error: invalid vlan)

Signed-off-by: William Gonzalez <gonzalez.williamalexander1@gmail.com>
```

Please resubmit with these required elements. The core idea is good but the
patch needs proper documentation and a few technical adjustments.

NACK - missing commit message and DCO sign-off