From: Theo Buehler <tb@theobuehler.org>
Subject: Re: bgpd: refine the update attr parse errors
To: tech@openbsd.org
Date: Mon, 2 Mar 2026 09:48:06 +0100

On Thu, Feb 19, 2026 at 11:15:48AM +0100, Claudio Jeker wrote:
> This diff is refining the error split I did in the previous commit to
> rde.c by introducing one more common failure case with extra debug info.
> I also adjusted the messages a bit, to be more self explanatory. I'm happy
> to adjust those texts further since it is important to have messages that
> operators understand without looking at the code.
> 
> I added a new message for the case where the attribute length causes an
> overflow of the attribute buffer. This is when the ibuf_truncate() fails
> since the alen is larger then the available data.
> In many cases it is possible to extract the flags, types and even length
> fields but the length value is for whatever reason garbage.

ok

> 
> This diff includes a bug fix in the OTC parser for ROLE_PEER. At that
> stage the parser should not consume the data in attrbuf and so it needs to
> use an extra temporary buffer to extract the OTC value. This is similar to
> the case in ATTR_AS4_AGGREGATOR where the same trick is used.
> This is the second hunk, and I will commit this separately.

also ok and yes, in a separate commit, please

> 
> -- 
> :wq Claudio
> 
> Index: rde.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> diff -u -p -r1.686 rde.c
> --- rde.c	18 Feb 2026 15:54:06 -0000	1.686
> +++ rde.c	19 Feb 2026 09:28:27 -0000
> @@ -2056,7 +2056,7 @@ rde_attr_parse(struct ibuf *buf, struct 
>  	}
>  
>  	if (ibuf_truncate(&attrbuf, alen) == -1)
> -		goto bad_ibuf;
> +		goto bad_size;
>  	/* consume the attribute in buf before moving forward */
>  	if (ibuf_skip(buf, hlen + alen) == -1)
>  		goto bad_ibuf;
> @@ -2375,7 +2375,8 @@ rde_attr_parse(struct ibuf *buf, struct 
>  			a->flags |= F_ATTR_OTC_LEAK;
>  			break;
>  		case ROLE_PEER:
> -			if (ibuf_get_n32(&attrbuf, &tmp32) == -1)
> +			ibuf_from_ibuf(&tmpbuf, &attrbuf);
> +			if (ibuf_get_n32(&tmpbuf, &tmp32) == -1)
>  				goto bad_len;
>  			if (tmp32 != peer->conf.remote_as)
>  				a->flags |= F_ATTR_OTC_LEAK;
> @@ -2407,11 +2408,18 @@ rde_attr_parse(struct ibuf *buf, struct 
>  	rde_update_err(peer, ERR_UPDATE, ERR_UPD_ATTRFLAGS, &attrbuf);
>  	return (-1);
>   bad_list:
> -	log_peer_warnx(&peer->conf, "bad path, list error for type %d", type);
> +	log_peer_warnx(&peer->conf, "bad update attributes, "
> +	    "list error for attribute #%d", type);
>  	rde_update_err(peer, ERR_UPDATE, ERR_UPD_ATTRLIST, NULL);
>  	return (-1);
>   bad_ibuf:
> -	log_peer_warn(&peer->conf, "bad path, header parse error");
> +	log_peer_warn(&peer->conf, "bad update attributes, "
> +	    "message parse error");
> +	rde_update_err(peer, ERR_UPDATE, ERR_UPD_ATTRLIST, NULL);
> +	return (-1);
> + bad_size:
> +	log_peer_warn(&peer->conf, "bad update attributes, "
> +	    "attribute #%d [%x] with size %zu overflowed", type, flags, alen);
>  	rde_update_err(peer, ERR_UPDATE, ERR_UPD_ATTRLIST, NULL);
>  	return (-1);
>  }
>