[PATCH] ndp: Suppress Coverity false positive for random()

[PATCH] ndp: Suppress Coverity false positive for random()

[Laurent Vivier]

Coverity flags the random() call in ndp_timer() with the dont_call
checker, warning that it should not be used for security-related
applications.

This is a false positive: random() is used here to jitter the interval
between unsolicited Router Advertisements as required by RFC 4861, to
prevent synchronisation between routers on a link.  No cryptographic
strength is needed.

Suppress the warning with an inline Coverity annotation.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 ndp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ndp.c b/ndp.c
index 1f2bcb0cc7ea..614932ac5829 100644
--- a/ndp.c
+++ b/ndp.c
@@ -441,6 +441,7 @@ void ndp_timer(const struct ctx *c, const struct timespec *now)
 	 * again, it's close enough for our purposes.
 	 */
 	interval = min_rtr_adv_interval +
+		/* coverity[dont_call:FALSE] */
 		random() % (max_rtr_adv_interval - min_rtr_adv_interval);
 
 	if (!next_ra)
-- 
2.54.0

Re: [PATCH] ndp: Suppress Coverity false positive for random()

[Stefano Brivio]

On Wed, 13 May 2026 12:26:17 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> Coverity flags the random() call in ndp_timer() with the dont_call
> checker, warning that it should not be used for security-related
> applications.
> 
> This is a false positive: random() is used here to jitter the interval
> between unsolicited Router Advertisements as required by RFC 4861, to
> prevent synchronisation between routers on a link.  No cryptographic
> strength is needed.
> 
> Suppress the warning with an inline Coverity annotation.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  ndp.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/ndp.c b/ndp.c
> index 1f2bcb0cc7ea..614932ac5829 100644
> --- a/ndp.c
> +++ b/ndp.c
> @@ -441,6 +441,7 @@ void ndp_timer(const struct ctx *c, const struct timespec *now)
>  	 * again, it's close enough for our purposes.
>  	 */
>  	interval = min_rtr_adv_interval +
> +		/* coverity[dont_call:FALSE] */

Sorry, I should have mentioned this to you explicitly, but we discussed
this in the past and we decided against having explicit suppressions
for warnings from Coverity Scan (at least, that would be my strong
preference).

The reason is that I would like to avoid referring to trademarks as
much as possible, as they might raise "interesting" legal questions,
and at the same time we have very little control or visibility into how
these suppressions evolve in future versions of the checker.

In this case, by the way, despite the fact that we use this to add some
randomness to the timing of router advertisements as required by RFC
4861, I started wondering recently if an attacker (I'm mostly thinking
about denials of service) could actually gain anything from making
these intervals predictable.

If that's the case, perhaps we could just switch to getrandom() and
be done with it.

-- 
Stefano

Re: [PATCH] ndp: Suppress Coverity false positive for random()

[David Gibson]

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

On Thu, May 14, 2026 at 01:08:26AM +0200, Stefano Brivio wrote:
> On Wed, 13 May 2026 12:26:17 +0200
> Laurent Vivier <lvivier@redhat.com> wrote:
> 
> > Coverity flags the random() call in ndp_timer() with the dont_call
> > checker, warning that it should not be used for security-related
> > applications.
> > 
> > This is a false positive: random() is used here to jitter the interval
> > between unsolicited Router Advertisements as required by RFC 4861, to
> > prevent synchronisation between routers on a link.  No cryptographic
> > strength is needed.
> > 
> > Suppress the warning with an inline Coverity annotation.
> > 
> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > ---
> >  ndp.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/ndp.c b/ndp.c
> > index 1f2bcb0cc7ea..614932ac5829 100644
> > --- a/ndp.c
> > +++ b/ndp.c
> > @@ -441,6 +441,7 @@ void ndp_timer(const struct ctx *c, const struct timespec *now)
> >  	 * again, it's close enough for our purposes.
> >  	 */
> >  	interval = min_rtr_adv_interval +
> > +		/* coverity[dont_call:FALSE] */
> 
> Sorry, I should have mentioned this to you explicitly, but we discussed
> this in the past and we decided against having explicit suppressions
> for warnings from Coverity Scan (at least, that would be my strong
> preference).
> 
> The reason is that I would like to avoid referring to trademarks as
> much as possible, as they might raise "interesting" legal questions,
> and at the same time we have very little control or visibility into how
> these suppressions evolve in future versions of the checker.

If this suppression format also works for the sort-of-public
scan.coverity.com, that mitigates those later concerns somewhat.
Still doesn't remove the kind of implicit endorsement of including a
trademark, which I can understand why we'd want to avoid.

> In this case, by the way, despite the fact that we use this to add some
> randomness to the timing of router advertisements as required by RFC
> 4861, I started wondering recently if an attacker (I'm mostly thinking
> about denials of service) could actually gain anything from making
> these intervals predictable.
> 
> If that's the case, perhaps we could just switch to getrandom() and
> be done with it.

Hm, maybe, yes.  These announcements are infrequent enuogh that the
extra cost of getrandom() shouldn't really be a problem.

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