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: 2646 bytes --]

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.  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?

Might be safer to make random_part a uint32_t, then cast it to a
time_t for the arithmetic.
>  
>  	if (!next_ra)
>  		goto first;
> -- 
> 2.43.0
> 

-- 
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 --]