Index | Thread | Search

From:
Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject:
bgpd rewrite rde update parser (step 1)
To:
tech@openbsd.org
Date:
Mon, 22 Jan 2024 15:15:38 +0100

Download raw body.

Thread
I made all those ibuf and imsg API changes to improve the parser in bgpd.
Mainly rde_update_dispatch() and all the function it calls. While the code
is careful some errors snuck in (e.g. because of type overflows).

This should all be prevented by using the ibuf API since the handling
becomes simpler.

Now this is a deep rabbit hole and the overall bgpd change I have is over
5000 lines long. To make review somewhat simpler I tried to split the diff
into chunks by introducing some hacks here and there to make progress
without rewriting everything.

This diff just rewrite rde_update_dispatch() to use ibufs. Because of this
rde_update_err(), rde_get_mp_nexthop(), nlri_get_prefix, and friends are
switched to use ibufs. For rde_attr_parse() an absolute minimum change was
done.

The problem is that bgpctl() uses nlri_get_prefix() itself and so some
code in there had to be adjusted. I kept the changes to a minimum for now
and sprinkled some XXX in places where I know I sinned. Those will be
cleaned up in the near future by pulling in more pending changes.

-- 
:wq Claudio

Index: usr.sbin/bgpctl/bgpctl.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
diff -u -p -r1.300 bgpctl.c
--- usr.sbin/bgpctl/bgpctl.c	18 Jan 2024 14:46:21 -0000	1.300
+++ usr.sbin/bgpctl/bgpctl.c	22 Jan 2024 13:27:56 -0000
@@ -1709,116 +1709,82 @@ static void
 show_mrt_update(u_char *p, uint16_t len, int reqflags, int addpath)
 {
 	struct bgpd_addr prefix;
-	int pos;
+	struct ibuf *b, buf, wbuf, abuf;
 	uint32_t pathid;
 	uint16_t wlen, alen;
 	uint8_t prefixlen;
 
-	if (len < sizeof(wlen)) {
-		printf("bad length");
-		return;
-	}
-	memcpy(&wlen, p, sizeof(wlen));
-	wlen = ntohs(wlen);
-	p += sizeof(wlen);
-	len -= sizeof(wlen);
-
-	if (len < wlen) {
-		printf("bad withdraw length");
+	ibuf_from_buffer(&buf, p, len);
+	b = &buf;
+	if (ibuf_get_n16(b, &wlen) == -1 ||
+	    ibuf_get_ibuf(b, wlen, &wbuf) == -1) {
+ trunc:
+		printf("truncated message");
 		return;
 	}
 	if (wlen > 0) {
 		printf("\n     Withdrawn prefixes:");
-		while (wlen > 0) {
-			if (addpath) {
-				if (wlen <= sizeof(pathid)) {
-					printf("bad withdraw prefix");
-					return;
-				}
-				memcpy(&pathid, p, sizeof(pathid));
-				pathid = ntohl(pathid);
-				p += sizeof(pathid);
-				len -= sizeof(pathid);
-				wlen -= sizeof(pathid);
-			}
-			if ((pos = nlri_get_prefix(p, wlen, &prefix,
-			    &prefixlen)) == -1) {
-				printf("bad withdraw prefix");
-				return;
-			}
+		while (ibuf_size(&wbuf) > 0) {
+			if (addpath)
+				if (ibuf_get_n32(&wbuf, &pathid) == -1)
+					goto trunc;
+			if (nlri_get_prefix(&wbuf, &prefix, &prefixlen) == -1)
+				goto trunc;
+
 			printf(" %s/%u", log_addr(&prefix), prefixlen);
 			if (addpath)
 				printf(" path-id %u", pathid);
-			p += pos;
-			len -= pos;
-			wlen -= pos;
 		}
 	}
 
-	if (len < sizeof(alen)) {
-		printf("bad length");
-		return;
-	}
-	memcpy(&alen, p, sizeof(alen));
-	alen = ntohs(alen);
-	p += sizeof(alen);
-	len -= sizeof(alen);
+	if (ibuf_get_n16(b, &alen) == -1 ||
+	    ibuf_get_ibuf(b, alen, &abuf) == -1)
+		goto trunc;
 
-	if (len < alen) {
-		printf("bad attribute length");
-		return;
-	}
 	printf("\n");
 	/* alen attributes here */
-	while (alen > 3) {
-		uint8_t flags;
+	while (ibuf_size(&abuf) > 0) {
+		struct ibuf attrbuf;
 		uint16_t attrlen;
+		uint8_t flags;
 
-		flags = p[0];
-		/* type = p[1]; */
+		ibuf_from_ibuf(&abuf, &attrbuf);
+		if (ibuf_get_n8(&attrbuf, &flags) == -1 ||
+		    ibuf_skip(&attrbuf, 1) == -1)
+			goto trunc;
 
 		/* get the attribute length */
 		if (flags & ATTR_EXTLEN) {
-			if (len < sizeof(attrlen) + 2)
-				printf("bad attribute length");
-			memcpy(&attrlen, &p[2], sizeof(attrlen));
-			attrlen = ntohs(attrlen);
-			attrlen += sizeof(attrlen) + 2;
+			if (ibuf_get_n16(&attrbuf, &attrlen) == -1)
+				goto trunc;
 		} else {
-			attrlen = p[2];
-			attrlen += 1 + 2;
+			uint8_t tmp;
+			if (ibuf_get_n8(&attrbuf, &tmp) == -1)
+				goto trunc;
+			attrlen = tmp;
 		}
+		if (ibuf_truncate(&attrbuf, attrlen) == -1)
+			goto trunc;
+		ibuf_rewind(&attrbuf);
+		if (ibuf_skip(&abuf, ibuf_size(&attrbuf)) == -1)
+			goto trunc;
 
-		output->attr(p, attrlen, reqflags, addpath);
-		p += attrlen;
-		alen -= attrlen;
-		len -= attrlen;
+		output->attr(ibuf_data(&attrbuf), ibuf_size(&attrbuf),
+		    reqflags, addpath);
 	}
 
-	if (len > 0) {
+	if (ibuf_size(b) > 0) {
 		printf("    NLRI prefixes:");
-		while (len > 0) {
-			if (addpath) {
-				if (len <= sizeof(pathid)) {
-					printf(" bad nlri prefix: pathid, "
-					    "len %d", len);
-					return;
-				}
-				memcpy(&pathid, p, sizeof(pathid));
-				pathid = ntohl(pathid);
-				p += sizeof(pathid);
-				len -= sizeof(pathid);
-			}
-			if ((pos = nlri_get_prefix(p, len, &prefix,
-			    &prefixlen)) == -1) {
-				printf(" bad nlri prefix");
-				return;
-			}
+		while (ibuf_size(b) > 0) {
+			if (addpath)
+				if (ibuf_get_n32(b, &pathid) == -1)
+					goto trunc;
+			if (nlri_get_prefix(b, &prefix, &prefixlen) == -1)
+				goto trunc;
+
 			printf(" %s/%u", log_addr(&prefix), prefixlen);
 			if (addpath)
 				printf(" path-id %u", pathid);
-			p += pos;
-			len -= pos;
 		}
 	}
 }
Index: usr.sbin/bgpctl/mrtparser.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpctl/mrtparser.c,v
diff -u -p -r1.20 mrtparser.c
--- usr.sbin/bgpctl/mrtparser.c	20 Nov 2023 14:18:21 -0000	1.20
+++ usr.sbin/bgpctl/mrtparser.c	22 Jan 2024 13:57:52 -0000
@@ -398,7 +398,7 @@ mrt_parse_v2_rib(struct mrt_hdr *hdr, vo
 		/* prefix */
 		ret = mrt_extract_prefix(b, len, AID_INET, &r->prefix,
 		    &r->prefixlen, verbose);
-		if (ret == 1)
+		if (ret == -1)
 			goto fail;
 		break;
 	case MRT_DUMP_V2_RIB_IPV6_UNICAST_ADDPATH:
@@ -410,7 +410,7 @@ mrt_parse_v2_rib(struct mrt_hdr *hdr, vo
 		/* prefix */
 		ret = mrt_extract_prefix(b, len, AID_INET6, &r->prefix,
 		    &r->prefixlen, verbose);
-		if (ret == 1)
+		if (ret == -1)
 			goto fail;
 		break;
 	case MRT_DUMP_V2_RIB_GENERIC_ADDPATH:
@@ -439,7 +439,7 @@ mrt_parse_v2_rib(struct mrt_hdr *hdr, vo
 		/* prefix */
 		ret = mrt_extract_prefix(b, len, aid, &r->prefix,
 		    &r->prefixlen, verbose);
-		if (ret == 1)
+		if (ret == -1)
 			goto fail;
 		break;
 	default:
@@ -744,7 +744,7 @@ mrt_parse_dump_mp(struct mrt_hdr *hdr, v
 	/* prefix */
 	ret = mrt_extract_prefix(b, len, aid, &r->prefix, &r->prefixlen,
 	    verbose);
-	if (ret == 1)
+	if (ret == -1)
 		goto fail;
 	b += ret;
 	len -= ret;
@@ -1032,23 +1032,25 @@ mrt_extract_addr(void *msg, u_int len, s
 }
 
 int
-mrt_extract_prefix(void *msg, u_int len, uint8_t aid,
+mrt_extract_prefix(void *m, u_int len, uint8_t aid,
     struct bgpd_addr *prefix, uint8_t *prefixlen, int verbose)
 {
+	struct ibuf buf, *msg = &buf;
 	int r;
 
+	ibuf_from_buffer(msg, m, len); /* XXX */
 	switch (aid) {
 	case AID_INET:
-		r = nlri_get_prefix(msg, len, prefix, prefixlen);
+		r = nlri_get_prefix(msg, prefix, prefixlen);
 		break;
 	case AID_INET6:
-		r = nlri_get_prefix6(msg, len, prefix, prefixlen);
+		r = nlri_get_prefix6(msg, prefix, prefixlen);
 		break;
 	case AID_VPN_IPv4:
-		r = nlri_get_vpn4(msg, len, prefix, prefixlen, 0);
+		r = nlri_get_vpn4(msg, prefix, prefixlen, 0);
 		break;
 	case AID_VPN_IPv6:
-		r = nlri_get_vpn6(msg, len, prefix, prefixlen, 0);
+		r = nlri_get_vpn6(msg, prefix, prefixlen, 0);
 		break;
 	default:
 		if (verbose)
@@ -1057,6 +1059,7 @@ mrt_extract_prefix(void *msg, u_int len,
 	}
 	if (r == -1 && verbose)
 		printf("failed to parse prefix of AID %d\n", aid);
+	r = len - ibuf_size(msg); /* XXX */
 	return r;
 }
 
Index: usr.sbin/bgpctl/output.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpctl/output.c,v
diff -u -p -r1.46 output.c
--- usr.sbin/bgpctl/output.c	11 Jan 2024 14:34:49 -0000	1.46
+++ usr.sbin/bgpctl/output.c	22 Jan 2024 13:34:15 -0000
@@ -772,11 +772,12 @@ show_attr(u_char *data, size_t len, int 
 	u_char		*path;
 	struct in_addr	 id;
 	struct bgpd_addr prefix;
+	struct ibuf	 ibuf, *buf = &ibuf;
 	char		*aspath;
 	uint32_t	 as, pathid;
 	uint16_t	 alen, ioff, short_as, afi;
 	uint8_t		 flags, type, safi, aid, prefixlen;
-	int		 i, pos, e2, e4;
+	int		 i, e2, e4;
 
 	if (len < 3) {
 		warnx("Too short BGP attribute");
@@ -951,43 +952,35 @@ show_attr(u_char *data, size_t len, int 
 			printf(" nexthop: %s", log_addr(&nexthop));
 		}
 
-		while (alen > 0) {
-			if (addpath) {
-				if (alen <= sizeof(pathid)) {
-					printf("bad nlri prefix");
-					return;
-				}
-				memcpy(&pathid, data, sizeof(pathid));
-				pathid = ntohl(pathid);
-				data += sizeof(pathid);
-				alen -= sizeof(pathid);
-			}
+		ibuf_from_buffer(buf, data, alen);
+
+		while (ibuf_size(buf) > 0) {
+			if (addpath)
+				if (ibuf_get_n32(buf, &pathid) == -1)
+					goto bad_len;
 			switch (aid) {
 			case AID_INET6:
-				pos = nlri_get_prefix6(data, alen, &prefix,
-				    &prefixlen);
+				if (nlri_get_prefix6(buf, &prefix,
+				    &prefixlen) == -1)
+					goto bad_len;
 				break;
 			case AID_VPN_IPv4:
-				pos = nlri_get_vpn4(data, alen, &prefix,
-				    &prefixlen, 1);
+				if (nlri_get_vpn4(buf, &prefix,
+				    &prefixlen, 1) == -1)
+					goto bad_len;
 				break;
 			case AID_VPN_IPv6:
-				pos = nlri_get_vpn6(data, alen, &prefix,
-				    &prefixlen, 1);
+				if (nlri_get_vpn6(buf, &prefix,
+				    &prefixlen, 1) == -1)
+					goto bad_len;
 				break;
 			default:
 				printf("unhandled AID #%u", aid);
 				goto done;
 			}
-			if (pos == -1) {
-				printf("bad %s prefix", aid2str(aid));
-				break;
-			}
 			printf(" %s/%u", log_addr(&prefix), prefixlen);
 			if (addpath)
 				printf(" path-id %u", pathid);
-			data += pos;
-			alen -= pos;
 		}
 		break;
 	case ATTR_EXT_COMMUNITIES:
Index: usr.sbin/bgpctl/output_json.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpctl/output_json.c,v
diff -u -p -r1.38 output_json.c
--- usr.sbin/bgpctl/output_json.c	11 Jan 2024 13:09:41 -0000	1.38
+++ usr.sbin/bgpctl/output_json.c	22 Jan 2024 13:49:42 -0000
@@ -589,12 +589,13 @@ json_attr(u_char *data, size_t len, int 
 {
 	struct bgpd_addr prefix;
 	struct in_addr id;
+	struct ibuf ibuf, *buf = &ibuf;
 	char *aspath;
 	u_char *path;
 	uint32_t as, pathid;
 	uint16_t alen, afi, off, short_as;
 	uint8_t flags, type, safi, aid, prefixlen;
-	int e4, e2, pos;
+	int e4, e2;
 
 	if (len < 3) {
 		warnx("Too short BGP attribute");
@@ -780,48 +781,39 @@ bad_len:
 			json_do_string("nexthop", log_addr(&nexthop));
 		}
 
+		ibuf_from_buffer(buf, data, alen);
+
 		json_do_array("NLRI");
-		while (alen > 0) {
+		while (ibuf_size(buf) > 0) {
 			json_do_object("prefix", 1);
-			if (addpath) {
-				if (alen <= sizeof(pathid)) {
-					json_do_string("error", "bad path-id");
-					break;
-				}
-				memcpy(&pathid, data, sizeof(pathid));
-				pathid = ntohl(pathid);
-				data += sizeof(pathid);
-				alen -= sizeof(pathid);
-			}
+			if (addpath)
+				if (ibuf_get_n32(buf, &pathid) == -1)
+					goto bad_len;
 			switch (aid) {
 			case AID_INET6:
-				pos = nlri_get_prefix6(data, alen, &prefix,
-				    &prefixlen);
+				if (nlri_get_prefix6(buf, &prefix,
+				    &prefixlen) == -1)
+					goto bad_len;
 				break;
 			case AID_VPN_IPv4:
-				 pos = nlri_get_vpn4(data, alen, &prefix,
-				     &prefixlen, 1);
+				if (nlri_get_vpn4(buf, &prefix,
+				    &prefixlen, 1) == -1)
+					goto bad_len;
 				 break;
 			case AID_VPN_IPv6:
-				 pos = nlri_get_vpn6(data, alen, &prefix,
-				     &prefixlen, 1);
+				if (nlri_get_vpn6(buf, &prefix,
+				    &prefixlen, 1) == -1)
+					goto bad_len;
 				 break;
 			default:
 				json_do_printf("error", "unhandled AID: %d",
 				    aid);
 				return;
 			}
-			if (pos == -1) {
-				json_do_printf("error", "bad %s prefix",
-				    aid2str(aid));
-				break;
-			}
 			json_do_printf("prefix", "%s/%u", log_addr(&prefix),
 			    prefixlen);
 			if (addpath)
 				 json_do_uint("path_id", pathid);
-			data += pos;
-			alen -= pos;
 			json_do_end();
 		}
 		json_do_end();
Index: usr.sbin/bgpd/bgpd.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
diff -u -p -r1.481 bgpd.h
--- usr.sbin/bgpd/bgpd.h	11 Jan 2024 13:08:39 -0000	1.481
+++ usr.sbin/bgpd/bgpd.h	22 Jan 2024 11:26:47 -0000
@@ -1557,14 +1557,12 @@ int		 aspath_verify(void *, uint16_t, in
 #define		 AS_ERR_SOFT	-4
 u_char		*aspath_inflate(void *, uint16_t, uint16_t *);
 int		 extract_prefix(const u_char *, int, void *, uint8_t, uint8_t);
-int		 nlri_get_prefix(u_char *, uint16_t, struct bgpd_addr *,
-		    uint8_t *);
-int		 nlri_get_prefix6(u_char *, uint16_t, struct bgpd_addr *,
-		    uint8_t *);
-int		 nlri_get_vpn4(u_char *, uint16_t, struct bgpd_addr *,
-		    uint8_t *, int);
-int		 nlri_get_vpn6(u_char *, uint16_t, struct bgpd_addr *,
-		    uint8_t *, int);
+int		 nlri_get_prefix(struct ibuf *, struct bgpd_addr *, uint8_t *);
+int		 nlri_get_prefix6(struct ibuf *, struct bgpd_addr *, uint8_t *);
+int		 nlri_get_vpn4(struct ibuf *, struct bgpd_addr *, uint8_t *,
+		    int);
+int		 nlri_get_vpn6(struct ibuf *, struct bgpd_addr *, uint8_t *,
+		    int);
 int		 prefix_compare(const struct bgpd_addr *,
 		    const struct bgpd_addr *, int);
 void		 inet4applymask(struct in_addr *, const struct in_addr *, int);
Index: usr.sbin/bgpd/rde.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
diff -u -p -r1.614 rde.c
--- usr.sbin/bgpd/rde.c	15 Jan 2024 15:44:50 -0000	1.614
+++ usr.sbin/bgpd/rde.c	22 Jan 2024 13:13:44 -0000
@@ -49,16 +49,16 @@ void		 rde_dispatch_imsg_session(struct 
 void		 rde_dispatch_imsg_parent(struct imsgbuf *);
 void		 rde_dispatch_imsg_rtr(struct imsgbuf *);
 void		 rde_dispatch_imsg_peer(struct rde_peer *, void *);
-void		 rde_update_dispatch(struct rde_peer *, struct imsg *);
+void		 rde_update_dispatch(struct rde_peer *, struct ibuf *);
 int		 rde_update_update(struct rde_peer *, uint32_t,
 		    struct filterstate *, struct bgpd_addr *, uint8_t);
 void		 rde_update_withdraw(struct rde_peer *, uint32_t,
 		    struct bgpd_addr *, uint8_t);
-int		 rde_attr_parse(u_char *, uint16_t, struct rde_peer *,
-		    struct filterstate *, struct mpattr *);
+int		 rde_attr_parse(struct ibuf *, struct rde_peer *,
+		    struct filterstate *, struct ibuf *, struct ibuf *);
 int		 rde_attr_add(struct filterstate *, struct ibuf *);
 uint8_t		 rde_attr_missing(struct rde_aspath *, int, uint16_t);
-int		 rde_get_mp_nexthop(u_char *, uint16_t, uint8_t,
+int		 rde_get_mp_nexthop(struct ibuf *, uint8_t,
 		    struct rde_peer *, struct filterstate *);
 void		 rde_as4byte_fixup(struct rde_peer *, struct rde_aspath *);
 uint8_t		 rde_aspa_validity(struct rde_peer *, struct rde_aspath *,
@@ -1301,6 +1301,7 @@ rde_dispatch_imsg_peer(struct rde_peer *
 {
 	struct route_refresh rr;
 	struct imsg imsg;
+	struct ibuf ibuf;
 
 	if (!peer_imsg_pop(peer, &imsg))
 		return;
@@ -1309,7 +1310,10 @@ rde_dispatch_imsg_peer(struct rde_peer *
 	case IMSG_UPDATE:
 		if (peer->state != PEER_UP)
 			break;
-		rde_update_dispatch(peer, &imsg);
+		if (imsg_get_ibuf(&imsg, &ibuf) == -1)
+			log_warn("update: bad imsg");
+		else
+			rde_update_dispatch(peer, &ibuf);
 		break;
 	case IMSG_REFRESH:
 		if (imsg_get_data(&imsg, &rr, sizeof(rr)) == -1) {
@@ -1368,80 +1372,57 @@ rde_dispatch_imsg_peer(struct rde_peer *
 
 /* handle routing updates from the session engine. */
 void
-rde_update_dispatch(struct rde_peer *peer, struct imsg *imsg)
+rde_update_dispatch(struct rde_peer *peer, struct ibuf *buf)
 {
 	struct filterstate	 state;
 	struct bgpd_addr	 prefix;
-	struct mpattr		 mpa;
-	u_char			*p, *mpp = NULL;
-	int			 pos = 0;
-	uint16_t		 afi, len, mplen;
-	uint16_t		 withdrawn_len;
-	uint16_t		 attrpath_len;
-	uint16_t		 nlri_len;
+	struct ibuf		 wdbuf, attrbuf, nlribuf, reachbuf, unreachbuf;
+	uint16_t		 afi, len;
 	uint8_t			 aid, prefixlen, safi, subtype;
 	uint32_t		 fas, pathid;
 
-	p = imsg->data;
-
-	if (imsg->hdr.len < IMSG_HEADER_SIZE + 2) {
-		rde_update_err(peer, ERR_UPDATE, ERR_UPD_ATTRLIST, NULL, 0);
-		return;
-	}
-
-	memcpy(&len, p, 2);
-	withdrawn_len = ntohs(len);
-	p += 2;
-	if (imsg->hdr.len < IMSG_HEADER_SIZE + 2 + withdrawn_len + 2) {
-		rde_update_err(peer, ERR_UPDATE, ERR_UPD_ATTRLIST, NULL, 0);
-		return;
-	}
-
-	p += withdrawn_len;
-	memcpy(&len, p, 2);
-	attrpath_len = len = ntohs(len);
-	p += 2;
-	if (imsg->hdr.len <
-	    IMSG_HEADER_SIZE + 2 + withdrawn_len + 2 + attrpath_len) {
-		rde_update_err(peer, ERR_UPDATE, ERR_UPD_ATTRLIST, NULL, 0);
+	if (ibuf_get_n16(buf, &len) == -1 ||
+	    ibuf_get_ibuf(buf, len, &wdbuf) == -1 ||
+	    ibuf_get_n16(buf, &len) == -1 ||
+	    ibuf_get_ibuf(buf, len, &attrbuf) == -1 ||
+	    ibuf_get_ibuf(buf, ibuf_size(buf), &nlribuf) == -1) {
+		rde_update_err(peer, ERR_UPDATE, ERR_UPD_ATTRLIST, NULL);
 		return;
 	}
 
-	nlri_len =
-	    imsg->hdr.len - IMSG_HEADER_SIZE - 4 - withdrawn_len - attrpath_len;
-
-	if (attrpath_len == 0) {
+	if (ibuf_size(&attrbuf) == 0) {
 		/* 0 = no NLRI information in this message */
-		if (nlri_len != 0) {
+		if (ibuf_size(&nlribuf) != 0) {
 			/* crap at end of update which should not be there */
-			rde_update_err(peer, ERR_UPDATE,
-			    ERR_UPD_ATTRLIST, NULL, 0);
+			rde_update_err(peer, ERR_UPDATE, ERR_UPD_ATTRLIST,
+			    NULL);
 			return;
 		}
-		if (withdrawn_len == 0) {
+		if (ibuf_size(&wdbuf) == 0) {
 			/* EoR marker */
 			rde_peer_recv_eor(peer, AID_INET);
 			return;
 		}
 	}
 
-	memset(&mpa, 0, sizeof(mpa));
+	ibuf_from_buffer(&reachbuf, NULL, 0);
+	ibuf_from_buffer(&unreachbuf, NULL, 0);
 	rde_filterstate_init(&state);
-	if (attrpath_len != 0) { /* 0 = no NLRI information in this message */
+	if (ibuf_size(&attrbuf) != 0) {
 		/* parse path attributes */
-		while (len > 0) {
-			if ((pos = rde_attr_parse(p, len, peer, &state,
-			    &mpa)) < 0)
+		while (ibuf_size(&attrbuf) > 0) {
+			if (rde_attr_parse(&attrbuf, peer, &state, &reachbuf,
+			    &unreachbuf) == -1)
 				goto done;
-			p += pos;
-			len -= pos;
 		}
 
 		/* check for missing but necessary attributes */
 		if ((subtype = rde_attr_missing(&state.aspath, peer->conf.ebgp,
-		    nlri_len))) {
+		    ibuf_size(&nlribuf)))) {
+			struct ibuf sbuf;
+			ibuf_from_buffer(&sbuf, &subtype, sizeof(subtype));
 			rde_update_err(peer, ERR_UPDATE, ERR_UPD_MISSNG_WK_ATTR,
-			    &subtype, sizeof(uint8_t));
+			    &sbuf);
 			goto done;
 		}
 
@@ -1452,12 +1433,13 @@ rde_update_dispatch(struct rde_peer *pee
 		    peer->conf.enforce_as == ENFORCE_AS_ON) {
 			fas = aspath_neighbor(state.aspath.aspath);
 			if (peer->conf.remote_as != fas) {
-			    log_peer_warnx(&peer->conf, "bad path, "
-				"starting with %s, "
-				"enforce neighbor-as enabled", log_as(fas));
-			    rde_update_err(peer, ERR_UPDATE, ERR_UPD_ASPATH,
-				    NULL, 0);
-			    goto done;
+				log_peer_warnx(&peer->conf, "bad path, "
+				    "starting with %s expected %u, "
+				    "enforce neighbor-as enabled",
+				    log_as(fas), peer->conf.remote_as);
+				rde_update_err(peer, ERR_UPDATE, ERR_UPD_ASPATH,
+				    NULL);
+				goto done;
 			}
 		}
 
@@ -1478,69 +1460,51 @@ rde_update_dispatch(struct rde_peer *pee
 		}
 	}
 
-	p = imsg->data;
-	len = withdrawn_len;
-	p += 2;
-
 	/* withdraw prefix */
-	if (len > 0) {
+	if (ibuf_size(&wdbuf) > 0) {
 		if (peer->capa.mp[AID_INET] == 0) {
 			log_peer_warnx(&peer->conf,
 			    "bad withdraw, %s disabled", aid2str(AID_INET));
-			rde_update_err(peer, ERR_UPDATE, ERR_UPD_OPTATTR,
-			    NULL, 0);
+			rde_update_err(peer, ERR_UPDATE, ERR_UPD_NETWORK,
+			    NULL);
 			goto done;
 		}
 	}
-	while (len > 0) {
+	while (ibuf_size(&wdbuf) > 0) {
 		if (peer_has_add_path(peer, AID_INET, CAPA_AP_RECV)) {
-			if (len <= sizeof(pathid)) {
+			if (ibuf_get_n32(&wdbuf, &pathid) == -1) {
 				log_peer_warnx(&peer->conf,
 				    "bad withdraw prefix");
 				rde_update_err(peer, ERR_UPDATE,
-				    ERR_UPD_NETWORK, NULL, 0);
+				    ERR_UPD_NETWORK, NULL);
 				goto done;
 			}
-			memcpy(&pathid, p, sizeof(pathid));
-			pathid = ntohl(pathid);
-			p += sizeof(pathid);
-			len -= sizeof(pathid);
 		} else
 			pathid = 0;
 
-		if ((pos = nlri_get_prefix(p, len, &prefix,
-		    &prefixlen)) == -1) {
+		if (nlri_get_prefix(&wdbuf, &prefix, &prefixlen) == -1) {
 			/*
 			 * the RFC does not mention what we should do in
 			 * this case. Let's do the same as in the NLRI case.
 			 */
 			log_peer_warnx(&peer->conf, "bad withdraw prefix");
 			rde_update_err(peer, ERR_UPDATE, ERR_UPD_NETWORK,
-			    NULL, 0);
+			    NULL);
 			goto done;
 		}
-		p += pos;
-		len -= pos;
 
 		rde_update_withdraw(peer, pathid, &prefix, prefixlen);
 	}
 
 	/* withdraw MP_UNREACH_NLRI if available */
-	if (mpa.unreach_len != 0) {
-		mpp = mpa.unreach;
-		mplen = mpa.unreach_len;
-		memcpy(&afi, mpp, 2);
-		mpp += 2;
-		mplen -= 2;
-		afi = ntohs(afi);
-		safi = *mpp++;
-		mplen--;
-
-		if (afi2aid(afi, safi, &aid) == -1) {
+	if (ibuf_size(&unreachbuf) != 0) {
+		if (ibuf_get_n16(&unreachbuf, &afi) == -1 ||
+		    ibuf_get_n8(&unreachbuf, &safi) == -1 ||
+		    afi2aid(afi, safi, &aid) == -1) {
 			log_peer_warnx(&peer->conf,
 			    "bad AFI/SAFI pair in withdraw");
 			rde_update_err(peer, ERR_UPDATE, ERR_UPD_OPTATTR,
-			    NULL, 0);
+			    &unreachbuf);
 			goto done;
 		}
 
@@ -1548,65 +1512,58 @@ rde_update_dispatch(struct rde_peer *pee
 			log_peer_warnx(&peer->conf,
 			    "bad withdraw, %s disabled", aid2str(aid));
 			rde_update_err(peer, ERR_UPDATE, ERR_UPD_OPTATTR,
-			    NULL, 0);
+			    &unreachbuf);
 			goto done;
 		}
 
 		if ((state.aspath.flags & ~F_ATTR_MP_UNREACH) == 0 &&
-		    mplen == 0) {
+		    ibuf_size(&unreachbuf) == 0) {
 			/* EoR marker */
 			rde_peer_recv_eor(peer, aid);
 		}
 
-		while (mplen > 0) {
+		while (ibuf_size(&unreachbuf) > 0) {
 			if (peer_has_add_path(peer, aid, CAPA_AP_RECV)) {
-				if (mplen <= sizeof(pathid)) {
+				if (ibuf_get_n32(&unreachbuf,
+				    &pathid) == -1) {
 					log_peer_warnx(&peer->conf,
 					    "bad %s withdraw prefix",
 					    aid2str(aid));
 					rde_update_err(peer, ERR_UPDATE,
-					    ERR_UPD_OPTATTR,
-					    mpa.unreach, mpa.unreach_len);
+					    ERR_UPD_OPTATTR, &unreachbuf);
 					goto done;
 				}
-				memcpy(&pathid, mpp, sizeof(pathid));
-				pathid = ntohl(pathid);
-				mpp += sizeof(pathid);
-				mplen -= sizeof(pathid);
 			} else
 				pathid = 0;
 
 			switch (aid) {
 			case AID_INET6:
-				if ((pos = nlri_get_prefix6(mpp, mplen,
-				    &prefix, &prefixlen)) == -1) {
+				if (nlri_get_prefix6(&unreachbuf,
+				    &prefix, &prefixlen) == -1) {
 					log_peer_warnx(&peer->conf,
 					    "bad IPv6 withdraw prefix");
 					rde_update_err(peer, ERR_UPDATE,
-					    ERR_UPD_OPTATTR,
-					    mpa.unreach, mpa.unreach_len);
+					    ERR_UPD_OPTATTR, &unreachbuf);
 					goto done;
 				}
 				break;
 			case AID_VPN_IPv4:
-				if ((pos = nlri_get_vpn4(mpp, mplen,
-				    &prefix, &prefixlen, 1)) == -1) {
+				if (nlri_get_vpn4(&unreachbuf,
+				    &prefix, &prefixlen, 1) == -1) {
 					log_peer_warnx(&peer->conf,
 					    "bad VPNv4 withdraw prefix");
 					rde_update_err(peer, ERR_UPDATE,
-					    ERR_UPD_OPTATTR,
-					    mpa.unreach, mpa.unreach_len);
+					    ERR_UPD_OPTATTR, &unreachbuf);
 					goto done;
 				}
 				break;
 			case AID_VPN_IPv6:
-				if ((pos = nlri_get_vpn6(mpp, mplen,
-				    &prefix, &prefixlen, 1)) == -1) {
+				if (nlri_get_vpn6(&unreachbuf,
+				    &prefix, &prefixlen, 1) == -1) {
 					log_peer_warnx(&peer->conf,
 					    "bad VPNv6 withdraw prefix");
 					rde_update_err(peer, ERR_UPDATE,
-					    ERR_UPD_OPTATTR, mpa.unreach,
-					    mpa.unreach_len);
+					    ERR_UPD_OPTATTR, &unreachbuf);
 					goto done;
 				}
 				break;
@@ -1615,14 +1572,17 @@ rde_update_dispatch(struct rde_peer *pee
 				/* ignore flowspec for now */
 			default:
 				/* ignore unsupported multiprotocol AF */
-				mpp += mplen;
-				mplen = 0;
+				if (ibuf_skip(&unreachbuf,
+				    ibuf_size(&unreachbuf)) == -1) {
+					log_peer_warnx(&peer->conf,
+					    "bad VPNv6 withdraw prefix");
+					rde_update_err(peer, ERR_UPDATE,
+					    ERR_UPD_OPTATTR, &unreachbuf);
+					goto done;
+				}
 				continue;
 			}
 
-			mpp += pos;
-			mplen -= pos;
-
 			rde_update_withdraw(peer, pathid, &prefix, prefixlen);
 		}
 
@@ -1630,16 +1590,13 @@ rde_update_dispatch(struct rde_peer *pee
 			goto done;
 	}
 
-	/* shift to NLRI information */
-	p += 2 + attrpath_len;
-
 	/* parse nlri prefix */
-	if (nlri_len > 0) {
+	if (ibuf_size(&nlribuf) > 0) {
 		if (peer->capa.mp[AID_INET] == 0) {
 			log_peer_warnx(&peer->conf,
 			    "bad update, %s disabled", aid2str(AID_INET));
-			rde_update_err(peer, ERR_UPDATE, ERR_UPD_OPTATTR,
-			    NULL, 0);
+			rde_update_err(peer, ERR_UPDATE, ERR_UPD_NETWORK,
+			    NULL);
 			goto done;
 		}
 
@@ -1655,7 +1612,7 @@ rde_update_dispatch(struct rde_peer *pee
 				    ATTR_OPTIONAL|ATTR_TRANSITIVE, ATTR_OTC,
 				    &tmp, sizeof(tmp)) == -1) {
 					rde_update_err(peer, ERR_UPDATE,
-					    ERR_UPD_ATTRLIST, NULL, 0);
+					    ERR_UPD_ATTRLIST, NULL);
 					goto done;
 				}
 				state.aspath.flags |= F_ATTR_OTC;
@@ -1665,54 +1622,39 @@ rde_update_dispatch(struct rde_peer *pee
 			}
 		}
 	}
-	while (nlri_len > 0) {
+	while (ibuf_size(&nlribuf) > 0) {
 		if (peer_has_add_path(peer, AID_INET, CAPA_AP_RECV)) {
-			if (nlri_len <= sizeof(pathid)) {
+			if (ibuf_get_n32(&nlribuf, &pathid) == -1) {
 				log_peer_warnx(&peer->conf,
 				    "bad nlri prefix");
 				rde_update_err(peer, ERR_UPDATE,
-				    ERR_UPD_NETWORK, NULL, 0);
+				    ERR_UPD_NETWORK, NULL);
 				goto done;
 			}
-			memcpy(&pathid, p, sizeof(pathid));
-			pathid = ntohl(pathid);
-			p += sizeof(pathid);
-			nlri_len -= sizeof(pathid);
 		} else
 			pathid = 0;
 
-		if ((pos = nlri_get_prefix(p, nlri_len, &prefix,
-		    &prefixlen)) == -1) {
+		if (nlri_get_prefix(&nlribuf, &prefix, &prefixlen) == -1) {
 			log_peer_warnx(&peer->conf, "bad nlri prefix");
 			rde_update_err(peer, ERR_UPDATE, ERR_UPD_NETWORK,
-			    NULL, 0);
+			    NULL);
 			goto done;
 		}
-		p += pos;
-		nlri_len -= pos;
 
 		if (rde_update_update(peer, pathid, &state,
 		    &prefix, prefixlen) == -1)
 			goto done;
-
 	}
 
 	/* add MP_REACH_NLRI if available */
-	if (mpa.reach_len != 0) {
-		mpp = mpa.reach;
-		mplen = mpa.reach_len;
-		memcpy(&afi, mpp, 2);
-		mpp += 2;
-		mplen -= 2;
-		afi = ntohs(afi);
-		safi = *mpp++;
-		mplen--;
-
-		if (afi2aid(afi, safi, &aid) == -1) {
+	if (ibuf_size(&reachbuf) != 0) {
+		if (ibuf_get_n16(&reachbuf, &afi) == -1 ||
+		    ibuf_get_n8(&reachbuf, &safi) == -1 ||
+		    afi2aid(afi, safi, &aid) == -1) {
 			log_peer_warnx(&peer->conf,
 			    "bad AFI/SAFI pair in update");
 			rde_update_err(peer, ERR_UPDATE, ERR_UPD_OPTATTR,
-			    NULL, 0);
+			    &reachbuf);
 			goto done;
 		}
 
@@ -1720,7 +1662,7 @@ rde_update_dispatch(struct rde_peer *pee
 			log_peer_warnx(&peer->conf,
 			    "bad update, %s disabled", aid2str(aid));
 			rde_update_err(peer, ERR_UPDATE, ERR_UPD_OPTATTR,
-			    NULL, 0);
+			    &reachbuf);
 			goto done;
 		}
 
@@ -1738,7 +1680,7 @@ rde_update_dispatch(struct rde_peer *pee
 					    ATTR_OTC, &tmp,
 					    sizeof(tmp)) == -1) {
 						rde_update_err(peer, ERR_UPDATE,
-						    ERR_UPD_ATTRLIST, NULL, 0);
+						    ERR_UPD_ATTRLIST, NULL);
 						goto done;
 					}
 					state.aspath.flags |= F_ATTR_OTC;
@@ -1755,64 +1697,53 @@ rde_update_dispatch(struct rde_peer *pee
 		/* unlock the previously locked nexthop, it is no longer used */
 		nexthop_unref(state.nexthop);
 		state.nexthop = NULL;
-		if ((pos = rde_get_mp_nexthop(mpp, mplen, aid, peer,
-		    &state)) == -1) {
+		if (rde_get_mp_nexthop(&reachbuf, aid, peer, &state) == -1) {
 			log_peer_warnx(&peer->conf, "bad nlri nexthop");
 			rde_update_err(peer, ERR_UPDATE, ERR_UPD_OPTATTR,
-			    mpa.reach, mpa.reach_len);
+			    &reachbuf);
 			goto done;
 		}
-		mpp += pos;
-		mplen -= pos;
 
-		while (mplen > 0) {
+		while (ibuf_size(&reachbuf) > 0) {
 			if (peer_has_add_path(peer, aid, CAPA_AP_RECV)) {
-				if (mplen <= sizeof(pathid)) {
+				if (ibuf_get_n32(&reachbuf, &pathid) == -1) {
 					log_peer_warnx(&peer->conf,
 					    "bad %s nlri prefix", aid2str(aid));
 					rde_update_err(peer, ERR_UPDATE,
-					    ERR_UPD_OPTATTR,
-					    mpa.reach, mpa.reach_len);
+					    ERR_UPD_OPTATTR, &reachbuf);
 					goto done;
 				}
-				memcpy(&pathid, mpp, sizeof(pathid));
-				pathid = ntohl(pathid);
-				mpp += sizeof(pathid);
-				mplen -= sizeof(pathid);
 			} else
 				pathid = 0;
 
 			switch (aid) {
 			case AID_INET6:
-				if ((pos = nlri_get_prefix6(mpp, mplen,
-				    &prefix, &prefixlen)) == -1) {
+				if (nlri_get_prefix6(&reachbuf,
+				    &prefix, &prefixlen) == -1) {
 					log_peer_warnx(&peer->conf,
 					    "bad IPv6 nlri prefix");
 					rde_update_err(peer, ERR_UPDATE,
-					    ERR_UPD_OPTATTR,
-					    mpa.reach, mpa.reach_len);
+					    ERR_UPD_OPTATTR, &reachbuf);
 					goto done;
 				}
 				break;
 			case AID_VPN_IPv4:
-				if ((pos = nlri_get_vpn4(mpp, mplen,
-				    &prefix, &prefixlen, 0)) == -1) {
+				if (nlri_get_vpn4(&reachbuf,
+				    &prefix, &prefixlen, 0) == -1) {
 					log_peer_warnx(&peer->conf,
 					    "bad VPNv4 nlri prefix");
 					rde_update_err(peer, ERR_UPDATE,
-					    ERR_UPD_OPTATTR,
-					    mpa.reach, mpa.reach_len);
+					    ERR_UPD_OPTATTR, &reachbuf);
 					goto done;
 				}
 				break;
 			case AID_VPN_IPv6:
-				if ((pos = nlri_get_vpn6(mpp, mplen,
-				    &prefix, &prefixlen, 0)) == -1) {
+				if (nlri_get_vpn6(&reachbuf,
+				    &prefix, &prefixlen, 0) == -1) {
 					log_peer_warnx(&peer->conf,
 					    "bad VPNv6 nlri prefix");
 					rde_update_err(peer, ERR_UPDATE,
-					    ERR_UPD_OPTATTR,
-					    mpa.reach, mpa.reach_len);
+					    ERR_UPD_OPTATTR, &reachbuf);
 					goto done;
 				}
 				break;
@@ -1821,14 +1752,17 @@ rde_update_dispatch(struct rde_peer *pee
 				/* ignore flowspec for now */
 			default:
 				/* ignore unsupported multiprotocol AF */
-				mpp += mplen;
-				mplen = 0;
+				if (ibuf_skip(&reachbuf,
+				    ibuf_size(&reachbuf)) == -1) {
+					log_peer_warnx(&peer->conf,
+					    "bad VPNv6 withdraw prefix");
+					rde_update_err(peer, ERR_UPDATE,
+					    ERR_UPD_OPTATTR, &reachbuf);
+					goto done;
+				}
 				continue;
 			}
 
-			mpp += pos;
-			mplen -= pos;
-
 			if (rde_update_update(peer, pathid, &state,
 			    &prefix, prefixlen) == -1)
 				goto done;
@@ -1920,7 +1854,7 @@ rde_update_update(struct rde_peer *peer,
 	    peer->stats.prefix_cnt > peer->conf.max_prefix) {
 		log_peer_warnx(&peer->conf, "prefix limit reached (>%u/%u)",
 		    peer->stats.prefix_cnt, peer->conf.max_prefix);
-		rde_update_err(peer, ERR_CEASE, ERR_CEASE_MAX_PREFIX, NULL, 0);
+		rde_update_err(peer, ERR_CEASE, ERR_CEASE_MAX_PREFIX, NULL);
 		return (-1);
 	}
 
@@ -1992,63 +1926,63 @@ rde_update_withdraw(struct rde_peer *pee
 	(((s) & ~(ATTR_DEFMASK | (m))) == (t))
 
 int
-rde_attr_parse(u_char *p, uint16_t len, struct rde_peer *peer,
-    struct filterstate *state, struct mpattr *mpa)
+rde_attr_parse(struct ibuf *buf, struct rde_peer *peer,
+    struct filterstate *state, struct ibuf *reach, struct ibuf *unreach)
 {
 	struct bgpd_addr nexthop;
 	struct rde_aspath *a = &state->aspath;
-	u_char		*op = p, *npath;
+	struct ibuf	 attrbuf;
+	u_char		*p, *npath;
 	uint32_t	 tmp32, zero = 0;
 	int		 error;
-	uint16_t	 attr_len, nlen;
-	uint16_t	 plen = 0;
-	uint8_t		 flags, type, tmp8;
-
-	if (len < 3) {
-bad_len:
-		rde_update_err(peer, ERR_UPDATE, ERR_UPD_ATTRLEN, op, len);
-		return (-1);
-	}
+	uint16_t	 nlen;
+	size_t		 attr_len, hlen, plen;
+	uint8_t		 flags, type;
+
+        ibuf_from_ibuf(&attrbuf, buf);
+	if (ibuf_get_n8(&attrbuf, &flags) == -1 ||
+	    ibuf_get_n8(&attrbuf, &type) == -1)
+		goto bad_list;
 
-	UPD_READ(&flags, p, plen, 1);
-	UPD_READ(&type, p, plen, 1);
 
 	if (flags & ATTR_EXTLEN) {
-		if (len - plen < 2)
-			goto bad_len;
-		UPD_READ(&attr_len, p, plen, 2);
-		attr_len = ntohs(attr_len);
+		uint16_t alen;
+		if (ibuf_get_n16(&attrbuf, &alen) == -1)
+			goto bad_list;
+		attr_len = alen;
+		hlen = 4;
 	} else {
-		UPD_READ(&tmp8, p, plen, 1);
-		attr_len = tmp8;
+		uint8_t alen;
+		if (ibuf_get_n8(&attrbuf, &alen) == -1)
+			goto bad_list;
+		attr_len = alen;
+		hlen = 3;
 	}
 
-	if (len - plen < attr_len)
-		goto bad_len;
+	if (ibuf_truncate(&attrbuf, attr_len) == -1)
+		goto bad_list;
+	/* consume the attribute in buf before moving forward */
+	if (ibuf_skip(buf, hlen + attr_len) == -1)
+		goto bad_list;
 
-	/* adjust len to the actual attribute size including header */
-	len = plen + attr_len;
+	p = ibuf_data(&attrbuf);
+	plen = ibuf_size(&attrbuf);
 
 	switch (type) {
 	case ATTR_UNDEF:
 		/* ignore and drop path attributes with a type code of 0 */
-		plen += attr_len;
 		break;
 	case ATTR_ORIGIN:
 		if (attr_len != 1)
 			goto bad_len;
 
-		if (!CHECK_FLAGS(flags, ATTR_WELL_KNOWN, 0)) {
-bad_flags:
-			rde_update_err(peer, ERR_UPDATE, ERR_UPD_ATTRFLAGS,
-			    op, len);
-			return (-1);
-		}
+		if (!CHECK_FLAGS(flags, ATTR_WELL_KNOWN, 0))
+			goto bad_flags;
 
 		UPD_READ(&a->origin, p, plen, 1);
 		if (a->origin > ORIGIN_INCOMPLETE) {
 			rde_update_err(peer, ERR_UPDATE, ERR_UPD_ORIGIN,
-			    op, len);
+			    &attrbuf);
 			return (-1);
 		}
 		if (a->flags & F_ATTR_ORIGIN)
@@ -2069,7 +2003,7 @@ bad_flags:
 			a->flags |= F_ATTR_PARSE_ERR;
 		} else if (error != 0) {
 			rde_update_err(peer, ERR_UPDATE, ERR_UPD_ASPATH,
-			    NULL, 0);
+			    NULL);
 			return (-1);
 		}
 		if (a->flags & F_ATTR_ASPATH)
@@ -2116,7 +2050,7 @@ bad_flags:
 		tmp32 = ntohl(nexthop.v4.s_addr);
 		if (IN_MULTICAST(tmp32)) {
 			rde_update_err(peer, ERR_UPDATE, ERR_UPD_NEXTHOP,
-			    op, len);
+			    &attrbuf);
 			return (-1);
 		}
 		nexthop_unref(state->nexthop);	/* just to be sure */
@@ -2270,9 +2204,7 @@ bad_flags:
 			goto bad_list;
 		a->flags |= F_ATTR_MP_REACH;
 
-		mpa->reach = p;
-		mpa->reach_len = attr_len;
-		plen += attr_len;
+		*reach = attrbuf;
 		break;
 	case ATTR_MP_UNREACH_NLRI:
 		if (attr_len < 3)
@@ -2284,9 +2216,7 @@ bad_flags:
 			goto bad_list;
 		a->flags |= F_ATTR_MP_UNREACH;
 
-		mpa->unreach = p;
-		mpa->unreach_len = attr_len;
-		plen += attr_len;
+		*unreach = attrbuf;
 		break;
 	case ATTR_AS4_AGGREGATOR:
 		if (attr_len != 8) {
@@ -2353,22 +2283,29 @@ bad_flags:
 	default:
 		if ((flags & ATTR_OPTIONAL) == 0) {
 			rde_update_err(peer, ERR_UPDATE, ERR_UPD_UNKNWN_WK_ATTR,
-			    op, len);
-			return (-1);
-		}
-optattr:
-		if (attr_optadd(a, flags, type, p, attr_len) == -1) {
-bad_list:
-			rde_update_err(peer, ERR_UPDATE, ERR_UPD_ATTRLIST,
-			    NULL, 0);
+			    &attrbuf);
 			return (-1);
 		}
+ optattr:
+		if (attr_optadd(a, flags, type, p, attr_len) == -1)
+			goto bad_list;
 
 		plen += attr_len;
 		break;
 	}
 
-	return (plen);
+	(void)plen;	/* XXX make compiler happy for now */
+	return (0);
+
+ bad_len:
+	rde_update_err(peer, ERR_UPDATE, ERR_UPD_ATTRLEN, &attrbuf);
+	return (-1);
+ bad_flags:
+	rde_update_err(peer, ERR_UPDATE, ERR_UPD_ATTRFLAGS, &attrbuf);
+	return (-1);
+ bad_list:
+	rde_update_err(peer, ERR_UPDATE, ERR_UPD_ATTRLIST, NULL);
+	return (-1);
 }
 
 #undef UPD_READ
@@ -2438,20 +2375,19 @@ rde_attr_missing(struct rde_aspath *a, i
 }
 
 int
-rde_get_mp_nexthop(u_char *data, uint16_t len, uint8_t aid,
+rde_get_mp_nexthop(struct ibuf *buf, uint8_t aid,
     struct rde_peer *peer, struct filterstate *state)
 {
 	struct bgpd_addr	nexthop;
-	uint8_t			totlen, nhlen;
+	struct ibuf		nhbuf;
+	uint8_t			nhlen;
 
-	if (len == 0)
+	if (ibuf_get_n8(buf, &nhlen) == -1)
 		return (-1);
-
-	nhlen = *data++;
-	totlen = 1;
-	len--;
-
-	if (nhlen + 1 > len)
+	if (ibuf_get_ibuf(buf, nhlen, &nhbuf) == -1)
+		return (-1);
+	/* ignore reserved (old SNPA) field as per RFC4760 */
+	if (ibuf_skip(buf, 1) == -1)
 		return (-1);
 
 	memset(&nexthop, 0, sizeof(nexthop));
@@ -2470,7 +2406,8 @@ rde_get_mp_nexthop(u_char *data, uint16_
 			    "bad size %d", aid2str(aid), nhlen);
 			return (-1);
 		}
-		memcpy(&nexthop.v6.s6_addr, data, 16);
+		if (ibuf_get(&nhbuf, &nexthop.v6, sizeof(nexthop.v6)) == -1)
+			return (-1);
 		nexthop.aid = AID_INET6;
 		if (IN6_IS_ADDR_LINKLOCAL(&nexthop.v6)) {
 			if (peer->local_if_scope != 0) {
@@ -2502,9 +2439,10 @@ rde_get_mp_nexthop(u_char *data, uint16_
 			    "bad size %d", aid2str(aid), nhlen);
 			return (-1);
 		}
+		if (ibuf_skip(&nhbuf, sizeof(uint64_t)) == -1 ||
+		    ibuf_get(&nhbuf, &nexthop.v4, sizeof(nexthop.v4)) == -1)
+			return (-1);
 		nexthop.aid = AID_INET;
-		memcpy(&nexthop.v4, data + sizeof(uint64_t),
-		    sizeof(nexthop.v4));
 		break;
 	case AID_VPN_IPv6:
 		if (nhlen != 24) {
@@ -2512,8 +2450,9 @@ rde_get_mp_nexthop(u_char *data, uint16_
 			    "bad size %d", aid2str(aid), nhlen);
 			return (-1);
 		}
-		memcpy(&nexthop.v6, data + sizeof(uint64_t),
-		    sizeof(nexthop.v6));
+		if (ibuf_skip(&nhbuf, sizeof(uint64_t)) == -1 ||
+		    ibuf_get(&nhbuf, &nexthop.v6, sizeof(nexthop.v6)) == -1)
+			return (-1);
 		nexthop.aid = AID_INET6;
 		if (IN6_IS_ADDR_LINKLOCAL(&nexthop.v6)) {
 			if (peer->local_if_scope != 0) {
@@ -2534,8 +2473,7 @@ rde_get_mp_nexthop(u_char *data, uint16_
 			    "bad size %d", aid2str(aid), nhlen);
 			return (-1);
 		}
-		/* also ignore reserved (old SNPA) field as per RFC4760 */
-		return (totlen + 1);
+		return (0);
 	default:
 		log_peer_warnx(&peer->conf, "bad multiprotocol nexthop, "
 		    "bad AID");
@@ -2544,25 +2482,29 @@ rde_get_mp_nexthop(u_char *data, uint16_
 
 	state->nexthop = nexthop_get(&nexthop);
 
-	/* ignore reserved (old SNPA) field as per RFC4760 */
-	totlen += nhlen + 1;
-
-	return (totlen);
+	return (0);
 }
 
 void
 rde_update_err(struct rde_peer *peer, uint8_t error, uint8_t suberr,
-    void *data, uint16_t size)
+    struct ibuf *opt)
 {
-	struct ibuf	*wbuf;
+	struct ibuf *wbuf;
+	size_t size = 0;
 
+	if (opt != NULL) {
+		ibuf_rewind(opt);
+		size = ibuf_size(opt);
+	}
 	if ((wbuf = imsg_create(ibuf_se, IMSG_UPDATE_ERR, peer->conf.id, 0,
 	    size + sizeof(error) + sizeof(suberr))) == NULL)
 		fatal("%s %d imsg_create error", __func__, __LINE__);
 	if (imsg_add(wbuf, &error, sizeof(error)) == -1 ||
-	    imsg_add(wbuf, &suberr, sizeof(suberr)) == -1 ||
-	    imsg_add(wbuf, data, size) == -1)
+	    imsg_add(wbuf, &suberr, sizeof(suberr)) == -1)
 		fatal("%s %d imsg_add error", __func__, __LINE__);
+	if (opt != NULL)
+		if (ibuf_add_ibuf(wbuf, opt) == -1)
+			fatal("%s %d imsg_add error", __func__, __LINE__);
 	imsg_close(ibuf_se, wbuf);
 	peer->state = PEER_ERR;
 }
Index: usr.sbin/bgpd/rde.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
diff -u -p -r1.297 rde.h
--- usr.sbin/bgpd/rde.h	16 Oct 2023 10:25:46 -0000	1.297
+++ usr.sbin/bgpd/rde.h	22 Jan 2024 11:23:09 -0000
@@ -169,13 +169,6 @@ struct attr {
 	uint8_t				 type;
 };
 
-struct mpattr {
-	void		*reach;
-	void		*unreach;
-	uint16_t	 reach_len;
-	uint16_t	 unreach_len;
-};
-
 struct rde_community {
 	RB_ENTRY(rde_community)		entry;
 	int				size;
@@ -341,7 +334,7 @@ void		mrt_dump_upcall(struct rib_entry *
 
 /* rde.c */
 void		 rde_update_err(struct rde_peer *, uint8_t , uint8_t,
-		    void *, uint16_t);
+		    struct ibuf *);
 void		 rde_update_log(const char *, uint16_t,
 		    const struct rde_peer *, const struct bgpd_addr *,
 		    const struct bgpd_addr *, uint8_t);
Index: usr.sbin/bgpd/rde_update.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_update.c,v
diff -u -p -r1.164 rde_update.c
--- usr.sbin/bgpd/rde_update.c	12 Oct 2023 14:16:28 -0000	1.164
+++ usr.sbin/bgpd/rde_update.c	22 Jan 2024 11:23:21 -0000
@@ -199,7 +199,7 @@ up_process_prefix(struct rde_peer *peer,
 		    "outbound prefix limit reached (>%u/%u)",
 		    peer->stats.prefix_out_cnt, peer->conf.max_out_prefix);
 		rde_update_err(peer, ERR_CEASE,
-		    ERR_CEASE_MAX_SENT_PREFIX, NULL, 0);
+		    ERR_CEASE_MAX_SENT_PREFIX, NULL);
 		return UP_ERR_LIMIT;
 	}
 
@@ -447,7 +447,7 @@ up_generate_default(struct rde_peer *pee
 		    "outbound prefix limit reached (>%u/%u)",
 		    peer->stats.prefix_out_cnt, peer->conf.max_out_prefix);
 		rde_update_err(peer, ERR_CEASE,
-		    ERR_CEASE_MAX_SENT_PREFIX, NULL, 0);
+		    ERR_CEASE_MAX_SENT_PREFIX, NULL);
 	}
 }
 
Index: usr.sbin/bgpd/util.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/util.c,v
diff -u -p -r1.78 util.c
--- usr.sbin/bgpd/util.c	10 Jan 2024 13:31:09 -0000	1.78
+++ usr.sbin/bgpd/util.c	22 Jan 2024 13:15:06 -0000
@@ -563,12 +563,13 @@ aspath_inflate(void *data, uint16_t len,
 	return (ndata);
 }
 
+static const u_char	addrmask[] = { 0x00, 0x80, 0xc0, 0xe0, 0xf0,
+			    0xf8, 0xfc, 0xfe, 0xff };
+
 /* NLRI functions to extract prefixes from the NLRI blobs */
 int
 extract_prefix(const u_char *p, int len, void *va, uint8_t pfxlen, uint8_t max)
 {
-	static u_char	 addrmask[] = {
-	    0x00, 0x80, 0xc0, 0xe0, 0xf0, 0xf8, 0xfc, 0xfe, 0xff };
 	u_char		*a = va;
 	int		 plen;
 
@@ -588,187 +589,177 @@ extract_prefix(const u_char *p, int len,
 	return (plen);
 }
 
+static int
+extract_prefix_buf(struct ibuf *buf, void *va, uint8_t pfxlen, uint8_t max)
+{
+	u_char		*a = va;
+	unsigned int	 plen;
+	uint8_t		 tmp;
+
+	plen = PREFIX_SIZE(pfxlen) - 1;
+	if (ibuf_size(buf) < plen || max < plen)
+		return -1;
+
+	while (pfxlen > 0) {
+		if (ibuf_get_n8(buf, &tmp) == -1)
+			return -1;
+
+		if (pfxlen < 8) {
+			*a++ = tmp & addrmask[pfxlen];
+			break;
+		} else {
+			*a++ = tmp;
+			pfxlen -= 8;
+		}
+	}
+	return (0);
+}
+
 int
-nlri_get_prefix(u_char *p, uint16_t len, struct bgpd_addr *prefix,
-    uint8_t *prefixlen)
+nlri_get_prefix(struct ibuf *buf, struct bgpd_addr *prefix, uint8_t *prefixlen)
 {
-	int	 plen;
 	uint8_t	 pfxlen;
 
-	if (len < 1)
+	if (ibuf_get_n8(buf, &pfxlen) == -1)
+		return (-1);
+	if (pfxlen > 32)
 		return (-1);
-
-	pfxlen = *p++;
-	len--;
 
 	memset(prefix, 0, sizeof(struct bgpd_addr));
 	prefix->aid = AID_INET;
-	*prefixlen = pfxlen;
 
-	if (pfxlen > 32)
-		return (-1);
-	if ((plen = extract_prefix(p, len, &prefix->v4, pfxlen,
-	    sizeof(prefix->v4))) == -1)
+	if (extract_prefix_buf(buf, &prefix->v4, pfxlen,
+	    sizeof(prefix->v4)) == -1)
 		return (-1);
 
-	return (plen + 1);	/* pfxlen needs to be added */
+	*prefixlen = pfxlen;
+	return (0);
 }
 
 int
-nlri_get_prefix6(u_char *p, uint16_t len, struct bgpd_addr *prefix,
-    uint8_t *prefixlen)
+nlri_get_prefix6(struct ibuf *buf, struct bgpd_addr *prefix, uint8_t *prefixlen)
 {
-	int	plen;
 	uint8_t	pfxlen;
 
-	if (len < 1)
+	if (ibuf_get_n8(buf, &pfxlen) == -1)
+		return (-1);
+	if (pfxlen > 128)
 		return (-1);
-
-	pfxlen = *p++;
-	len--;
 
 	memset(prefix, 0, sizeof(struct bgpd_addr));
 	prefix->aid = AID_INET6;
-	*prefixlen = pfxlen;
 
-	if (pfxlen > 128)
-		return (-1);
-	if ((plen = extract_prefix(p, len, &prefix->v6, pfxlen,
-	    sizeof(prefix->v6))) == -1)
+	if (extract_prefix_buf(buf, &prefix->v6, pfxlen,
+	    sizeof(prefix->v6)) == -1)
 		return (-1);
 
-	return (plen + 1);	/* pfxlen needs to be added */
+	*prefixlen = pfxlen;
+	return (0);
 }
 
 int
-nlri_get_vpn4(u_char *p, uint16_t len, struct bgpd_addr *prefix,
+nlri_get_vpn4(struct ibuf *buf, struct bgpd_addr *prefix,
     uint8_t *prefixlen, int withdraw)
 {
-	int		 rv, done = 0;
-	uint16_t	 plen;
+	int		 done = 0;
 	uint8_t		 pfxlen;
 
-	if (len < 1)
+	if (ibuf_get_n8(buf, &pfxlen) == -1)
 		return (-1);
 
-	memcpy(&pfxlen, p, 1);
-	p += 1;
-	plen = 1;
-
 	memset(prefix, 0, sizeof(struct bgpd_addr));
+	prefix->aid = AID_VPN_IPv4;
 
 	/* label stack */
 	do {
-		if (len - plen < 3 || pfxlen < 3 * 8)
-			return (-1);
-		if (prefix->labellen + 3U >
-		    sizeof(prefix->labelstack))
+		if (prefix->labellen + 3U > sizeof(prefix->labelstack) ||
+		    pfxlen < 3 * 8)
 			return (-1);
 		if (withdraw) {
 			/* on withdraw ignore the labelstack all together */
-			p += 3;
-			plen += 3;
+			if (ibuf_skip(buf, 3) == -1)
+				return (-1);
 			pfxlen -= 3 * 8;
 			break;
 		}
-		prefix->labelstack[prefix->labellen++] = *p++;
-		prefix->labelstack[prefix->labellen++] = *p++;
-		prefix->labelstack[prefix->labellen] = *p++;
-		if (prefix->labelstack[prefix->labellen] &
+		if (ibuf_get(buf, &prefix->labelstack[prefix->labellen], 3) ==
+		    -1)
+			return -1;
+		if (prefix->labelstack[prefix->labellen + 2] &
 		    BGP_MPLS_BOS)
 			done = 1;
-		prefix->labellen++;
-		plen += 3;
+		prefix->labellen += 3;
 		pfxlen -= 3 * 8;
 	} while (!done);
 
 	/* RD */
-	if (len - plen < (int)sizeof(uint64_t) ||
-	    pfxlen < sizeof(uint64_t) * 8)
+	if (pfxlen < sizeof(uint64_t) * 8 ||
+	    ibuf_get_h64(buf, &prefix->rd) == -1)
 		return (-1);
-	memcpy(&prefix->rd, p, sizeof(uint64_t));
 	pfxlen -= sizeof(uint64_t) * 8;
-	p += sizeof(uint64_t);
-	plen += sizeof(uint64_t);
 
 	/* prefix */
-	prefix->aid = AID_VPN_IPv4;
-	*prefixlen = pfxlen;
-
 	if (pfxlen > 32)
 		return (-1);
-	if ((rv = extract_prefix(p, len, &prefix->v4,
-	    pfxlen, sizeof(prefix->v4))) == -1)
+	if (extract_prefix_buf(buf, &prefix->v4, pfxlen,
+	    sizeof(prefix->v4)) == -1)
 		return (-1);
 
-	return (plen + rv);
+	*prefixlen = pfxlen;
+	return (0);
 }
 
 int
-nlri_get_vpn6(u_char *p, uint16_t len, struct bgpd_addr *prefix,
+nlri_get_vpn6(struct ibuf *buf, struct bgpd_addr *prefix,
     uint8_t *prefixlen, int withdraw)
 {
-	int		rv, done = 0;
-	uint16_t	plen;
+	int		done = 0;
 	uint8_t		pfxlen;
 
-	if (len < 1)
+	if (ibuf_get_n8(buf, &pfxlen) == -1)
 		return (-1);
 
-	memcpy(&pfxlen, p, 1);
-	p += 1;
-	plen = 1;
-
 	memset(prefix, 0, sizeof(struct bgpd_addr));
+	prefix->aid = AID_VPN_IPv6;
 
 	/* label stack */
 	do {
-		if (len - plen < 3 || pfxlen < 3 * 8)
-			return (-1);
-		if (prefix->labellen + 3U >
-		    sizeof(prefix->labelstack))
+		if (prefix->labellen + 3U > sizeof(prefix->labelstack) ||
+		    pfxlen < 3 * 8)
 			return (-1);
 		if (withdraw) {
 			/* on withdraw ignore the labelstack all together */
-			p += 3;
-			plen += 3;
+			if (ibuf_skip(buf, 3) == -1)
+				return (-1);
 			pfxlen -= 3 * 8;
 			break;
 		}
 
-		prefix->labelstack[prefix->labellen++] = *p++;
-		prefix->labelstack[prefix->labellen++] = *p++;
-		prefix->labelstack[prefix->labellen] = *p++;
-		if (prefix->labelstack[prefix->labellen] &
+		if (ibuf_get(buf, &prefix->labelstack[prefix->labellen], 3) ==
+		    -1)
+			return (-1);
+		if (prefix->labelstack[prefix->labellen + 2] &
 		    BGP_MPLS_BOS)
 			done = 1;
-		prefix->labellen++;
-		plen += 3;
+		prefix->labellen += 3;
 		pfxlen -= 3 * 8;
 	} while (!done);
 
 	/* RD */
-	if (len - plen < (int)sizeof(uint64_t) ||
-	    pfxlen < sizeof(uint64_t) * 8)
+	if (pfxlen < sizeof(uint64_t) * 8 ||
+	    ibuf_get_h64(buf, &prefix->rd) == -1)
 		return (-1);
-
-	memcpy(&prefix->rd, p, sizeof(uint64_t));
 	pfxlen -= sizeof(uint64_t) * 8;
-	p += sizeof(uint64_t);
-	plen += sizeof(uint64_t);
 
 	/* prefix */
-	prefix->aid = AID_VPN_IPv6;
-	*prefixlen = pfxlen;
-
 	if (pfxlen > 128)
 		return (-1);
-
-	if ((rv = extract_prefix(p, len, &prefix->v6,
-	    pfxlen, sizeof(prefix->v6))) == -1)
+	if (extract_prefix_buf(buf, &prefix->v6, pfxlen,
+	    sizeof(prefix->v6)) == -1)
 		return (-1);
 
-	return (plen + rv);
+	*prefixlen = pfxlen;
+	return (0);
 }
 
 static in_addr_t