Re: [PATCH 4/4] ndp: Use high quality entropy in NDP timer even if not needed

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3409 bytes --]

On Thu, Jul 16, 2026 at 09:22:44AM +0200, Stefano Brivio wrote:
> On Thu, 16 Jul 2026 15:45:30 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Jul 16, 2026 at 01:25:23AM +0200, Stefano Brivio wrote:
> > > ...instead of calling random(), to make static checkers happy.
> > > 
> > > I don't think that an attacker could actually gain anything by making
> > > router advertisement intervals predictable, but a doubt remains, and
> > > this is cheap enough that we might just want to do this to get rid of
> > > the noise from static checkers informing us that random() shouldn't be
> > > used.
> > > 
> > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > > ---
> > >  ndp.c | 11 +++++++----
> > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/ndp.c b/ndp.c
> > > index 1f2bcb0..43457b3 100644
> > > --- a/ndp.c
> > > +++ b/ndp.c
> > > @@ -413,6 +413,7 @@ 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;
> > > +	long random_part;
> > >  
> > >  	if (!tap_is_ready(c) || c->no_ra || now->tv_sec < next_ra)
> > >  		return;
> > > @@ -433,15 +434,17 @@ void ndp_timer(const struct ctx *c, const struct timespec *now)
> > >  	 * and causing flurries of RAs at the same time.
> > >  	 *
> > >  	 * This random doesn't need to be cryptographically strong, so random(3)
> > > -	 * is fine.  Other routers on the link also want to avoid
> > > -	 * synchronisation, and anything malicious has much easier ways to cause
> > > -	 * trouble.
> > > +	 * would be fine.  Other routers on the link also want to avoid
> > > +	 * synchronisation, and anything malicious would have much easier ways
> > > +	 * to cause trouble.  However, for the sake of static checkers, use high
> > > +	 * quality entropy as provided by raw_random().
> > >  	 *
> > >  	 * The modulus also makes this not strictly a uniform distribution, but,
> > >  	 * again, it's close enough for our purposes.
> > >  	 */
> > > +	raw_random(&random_part, sizeof(random_part));
> > >  	interval = min_rtr_adv_interval +
> > > -		random() % (max_rtr_adv_interval - min_rtr_adv_interval);
> > > +		   random_part % (max_rtr_adv_interval - min_rtr_adv_interval);  
> > 
> > Although it returns a signed long, random() is explicitly defined to
> > only return values between 0 and 2^31-1.
> 
> Oops, I missed that part. I would have naturally used uint32_t here but
> then I looked (too quickly) at the prototype of random() and concluded
> it would be better to make it equivalent... except it's not.
> 
> > Using raw_random() means we
> > can get anything in the full range of a 'long', including negative
> > numbers.  Is that going to mess up our calculations here?
> 
> I don't think in any catastrophic way, but it might, yes.

Probably not, no.  But I always forget what the rules are for % on
signed values, so best avoided, I think.

> > Might be safer to make random_part a uint32_t, then cast it to a
> > time_t for the arithmetic.
> 
> Right, v2 does that.
> 
> -- 
> Stefano
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]