From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: bgpd more ibuf in session.c parse functions
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Mon, 20 May 2024 11:35:58 +0200

On Mon, May 20, 2024 at 11:06:38AM +0200, Theo Buehler wrote:
> On Fri, May 17, 2024 at 12:27:59PM +0200, Claudio Jeker wrote:
> > This converts most parse functions over to use the ibuf API.
> > This is still not perfect mainly because of the way these parse functions
> > are called. So at the start they do some scary buffer shit before finally
> > using an ibuf. I want to tackle that slowly but this diff is already big
> > enough.
> > 
> > This passes regress and the cert-bgp-testcases which fuzz OPEN a fair bit.
> > I still need to build something to test RFC9072 support (at least it does
> > not affect the non extended lenght mode).
> 
> Couldn't spot anything wrong with it in multiple passes.
> 
> ok tb
> 
> nits inline.
> 
> > 
> > -- 
> > :wq Claudio
> > 
> > Index: session.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
> > diff -u -p -r1.476 session.c
> > --- session.c	16 May 2024 09:38:21 -0000	1.476
> > +++ session.c	17 May 2024 10:15:24 -0000
> > @@ -87,7 +87,7 @@ int	parse_open(struct peer *);
> >  int	parse_update(struct peer *);
> >  int	parse_rrefresh(struct peer *);
> >  void	parse_notification(struct peer *);
> > -int	parse_capabilities(struct peer *, u_char *, uint16_t, uint32_t *);
> > +int	parse_capabilities(struct peer *, struct ibuf *, uint32_t *);
> >  int	capa_neg_calc(struct peer *);
> >  void	session_dispatch_imsg(struct imsgbuf *, int, u_int *);
> >  void	session_up(struct peer *);
> > @@ -115,6 +115,11 @@ u_int			 peer_cnt;
> >  struct mrt_head		 mrthead;
> >  time_t			 pauseaccept;
> >  
> > +static const u_char	 marker[MSGSIZE_HEADER_MARKER] = {
> 
> I think uint8_t would be better

Changed.
 
> > @@ -2157,13 +2156,13 @@ parse_header(struct peer *peer, u_char *
> >  int
> >  parse_open(struct peer *peer)
> >  {
> > -	u_char		*p, *op_val;
> > +	struct ibuf	 ibuf;
> > +	u_char		*p;
> 
> Not clear when you use u_char and when uint8_t. I'd prefer sticking
> to the latter.

I left that as is since it was like that before and it is part of the code
that I want to kill anyway.
 
> >  	uint8_t		 version, rversion;
> >  	uint16_t	 short_as, msglen;
> > -	uint16_t	 holdtime, oholdtime, myholdtime;
> > +	uint16_t	 holdtime, myholdtime;
> >  	uint32_t	 as, bgpid;
> > -	uint16_t	 optparamlen, extlen, plen, op_len;
> > -	uint8_t		 op_type;
> > +	uint8_t		 op_type, optparamlen;
> 
> The op_type could move into the if (optparamlen != 0) scope or even the while loop.

Moved.
 
> > +	if (ibuf_get_n8(&ibuf, &version) == -1 ||
> > +	    ibuf_get_n16(&ibuf, &short_as) == -1 ||
> > +	    ibuf_get_n16(&ibuf, &holdtime) == -1 ||
> > +	    ibuf_get_h32(&ibuf, &bgpid) == -1 ||
> 
> The get_h32 hiding between all the get_n* feels like a trap. I'd be inclined to
> use ibuf_get_n32() and convert back when assigning to peer->remote_bgpid.
> Your call.

Done it like that plus comment. We could swap the byte order in a second
diff since the RDE already uses it that way. Will be another diff.
 
> > +		/* check for RFC9072 encoding */
> > +		if (ibuf_get_n8(&oparams, &ext_type) == -1)
> > +			goto bad_len;
> > +		if (ext_type == OPT_PARAM_EXT_LEN) {
> > +			if (ibuf_get_n16(&oparams, &ext_len) == -1)
> > +				goto bad_len;
> > +			/* skip RFC9072 header */
> > +		    	if (ibuf_skip(&ibuf, 3) == -1)
> 
> There are four spaces hiding in the previuos line

Fixed
 
> > +	/* XXX */
> > +	ibuf_from_buffer(&ibuf, p, datalen);
> > +	
> 
> stray tab in previous line

Fixed
 
-- 
:wq Claudio