From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: bgpd: other attributes vs the bucket allocator
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Mon, 4 May 2026 16:06:54 +0200

On Mon, May 04, 2026 at 02:03:52PM +0200, Theo Buehler wrote:
> On Thu, Apr 30, 2026 at 09:22:56PM +0200, Claudio Jeker wrote:
> > This is a fun one. The new bucket allocator used for attrs jumps from
> > 240 to 256 entries. The problem is that l is a uint8_t and so 256
> > becomes 0. The reallocarray shrinks the array and you can imagine the
> > rest.
> > 
> > In attr_optadd() check the value returned by bin_of_attrs() and limit it
> > to UCHAR_MAX to prevent the overflow. Also switch the iterator variable
> > from uint8_t to int (I did this everywhere because it is more natural).
> > 
> > Does this make sense?
> 
> I think it does but I find it very difficult to see through all the magic.
> 
> Just to make sure: do I understand correctly that the
> 
> 	fatalx("attr_optadd: attribute overflow");
> 
> wasn't reachable before and that the fact that it is reachable now is
> making things work as intended?

Yes. The check
	if (asp->others_len == UCHAR_MAX)
could no longer trigger since bin_of_attrs() returns a sequence that
goes from 240 to 256.

In other words when adding the 241 attribute l turned 0 and as a
consequence of that a reallocarray of 0 elements and a memset with a
negative size would happen. 

We can't really get to 255 attributes since a fair bunch are handled
differently (like ASPATH, COMMUNITY or MED) but it is possible to exceed
240.

> > -- 
> > :wq Claudio
> > 
> > Index: mrt.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/bgpd/mrt.c,v
> > diff -u -p -r1.134 mrt.c
> > --- mrt.c	17 Feb 2026 14:06:44 -0000	1.134
> > +++ mrt.c	30 Apr 2026 18:35:08 -0000
> > @@ -222,9 +222,9 @@ mrt_attr_dump(struct ibuf *buf, struct r
> >  	struct attr	*oa;
> >  	u_char		*pdata;
> >  	uint32_t	 tmp;
> > -	int		 neednewpath = 0;
> > +	int		 l, neednewpath = 0;
> >  	uint16_t	 plen, afi;
> > -	uint8_t		 l, safi;
> > +	uint8_t		 safi;
> >  
> >  	/* origin */
> >  	if (attr_writebuf(buf, ATTR_WELL_KNOWN, ATTR_ORIGIN,
> > Index: rde.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> > diff -u -p -r1.694 rde.c
> > --- rde.c	27 Apr 2026 15:52:20 -0000	1.694
> > +++ rde.c	30 Apr 2026 19:00:49 -0000
> > @@ -2924,7 +2924,7 @@ rde_dump_rib_as(struct prefix *p, struct
> >  	struct rde_peer		*peer;
> >  	monotime_t		 staletime;
> >  	size_t			 aslen;
> > -	uint8_t			 l;
> > +	int			 l;
> >  
> >  	nexthop = prefix_nexthop(p);
> >  	peer = prefix_peer(p);
> > @@ -3042,7 +3042,7 @@ rde_dump_adjout_as(struct rde_peer *peer
> >  	struct rde_aspath	*asp;
> >  	struct nexthop		*nexthop;
> >  	size_t			 aslen;
> > -	uint8_t			 l;
> > +	int			 l;
> >  
> >  	nexthop = attrs->nexthop;
> >  	asp = attrs->aspath;
> > Index: rde_attr.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/bgpd/rde_attr.c,v
> > diff -u -p -r1.141 rde_attr.c
> > --- rde_attr.c	17 Mar 2026 09:29:29 -0000	1.141
> > +++ rde_attr.c	30 Apr 2026 19:08:36 -0000
> > @@ -95,7 +95,7 @@ int
> >  attr_optadd(struct rde_aspath *asp, uint8_t flags, uint8_t type,
> >      void *data, uint16_t len)
> >  {
> > -	uint8_t		 l;
> > +	int		 l;
> >  	struct attr	*a, *t;
> >  	struct attr	**p;
> >  	uint64_t	 h;
> > @@ -137,6 +137,8 @@ attr_optadd(struct rde_aspath *asp, uint
> >  		fatalx("attr_optadd: attribute overflow");
> >  
> >  	l = bin_of_attrs(asp->others_len + 1);
> > +	if (l > UCHAR_MAX)
> > +		l = UCHAR_MAX;
> >  	if ((p = reallocarray(asp->others, l, sizeof(struct attr *))) == NULL)
> >  		fatal("%s", __func__);
> >  	memset(&p[asp->others_len], 0, (l - asp->others_len) * sizeof(*p));
> > @@ -150,7 +152,7 @@ attr_optadd(struct rde_aspath *asp, uint
> >  struct attr *
> >  attr_optget(const struct rde_aspath *asp, uint8_t type)
> >  {
> > -	uint8_t l;
> > +	int l;
> >  
> >  	for (l = 0; l < asp->others_len; l++) {
> >  		if (asp->others[l] == NULL)
> > @@ -166,7 +168,7 @@ attr_optget(const struct rde_aspath *asp
> >  void
> >  attr_copy(struct rde_aspath *t, const struct rde_aspath *s)
> >  {
> > -	uint8_t l;
> > +	int l;
> >  
> >  	if (t->others != NULL)
> >  		attr_freeall(t);
> > @@ -206,7 +208,7 @@ attr_eq(const struct attr *oa, const str
> >  int
> >  attr_equal(const struct rde_aspath *a, const struct rde_aspath *b)
> >  {
> > -	uint8_t l;
> > +	int l;
> >  
> >  	if (a->others_len != b->others_len)
> >  		return (0);
> > @@ -219,7 +221,7 @@ attr_equal(const struct rde_aspath *a, c
> >  void
> >  attr_free(struct rde_aspath *asp, struct attr *attr)
> >  {
> > -	uint8_t l;
> > +	int l;
> >  
> >  	for (l = 0; l < asp->others_len; l++)
> >  		if (asp->others[l] == attr) {
> > @@ -236,7 +238,7 @@ attr_free(struct rde_aspath *asp, struct
> >  void
> >  attr_freeall(struct rde_aspath *asp)
> >  {
> > -	uint8_t l;
> > +	int l;
> >  
> >  	for (l = 0; l < asp->others_len; l++)
> >  		attr_put(asp->others[l]);
> > Index: rde_update.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/bgpd/rde_update.c,v
> > diff -u -p -r1.193 rde_update.c
> > --- rde_update.c	11 Feb 2026 10:24:57 -0000	1.193
> > +++ rde_update.c	30 Apr 2026 18:36:05 -0000
> > @@ -591,9 +591,9 @@ up_generate_attr(struct ibuf *buf, struc
> >  	struct attr	*oa = NULL, *newaggr = NULL;
> >  	u_char		*pdata;
> >  	uint32_t	 tmp32;
> > -	int		 flags, neednewpath = 0, rv;
> > +	int		 flags, neednewpath = 0, rv, oalen = 0;
> >  	uint16_t	 plen;
> > -	uint8_t		 oalen = 0, type;
> > +	uint8_t		 type;
> >  
> >  	if (asp->others_len > 0)
> >  		oa = asp->others[oalen++];
> > 
> 

-- 
:wq Claudio