Index | Thread | Search

From:
Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject:
bgpd: rework pt_fill and friends
To:
tech@openbsd.org
Date:
Wed, 13 May 2026 16:15:40 +0200

Download raw body.

Thread
pt_fill() is in some cases used by semi-trusted content (e.g. from
bgpctl). The fatalx calls in that function are therefor a problem.

This alters pt_fill to instead return a pt_entry object that can not
exist. I decided to return a pt_entry initalised with 0xff.
Also if the prefixlen is too big for the address family just clip it down
to the maximum (with a log message).

In pt_add(), the only place a pt_fill() object would be added to the tree,
check if the returned object is valid. There it is ok to fatal (at least
for now) since the code previous to pt_add() should validate the prefix.

On top of this I uniformed some of the error messages and switched the
prefixlen argument to u_int (there is no negative prefix length).

-- 
:wq Claudio

Index: rde.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
diff -u -p -r1.347 rde.h
--- rde.h	7 May 2026 11:21:24 -0000	1.347
+++ rde.h	13 May 2026 09:39:38 -0000
@@ -582,10 +582,10 @@ void	 pt_init(void);
 void	 pt_shutdown(void);
 void	 pt_getaddr(struct pt_entry *, struct bgpd_addr *);
 int	 pt_getflowspec(struct pt_entry *, uint8_t **);
-struct pt_entry	*pt_fill(struct bgpd_addr *, int);
-struct pt_entry	*pt_get(struct bgpd_addr *, int);
-struct pt_entry	*pt_get_next(struct bgpd_addr *, int);
-struct pt_entry	*pt_add(struct bgpd_addr *, int);
+struct pt_entry	*pt_fill(struct bgpd_addr *, u_int);
+struct pt_entry	*pt_get(struct bgpd_addr *, u_int);
+struct pt_entry	*pt_get_next(struct bgpd_addr *, u_int);
+struct pt_entry	*pt_add(struct bgpd_addr *, u_int);
 struct pt_entry	*pt_get_flow(struct flowspec *);
 struct pt_entry	*pt_add_flow(struct flowspec *);
 struct pt_entry	*pt_first(uint8_t);
Index: rde_prefix.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_prefix.c,v
diff -u -p -r1.60 rde_prefix.c
--- rde_prefix.c	24 Dec 2025 07:59:55 -0000	1.60
+++ rde_prefix.c	13 May 2026 11:53:23 -0000
@@ -46,7 +46,7 @@
  */
 
 /* internal prototypes */
-static struct pt_entry	*pt_alloc(struct pt_entry *, int len);
+static struct pt_entry	*pt_alloc(struct pt_entry *, size_t len);
 static void		 pt_free(struct pt_entry *);
 
 struct pt_entry4 {
@@ -223,7 +223,7 @@ pt_getaddr(struct pt_entry *pte, struct 
 		    addr, &pflow->prefixlen, NULL);
 		break;
 	default:
-		fatalx("pt_getaddr: unknown af");
+		fatalx("%s: unknown aid %d", __func__, pte->aid);
 	}
 }
 
@@ -239,12 +239,16 @@ pt_getflowspec(struct pt_entry *pte, uin
 		*flow = pflow->flow;
 		return pflow->len - PT_FLOW_SIZE;
 	default:
-		fatalx("pt_getflowspec: unknown af");
+		fatalx("%s: unknown aid %d", __func__, pte->aid);
 	}
 }
 
+/*
+ * Fill out a pt_entry for lookup, on failure return an object initalized
+ * with 0xff. pt_add must reject such objects (by checking pte->aid).
+ */
 struct pt_entry *
-pt_fill(struct bgpd_addr *prefix, int prefixlen)
+pt_fill(struct bgpd_addr *prefix, u_int prefixlen)
 {
 	static struct pt_entry4		pte4;
 	static struct pt_entry6		pte6;
@@ -258,8 +262,10 @@ pt_fill(struct bgpd_addr *prefix, int pr
 		pte4.len = sizeof(pte4);
 		pte4.refcnt = UINT32_MAX;
 		pte4.aid = prefix->aid;
-		if (prefixlen > 32)
-			fatalx("pt_fill: bad IPv4 prefixlen");
+		if (prefixlen > 32) {
+			log_warnx("pt_fill: %d, bad IPv4 prefixlen", prefixlen);
+			prefixlen = 32;
+		}
 		inet4applymask(&pte4.prefix4, &prefix->v4, prefixlen);
 		pte4.prefixlen = prefixlen;
 		return ((struct pt_entry *)&pte4);
@@ -268,8 +274,10 @@ pt_fill(struct bgpd_addr *prefix, int pr
 		pte6.len = sizeof(pte6);
 		pte6.refcnt = UINT32_MAX;
 		pte6.aid = prefix->aid;
-		if (prefixlen > 128)
-			fatalx("pt_fill: bad IPv6 prefixlen");
+		if (prefixlen > 128) {
+			log_warnx("pt_fill: %d, bad IPv6 prefixlen", prefixlen);
+			prefixlen = 128;
+		}
 		inet6applymask(&pte6.prefix6, &prefix->v6, prefixlen);
 		pte6.prefixlen = prefixlen;
 		return ((struct pt_entry *)&pte6);
@@ -278,8 +286,10 @@ pt_fill(struct bgpd_addr *prefix, int pr
 		pte_vpn4.len = sizeof(pte_vpn4);
 		pte_vpn4.refcnt = UINT32_MAX;
 		pte_vpn4.aid = prefix->aid;
-		if (prefixlen > 32)
-			fatalx("pt_fill: bad IPv4 prefixlen");
+		if (prefixlen > 32) {
+			log_warnx("pt_fill: %d, bad IPv4 prefixlen", prefixlen);
+			prefixlen = 32;
+		}
 		inet4applymask(&pte_vpn4.prefix4, &prefix->v4, prefixlen);
 		pte_vpn4.prefixlen = prefixlen;
 		pte_vpn4.rd = prefix->rd;
@@ -292,8 +302,10 @@ pt_fill(struct bgpd_addr *prefix, int pr
 		pte_vpn6.len = sizeof(pte_vpn6);
 		pte_vpn6.refcnt = UINT32_MAX;
 		pte_vpn6.aid = prefix->aid;
-		if (prefixlen > 128)
-			fatalx("pt_fill: bad IPv6 prefixlen");
+		if (prefixlen > 128) {
+			log_warnx("pt_fill: %d, bad IPv6 prefixlen", prefixlen);
+			prefixlen = 128;
+		}
 		inet6applymask(&pte_vpn6.prefix6, &prefix->v6, prefixlen);
 		pte_vpn6.prefixlen = prefixlen;
 		pte_vpn6.rd = prefix->rd;
@@ -310,13 +322,24 @@ pt_fill(struct bgpd_addr *prefix, int pr
 			/* See rfc7432 section 7.2 */
 			break;
 		case AID_INET:
+			if (prefixlen > 32) {
+				log_warnx("pt_fill: %d, bad IPv4 prefixlen",
+				    prefixlen);
+				prefixlen = 32;
+			}
 			pte_evpn.prefix4 = prefix->evpn.v4;
 			break;
 		case AID_INET6:
+			if (prefixlen > 128) {
+				log_warnx("pt_fill: %d, bad IPv6 prefixlen",
+				    prefixlen);
+				prefixlen = 128;
+			}
 			pte_evpn.prefix6 = prefix->evpn.v6;
 			break;
 		default:
-			fatalx("pt_fill: bad EVPN prefixlen");
+			log_warnx("pt_fill: unknown EVPN prefix");
+			goto fail;
 		}
 		pte_evpn.aid = prefix->aid;
 		pte_evpn.vpnaid = prefix->evpn.aid;
@@ -333,12 +356,17 @@ pt_fill(struct bgpd_addr *prefix, int pr
 		    sizeof(prefix->evpn.mac));
 		return ((struct pt_entry *)&pte_evpn);
 	default:
-		fatalx("pt_fill: unknown af");
+		log_warnx("%s: unknown aid %d", __func__, prefix->aid);
+		goto fail;
 	}
+
+ fail:
+	memset(&pte4, 0xff, sizeof(pte4));
+	return ((struct pt_entry *)&pte4);
 }
 
 struct pt_entry *
-pt_get(struct bgpd_addr *prefix, int prefixlen)
+pt_get(struct bgpd_addr *prefix, u_int prefixlen)
 {
 	struct pt_entry	*pte;
 
@@ -347,7 +375,7 @@ pt_get(struct bgpd_addr *prefix, int pre
 }
 
 struct pt_entry *
-pt_get_next(struct bgpd_addr *prefix, int prefixlen)
+pt_get_next(struct bgpd_addr *prefix, u_int prefixlen)
 {
 	struct pt_entry	*pte;
 
@@ -356,11 +384,13 @@ pt_get_next(struct bgpd_addr *prefix, in
 }
 
 struct pt_entry *
-pt_add(struct bgpd_addr *prefix, int prefixlen)
+pt_add(struct bgpd_addr *prefix, u_int prefixlen)
 {
 	struct pt_entry		*p = NULL;
 
 	p = pt_fill(prefix, prefixlen);
+	if (p->aid == 0xff)
+		fatalx("pt_add: insert failed, pt_fill failed");
 	p = pt_alloc(p, p->len);
 
 	if (RB_INSERT(pt_tree, &pttable, p) != NULL)
@@ -457,7 +487,8 @@ pt_lookup(struct bgpd_addr *addr)
 		i = 128;
 		break;
 	default:
-		fatalx("pt_lookup: unknown af");
+		log_warnx("%s: unknown aid %d", __func__, addr->aid);
+		return (NULL);
 	}
 	for (; i >= 0; i--) {
 		p = pt_get(addr, i);
@@ -582,7 +613,7 @@ pt_prefix_cmp(const struct pt_entry *a, 
 				return (-1);
 			break;
 		default:
-			fatalx("pt_prefix_cmp: unknown evpn af %d", ea->vpnaid);
+			fatalx("%s: unknown evpn aid %d", __func__, ea->vpnaid);
 		}
 		return (0);
 	case AID_FLOWSPECv4:
@@ -593,7 +624,7 @@ pt_prefix_cmp(const struct pt_entry *a, 
 		    bf->flow, bf->len - PT_FLOW_SIZE,
 		    a->aid == AID_FLOWSPECv6);
 	default:
-		fatalx("pt_prefix_cmp: unknown af %d", a->aid);
+		fatalx("%s: unknown aid %d", __func__, a->aid);
 	}
 	return (-1);
 }
@@ -603,7 +634,7 @@ pt_prefix_cmp(const struct pt_entry *a, 
  * Function may not return on failure.
  */
 static struct pt_entry *
-pt_alloc(struct pt_entry *op, int len)
+pt_alloc(struct pt_entry *op, size_t len)
 {
 	struct pt_entry		*p;