[PATCH 0/4] Assorted fixes, address a static checker warning

[PATCH 0/4] Assorted fixes, address a static checker warning

[Stefano Brivio]

These are small assorted fixes and a change to address a static checker
warning I had on my list for a while but never found the time to polish
up for sending.

So here they are. There's no particular relationship between patches,
they are actually independent changes.

Stefano Brivio (4):
  CONTRIBUTING.md: The tag is "Link:", regardless of how many we have
  dhcp: Make option parsing more robust, explicitly handle options 0 and
    255
  passt.1, pesto.1: ::1 is an address, not a port
  ndp: Use high quality entropy in NDP timer even if not needed

 CONTRIBUTING.md |  2 +-
 dhcp.c          | 10 +++++++++-
 ndp.c           | 11 +++++++----
 passt.1         |  2 +-
 pesto.1         |  2 +-
 5 files changed, 19 insertions(+), 8 deletions(-)

-- 
2.43.0

[PATCH 1/4] CONTRIBUTING.md: The tag is "Link:", regardless of how many we have

[Stefano Brivio]

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 CONTRIBUTING.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 64c191f..98997ae 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -48,7 +48,7 @@ to prepare patches and participate in the email-based review process.
     The `Subsystem` means: which part of the code your change touches.
     For example, it could be "tcp", "test", or "doc" etc.
 
-    If there are some references, use "Links:" tag for anything.
+    If there are some references, use "Link:" tags for anything.
 
     Besides, passt uses the Linux kernel's "Signed-off-by" process. If you can
     certify the below:
-- 
2.43.0

Re: [PATCH 1/4] CONTRIBUTING.md: The tag is "Link:", regardless of how many we have

[David Gibson]

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

On Thu, Jul 16, 2026 at 01:25:20AM +0200, Stefano Brivio wrote:
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  CONTRIBUTING.md | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
> index 64c191f..98997ae 100644
> --- a/CONTRIBUTING.md
> +++ b/CONTRIBUTING.md
> @@ -48,7 +48,7 @@ to prepare patches and participate in the email-based review process.
>      The `Subsystem` means: which part of the code your change touches.
>      For example, it could be "tcp", "test", or "doc" etc.
>  
> -    If there are some references, use "Links:" tag for anything.
> +    If there are some references, use "Link:" tags for anything.
>  
>      Besides, passt uses the Linux kernel's "Signed-off-by" process. If you can
>      certify the below:
> -- 
> 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 --]

[PATCH 2/4] dhcp: Make option parsing more robust, explicitly handle options 0 and 255

[Stefano Brivio]

The initial option-scanning loop in dhcp(), so far, ignored options 0
(Pad Option, RFC 2132, Section 3.1) and 255 (End Option, RFC 2132,
Section 3.2).

As a result:

- if we ever encountered option 0 in the middle of option fields
  (never seen in practice), we would potentially terminate the loop
  too early, before scanning remaining options

- a malformed message with an option 255 followed by a length byte
  would (reliably) cause us to terminate as we would exceed the
  allocated size for the 'opts' array, which is detected as buffer
  overflow by the FORTIFY_SOURCE mechanism

The latter was reported as potential vulnerability by AISLE, but it's
not actually a vulnerability as we always terminate without carrying
on further handling, and in our security model the guest is able to
sabotage its own connectivity in any case (for example, a malformed
frame from the hypervisor would cause us to reset the connection, or
entirely flooding the flow table would cause inbound connectivity to
stop working, etc.).

The reported behaviour, however, is indeed a defect, as it affects
the functional robustness to a hypothetical issue in a DHCP client,
and that's something we definitely want to fix.

Make the option parsing loop more robust by:

- resizing 'opts' from 255 to 256 elements: there's no particular
  reason to try to save a tiny bit of memory (which shouldn't even
  be allocated in practice) instead of being defensive about it

- explicitly handle options 0 (skip one byte, continue) and 255 (stop
  processing options) in the option-scanning loop

This bug was found and an initial version of the patch was written by
the AISLE AI security scanning tool (https://aisle.com/platform).

Reported-by: AISLE
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 dhcp.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/dhcp.c b/dhcp.c
index 1ff8cba..632019a 100644
--- a/dhcp.c
+++ b/dhcp.c
@@ -49,7 +49,7 @@ struct opt {
 	uint8_t c[255];
 };
 
-static struct opt opts[255];
+static struct opt opts[256];
 
 #define DHCPDISCOVER	1
 #define DHCPOFFER	2
@@ -374,6 +374,14 @@ int dhcp(const struct ctx *c, struct iov_tail *data)
 		if (!type || !olen)
 			return -1;
 
+		if (*type == 255)
+			break;
+
+		if (*type == 0) { /* Pad Option (RFC 2132, 3.1): one byte */
+			opt_len--;
+			continue;
+		}
+
 		opt_len = iov_tail_size(data);
 		if (opt_len < *olen)
 			return -1;
-- 
2.43.0

Re: [PATCH 2/4] dhcp: Make option parsing more robust, explicitly handle options 0 and 255

[David Gibson]

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

On Thu, Jul 16, 2026 at 01:25:21AM +0200, Stefano Brivio wrote:
> The initial option-scanning loop in dhcp(), so far, ignored options 0
> (Pad Option, RFC 2132, Section 3.1) and 255 (End Option, RFC 2132,
> Section 3.2).
> 
> As a result:
> 
> - if we ever encountered option 0 in the middle of option fields
>   (never seen in practice), we would potentially terminate the loop
>   too early, before scanning remaining options
> 
> - a malformed message with an option 255 followed by a length byte
>   would (reliably) cause us to terminate as we would exceed the
>   allocated size for the 'opts' array, which is detected as buffer
>   overflow by the FORTIFY_SOURCE mechanism
> 
> The latter was reported as potential vulnerability by AISLE, but it's
> not actually a vulnerability as we always terminate without carrying
> on further handling, and in our security model the guest is able to
> sabotage its own connectivity in any case (for example, a malformed
> frame from the hypervisor would cause us to reset the connection, or
> entirely flooding the flow table would cause inbound connectivity to
> stop working, etc.).
> 
> The reported behaviour, however, is indeed a defect, as it affects
> the functional robustness to a hypothetical issue in a DHCP client,
> and that's something we definitely want to fix.
> 
> Make the option parsing loop more robust by:
> 
> - resizing 'opts' from 255 to 256 elements: there's no particular
>   reason to try to save a tiny bit of memory (which shouldn't even
>   be allocated in practice) instead of being defensive about it
> 
> - explicitly handle options 0 (skip one byte, continue) and 255 (stop
>   processing options) in the option-scanning loop
> 
> This bug was found and an initial version of the patch was written by
> the AISLE AI security scanning tool (https://aisle.com/platform).
> 
> Reported-by: AISLE
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  dhcp.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/dhcp.c b/dhcp.c
> index 1ff8cba..632019a 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -49,7 +49,7 @@ struct opt {
>  	uint8_t c[255];
>  };
>  
> -static struct opt opts[255];
> +static struct opt opts[256];
>  
>  #define DHCPDISCOVER	1
>  #define DHCPOFFER	2
> @@ -374,6 +374,14 @@ int dhcp(const struct ctx *c, struct iov_tail *data)
>  		if (!type || !olen)
>  			return -1;
>  
> +		if (*type == 255)
> +			break;
> +
> +		if (*type == 0) { /* Pad Option (RFC 2132, 3.1): one byte */
> +			opt_len--;
> +			continue;
> +		}
> +

I don't think this is quite right: at this point we've already
stripped off the non-existent length-byte with IOV_REMOVE_HEADER,
meaning opt_len will get out of sync with iov_tail_size(data).

I think we instead need to check for the 1-byte option cases between
the two IOV_REMOVE_HEADER() calls.  And.. since presumably the options
could theoretically end with some padding options, we probably want
the loop to be while (opt_len >= 1) instead of 2.  In fact the way we
mix recalculating opt_len from iov_tail_size() with sometimes directly
updating it is pretty nasty.  We might be better off with
	while ((opt_len = iov_tail_size(data)))


>  		opt_len = iov_tail_size(data);
>  		if (opt_len < *olen)
>  			return -1;
> -- 
> 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 --]

Re: [PATCH 2/4] dhcp: Make option parsing more robust, explicitly handle options 0 and 255

[Stefano Brivio]

On Thu, 16 Jul 2026 15:37:13 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Jul 16, 2026 at 01:25:21AM +0200, Stefano Brivio wrote:
> > The initial option-scanning loop in dhcp(), so far, ignored options 0
> > (Pad Option, RFC 2132, Section 3.1) and 255 (End Option, RFC 2132,
> > Section 3.2).
> > 
> > As a result:
> > 
> > - if we ever encountered option 0 in the middle of option fields
> >   (never seen in practice), we would potentially terminate the loop
> >   too early, before scanning remaining options
> > 
> > - a malformed message with an option 255 followed by a length byte
> >   would (reliably) cause us to terminate as we would exceed the
> >   allocated size for the 'opts' array, which is detected as buffer
> >   overflow by the FORTIFY_SOURCE mechanism
> > 
> > The latter was reported as potential vulnerability by AISLE, but it's
> > not actually a vulnerability as we always terminate without carrying
> > on further handling, and in our security model the guest is able to
> > sabotage its own connectivity in any case (for example, a malformed
> > frame from the hypervisor would cause us to reset the connection, or
> > entirely flooding the flow table would cause inbound connectivity to
> > stop working, etc.).
> > 
> > The reported behaviour, however, is indeed a defect, as it affects
> > the functional robustness to a hypothetical issue in a DHCP client,
> > and that's something we definitely want to fix.
> > 
> > Make the option parsing loop more robust by:
> > 
> > - resizing 'opts' from 255 to 256 elements: there's no particular
> >   reason to try to save a tiny bit of memory (which shouldn't even
> >   be allocated in practice) instead of being defensive about it
> > 
> > - explicitly handle options 0 (skip one byte, continue) and 255 (stop
> >   processing options) in the option-scanning loop
> > 
> > This bug was found and an initial version of the patch was written by
> > the AISLE AI security scanning tool (https://aisle.com/platform).
> > 
> > Reported-by: AISLE
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > ---
> >  dhcp.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/dhcp.c b/dhcp.c
> > index 1ff8cba..632019a 100644
> > --- a/dhcp.c
> > +++ b/dhcp.c
> > @@ -49,7 +49,7 @@ struct opt {
> >  	uint8_t c[255];
> >  };
> >  
> > -static struct opt opts[255];
> > +static struct opt opts[256];
> >  
> >  #define DHCPDISCOVER	1
> >  #define DHCPOFFER	2
> > @@ -374,6 +374,14 @@ int dhcp(const struct ctx *c, struct iov_tail *data)
> >  		if (!type || !olen)
> >  			return -1;
> >  
> > +		if (*type == 255)
> > +			break;
> > +
> > +		if (*type == 0) { /* Pad Option (RFC 2132, 3.1): one byte */
> > +			opt_len--;
> > +			continue;
> > +		}
> > +  
> 
> I don't think this is quite right: at this point we've already
> stripped off the non-existent length-byte with IOV_REMOVE_HEADER,
> meaning opt_len will get out of sync with iov_tail_size(data).

Oops, right, I just tested that we would decrease opt_len "enough" for
the loop to terminate when we meet pad options at the end (which, in
practice, is the only place where we'll find them nowadays), but indeed
it's wrong.

> I think we instead need to check for the 1-byte option cases between
> the two IOV_REMOVE_HEADER() calls.

Right, done.

> And.. since presumably the options could theoretically end with some
> padding options,

That's actually the only place where I've seen the pad option occurring,
in a long while.

> we probably want the loop to be while (opt_len >= 1) instead of 2.

Not strictly needed I think as we don't necessarily want to validate
the last bytes. But anyway:

> In fact the way we
> mix recalculating opt_len from iov_tail_size() with sometimes directly
> updating it is pretty nasty.  We might be better off with
> 	while ((opt_len = iov_tail_size(data)))

...that looks much more natural indeed. I'm using that in v2.

-- 
Stefano

[PATCH 3/4] passt.1, pesto.1: ::1 is an address, not a port

[Stefano Brivio]

Fixes: 4e09ddf03443 ("conf: Allow user-specified auto-scanned port forwarding ranges")
Fixes: cbd58d631db9 ("pesto: Parse and add new rules from command line")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 passt.1 | 2 +-
 pesto.1 | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/passt.1 b/passt.1
index 1570f6a..995590a 100644
--- a/passt.1
+++ b/passt.1
@@ -555,7 +555,7 @@ Automatically forward any ports which are bound in the namespace
 .TP
 -t ::1/auto
 Automatically forward any ports which are bound in the namespace,
-listening only on local port ::1
+listening only on local address ::1
 .TP
 -t 8000-8010,auto
 Forward ports in the range 8000-8010 if and only if they are bound in
diff --git a/pesto.1 b/pesto.1
index c13a18e..3650957 100644
--- a/pesto.1
+++ b/pesto.1
@@ -175,7 +175,7 @@ Automatically forward any ports which are bound in the namespace
 .TP
 -t ::1/auto
 Automatically forward any ports which are bound in the namespace,
-listening only on local port ::1
+listening only on local address ::1
 .TP
 -t 8000-8010,auto
 Forward ports in the range 8000-8010 if and only if they are bound in
-- 
2.43.0

Re: [PATCH 3/4] passt.1, pesto.1: ::1 is an address, not a port

[David Gibson]

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

On Thu, Jul 16, 2026 at 01:25:22AM +0200, Stefano Brivio wrote:
> Fixes: 4e09ddf03443 ("conf: Allow user-specified auto-scanned port forwarding ranges")
> Fixes: cbd58d631db9 ("pesto: Parse and add new rules from command line")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  passt.1 | 2 +-
>  pesto.1 | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/passt.1 b/passt.1
> index 1570f6a..995590a 100644
> --- a/passt.1
> +++ b/passt.1
> @@ -555,7 +555,7 @@ Automatically forward any ports which are bound in the namespace
>  .TP
>  -t ::1/auto
>  Automatically forward any ports which are bound in the namespace,
> -listening only on local port ::1
> +listening only on local address ::1
>  .TP
>  -t 8000-8010,auto
>  Forward ports in the range 8000-8010 if and only if they are bound in
> diff --git a/pesto.1 b/pesto.1
> index c13a18e..3650957 100644
> --- a/pesto.1
> +++ b/pesto.1
> @@ -175,7 +175,7 @@ Automatically forward any ports which are bound in the namespace
>  .TP
>  -t ::1/auto
>  Automatically forward any ports which are bound in the namespace,
> -listening only on local port ::1
> +listening only on local address ::1
>  .TP
>  -t 8000-8010,auto
>  Forward ports in the range 8000-8010 if and only if they are bound in
> -- 
> 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 --]

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

[Stefano Brivio]

...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);
 
 	if (!next_ra)
 		goto first;
-- 
2.43.0

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

[David Gibson]

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

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

[Stefano Brivio]

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.

> 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

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

[David Gibson]

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