From: Theo Buehler Subject: Re: bgpd rewrite rde update parser (step 1) To: Claudio Jeker Cc: tech@openbsd.org Date: Tue, 23 Jan 2024 16:08:50 +0100 On Mon, Jan 22, 2024 at 03:15:38PM +0100, Claudio Jeker wrote: > 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. I'm fine with these XXX except that I don't really like the one in mrt_extract_prefix(). The other comments inline are purely cosmetic. ok tb > > -- > :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; Maybe this could be moved to the end of the function like you did elsewhere > } > 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 */ Should this really be done unconditionally and hide failures of nlri_get_*? I would have expected something equivalent to if (r != -1) r = len - ibuf_size(msg); /* XXX */ Even if this will be cleaned up in the next few days, I think this would make more sense. > 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; kill the ' ' before the 'b' in 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; same here > 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); spaces -> tab > + 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 >