Re: [PATCH 6/6] ndp: Send unsolicited Router Advertisements

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 13 Nov 2024 14:01:16 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Nov 13, 2024 at 02:14:16AM +0100, Stefano Brivio wrote:
> > Kind of nits only, the whole series looks good to me otherwise:
> > 
> > On Tue, 12 Nov 2024 15:06:18 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > Currently, our NDP implementation only sends Router Advertisements (RA)
> > > when it receives a Router Solicitation (RS) from the guest.  However,
> > > RFC 4861 requires that we periodically send unsolicited RAs.
> > > 
> > > Linux as a guest also requires this: it will send an RS when a link first
> > > comes up, but the route it gets from this will have a finite lifetime (we
> > > set this to 65535s, the maximum allowed, around 18 hours).  When that
> > > expires the guest will not send a new RS, but instead expects the route to
> > > have been renewed (if still valid) by an unsolicited RA.
> > > 
> > > Implement sending unsolicited RAs on a partially randomised timer, as
> > > required by RFC 4861.  The RFC also specifies that solicited RAs should
> > > also be delayed, or even not omitted, if the next unsolicited RA is soon  
> > 
> > s/not//  
> 
> Fixed.
> 
> > > enough.  For now we don't do that, always sending an immediate RA in
> > > response to an RS.  We can get away with this because in our use cases
> > > we expect to just have passt itself and the guest on the link, rather than
> > > a large broadcast domain.
> > > 
> > > Link: https://github.com/kubevirt/kubevirt/issues/13191
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  ip.h    |  9 +++++++++
> > >  ndp.c   | 41 +++++++++++++++++++++++++++++++++++++++++
> > >  ndp.h   |  3 +++
> > >  passt.c |  3 +++
> > >  4 files changed, 56 insertions(+)
> > > 
> > > diff --git a/ip.h b/ip.h
> > > index b8d4a5b..0742612 100644
> > > --- a/ip.h
> > > +++ b/ip.h
> > > @@ -92,4 +92,13 @@ struct ipv6_opt_hdr {
> > >  
> > >  char *ipv6_l4hdr(const struct pool *p, int idx, size_t offset, uint8_t *proto,
> > >  		 size_t *dlen);
> > > +
> > > +/* IPv6 link-local all-nodes multicast adddress, ff02::1 */
> > > +static const struct in6_addr in6addr_ll_all_nodes = {
> > > +	.s6_addr = {
> > > +		0xff, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > +		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01,
> > > +	},
> > > +};
> > > +
> > >  #endif /* IP_H */
> > > diff --git a/ndp.c b/ndp.c
> > > index c5fbccf..8c019df 100644
> > > --- a/ndp.c
> > > +++ b/ndp.c
> > > @@ -372,3 +372,44 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
> > >  
> > >  	return 1;
> > >  }
> > > +
> > > +/* Default interval between unsolicited RAs (seconds) */
> > > +#define DEFAULT_MAX_RTR_ADV_INTERVAL	600	/* RFC 4861, 6.2.1 */
> > > +
> > > +/* Minimum required interval between RAs (seconds) */
> > > +#define MIN_DELAY_BETWEEN_RAS		3	/* RFC 4861, 10 */  
> > 
> > By the way, I think this is still all correct, as I quickly double
> > checked it against RFC 8319. If you're not aware: see also sections 3
> > and 4 there.  
> 
> Ah, thanks for checking.  I didn't think to look for a superseding RFC.
> 
> > > +
> > > +static time_t next_ra;
> > > +
> > > +/**
> > > + * ndp_timer() - Send unsolicited NDP messages if necessary
> > > + * @c:		Execution context
> > > + * @now:	Current (monotonic) time
> > > + */
> > > +void ndp_timer(const struct ctx *c, const struct timespec *now)
> > > +{
> > > +	time_t max_rtr_adv_interval = DEFAULT_MAX_RTR_ADV_INTERVAL;
> > > +	time_t min_rtr_adv_interval, interval;
> > > +
> > > +	if (c->no_ra || now->tv_sec < next_ra)
> > > +		return;
> > > +
> > > +	/* We must advertise before the route's lifetime expires */
> > > +	max_rtr_adv_interval = MIN(max_rtr_adv_interval, RT_LIFETIME - 1);
> > > +
> > > +	/* But we must not go smaller than the minimum delay */
> > > +	max_rtr_adv_interval = MAX(max_rtr_adv_interval, MIN_DELAY_BETWEEN_RAS);
> > > +
> > > +	/* RFC 4861, 6.2.1 */
> > > +	min_rtr_adv_interval = MAX(max_rtr_adv_interval / 3,
> > > +				   MIN_DELAY_BETWEEN_RAS);
> > > +
> > > +	interval = min_rtr_adv_interval +
> > > +		random() % (max_rtr_adv_interval - min_rtr_adv_interval);  
> > 
> > So, I would have called srandom() before this, especially in the case
> > we get, one day, two instances of passt advertising at the same
> > time.  
> 
> Good point, I'll add that.
> 
> > But Coverity is more annoying than I am and reports:
> > 
> > /home/sbrivio/passt/ndp.c:408:3:
> >   Type: Calling risky function (DC.WEAK_CRYPTO)
> > 
> > /home/sbrivio/passt/ndp.c:408:3:
> >   dont_call: "random" should not be used for security-related applications, because linear congruential algorithms are too easy to break.
> > /home/sbrivio/passt/ndp.c:408:3:
> >   remediation: Use a compliant random number generator, such as "/dev/random" or "/dev/urandom" on Unix-like systems, and CNG (Cryptography API: Next Generation) on Windows.
> > 
> > Of course it's all bogus but I would have a *slight* preference to get
> > rid of this, by either picking a fixed interval deviation at the
> > beginning with getrandom(), or using something like tcp_init_seq()
> > modulo something << 600.  
> 
> I don't think the latter approach really works.  Using the time as the
> basic input means its still subject to two passt instances (say)
> starting at the same time on the same link picking the same values and
> keeping on sending their announcements in lockstep.

Well, but the seeds would be different (I would still factor
hash_secret in), so we would start out of sync right away, and we would
use a fine-grained time resolution to pick a coarser one, which adds
(enough) to the pseudorandomness.

> Picking a random interval once is better.  Still, I don't particularly
> like deviating from the RFC's recommendations just to keep a fussy
> tool happy.

Oh, I thought it would still fit the recommendation, but now I read RFC
4861 6.2.4 again and it definitely doesn't.

> > Alternatively, we could also keep /dev/random or /dev/urandom open but
> > it looks totally overkill. At that point I'd rather keep random()
> > here.  
> 
> Also a waste of /dev/random entropy, IMO.
> 
> Surely there must be some way we can suppress the Coverity whinge.

That adds more maintenance burden than having a warning at this point,
so I'd rather not try to suppress anything. If the tcp_init_seq()-like
thing works, I'd be happy, otherwise let's leave this like it is.

-- 
Stefano