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: Tue, 5 May 2026 10:40:05 +0200

On Tue, May 05, 2026 at 09:27:21AM +0200, Theo Buehler wrote:
> On Mon, May 04, 2026 at 04:06:54PM +0200, Claudio Jeker wrote:
> > 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.
> 
> Thanks, that helped. I'm not sure I would have changed all the other l
> and oalen, but if you prefer it this way I won't object.
> 
> ok tb

I committed this in the end. I switched the type of l to unsigned int and
skipped the change to all other length vars for now. 
This is the minimal fix for now, which is good enough.

-- 
:wq Claudio

Index: rde_attr.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_attr.c,v
diff -u -p -r1.142 -r1.143
--- rde_attr.c	30 Apr 2026 15:51:07 -0000	1.142
+++ rde_attr.c	5 May 2026 08:37:45 -0000	1.143
@@ -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;
+	unsigned 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));