Download raw body.
bgpd rewrite rde update parser (step 1)
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
>
bgpd rewrite rde update parser (step 1)