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

older
[PATCH 0/6] Fix bug 215 and some...

Stefano Brivio

16 Jul 2026 16 Jul '26

9:22 a.m.

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. v2: Changes suggested by David in 2/4 and 4/4, while 1/4 and 3/4 are unchanged 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 | 22 +++++++++++++--------- ndp.c | 12 ++++++++---- passt.1 | 2 +- pesto.1 | 2 +- 5 files changed, 24 insertions(+), 16 deletions(-) -- 2.43.0

Show replies by date

Stefano Brivio

16 Jul 16 Jul

9:22 a.m.

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

Signed-off-by: Stefano Brivio Reviewed-by: David Gibson --- 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

Stefano Brivio

9:22 a.m.

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

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 - scanning the last two bytes of options as well and using iov_tail_size(data) directly as loop condition, instead of a rather inconsistent usage of opt_len 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 --- v2: - Handle one-byte options before IOV_REMOVE_HEADER() for the length byte - Use iov_tail_size(data) as loop condition instead of mixing things up with opt_len dhcp.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/dhcp.c b/dhcp.c index 1ff8cba..c3c7422 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 @@ -363,25 +363,29 @@ int dhcp(const struct ctx *c, struct iov_tail *data) for (i = 0; i < ARRAY_SIZE(opts); i++) opts[i].clen = -1; - opt_len = iov_tail_size(data); - while (opt_len >= 2) { + while ((opt_len = iov_tail_size(data))) { uint8_t olen_storage, type_storage; const uint8_t *olen; uint8_t *type; - type = IOV_REMOVE_HEADER(data, type_storage); - olen = IOV_REMOVE_HEADER(data, olen_storage); - if (!type || !olen) + if (!(type = IOV_REMOVE_HEADER(data, type_storage))) return -1; - opt_len = iov_tail_size(data); - if (opt_len < *olen) + if (*type == 255) + break; + + if (*type == 0) /* Pad Option (RFC 2132, 3.1): one byte */ + continue; + + if (!(olen = IOV_REMOVE_HEADER(data, olen_storage))) + return -1; + + if (opt_len - 2 < *olen) return -1; iov_to_buf(&data->iov[0], data->cnt, data->off, &opts[*type].c, *olen); opts[*type].clen = *olen; iov_drop_header(data, *olen); - opt_len -= *olen; } opts[80].slen = -1; -- 2.43.0

Stefano Brivio

9:22 a.m.

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

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 Reviewed-by: David Gibson --- 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

Stefano Brivio

9:22 a.m.

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

...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 --- v2: Turn random_part to uint32_t and cast to time_t before using it, to avoid using negative values ndp.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/ndp.c b/ndp.c index 1f2bcb0..439fc0c 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; + uint32_t random_part; if (!tap_is_ready(c) || c->no_ra || now->tv_sec < next_ra) return; @@ -433,15 +434,18 @@ 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); + (time_t)random_part % (max_rtr_adv_interval - + min_rtr_adv_interval); if (!next_ra) goto first; -- 2.43.0

David Gibson

9:34 a.m.

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

On Thu, Jul 16, 2026 at 09:22:22AM +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

Reviewed-by: David Gibson

...

--- v2: Turn random_part to uint32_t and cast to time_t before using it, to avoid using negative values

ndp.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/ndp.c b/ndp.c index 1f2bcb0..439fc0c 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; + uint32_t random_part;

if (!tap_is_ready(c) || c->no_ra || now->tv_sec < next_ra) return; @@ -433,15 +434,18 @@ 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); + (time_t)random_part % (max_rtr_adv_interval - + min_rtr_adv_interval);

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

David Gibson

9:30 a.m.

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

On Thu, Jul 16, 2026 at 09:22:20AM +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

- scanning the last two bytes of options as well and using iov_tail_size(data) directly as loop condition, instead of a rather inconsistent usage of opt_len

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

Reviewed-by: David Gibson

...

--- v2: - Handle one-byte options before IOV_REMOVE_HEADER() for the length byte - Use iov_tail_size(data) as loop condition instead of mixing things up with opt_len

dhcp.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/dhcp.c b/dhcp.c index 1ff8cba..c3c7422 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 @@ -363,25 +363,29 @@ int dhcp(const struct ctx *c, struct iov_tail *data) for (i = 0; i < ARRAY_SIZE(opts); i++) opts[i].clen = -1;

- opt_len = iov_tail_size(data); - while (opt_len >= 2) { + while ((opt_len = iov_tail_size(data))) { uint8_t olen_storage, type_storage; const uint8_t *olen; uint8_t *type;

- type = IOV_REMOVE_HEADER(data, type_storage); - olen = IOV_REMOVE_HEADER(data, olen_storage); - if (!type || !olen) + if (!(type = IOV_REMOVE_HEADER(data, type_storage))) return -1;

- opt_len = iov_tail_size(data); - if (opt_len < *olen) + if (*type == 255) + break; + + if (*type == 0) /* Pad Option (RFC 2132, 3.1): one byte */ + continue; + + if (!(olen = IOV_REMOVE_HEADER(data, olen_storage))) + return -1; + + if (opt_len - 2 < *olen) return -1;

iov_to_buf(&data->iov[0], data->cnt, data->off, &opts[*type].c, *olen); opts[*type].clen = *olen; iov_drop_header(data, *olen); - opt_len -= *olen; }

opts[80].slen = -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

Stefano Brivio

4:31 p.m.

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

On Thu, 16 Jul 2026 09:22:22 +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 --- v2: Turn random_part to uint32_t and cast to time_t before using it, to avoid using negative values

Oops, I guess I ran tests at 3/4 of this series but not on this patch, I'm not quite sure, but now I ran them again and realised that I've been here, done that, around the time of: https://archives.passt.top/passt-dev/20241113091805.6f6e2f6a@elisabeth/ just to find out that this would be the single usage of getrandom(2) at runtime, and we don't include it in the list of system calls we can use. Maybe Laurent tried this as well. Anyway, I guess it's not worth making things actually less secure just to satisfy Coverity Scan here, so I'll drop this patch. -- Stefano

Age (days ago)

Last active (days ago)

List overview

Download

7 comments

2 participants

participants (2)

David Gibson
Stefano Brivio